Feat/automap extension #748
Conversation
![flatfile-nullify[bot]](https://avatars.githubusercontent.com/u/31830338?s=60&v=4){kind=small}
Nullify Code Vulnerabilities1 findings found in this pull request
You can find a list of all findings here |
WalkthroughThe pull request introduces enhancements to the Changes
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
plugins/automap/src/automap.service.ts (5)
47-48: Consider inverting the condition for clarity.
Currently, the code returns if the filename does not match, and otherwise callsupdateFileName('check'). Reversing the condition can make the control flow more intuitive.-if (!this.isFileNameMatch(file)) { - return -} else { - await this.updateFileName('check', file.id) -} +if (this.isFileNameMatch(file)) { + await this.updateFileName('check', file.id) +}🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
201-203: Ensure missing sheet handling is explicit.
Currently, if the workbook has no matching sheet,sheetNamebecomesundefined. Consider logging or handling that case explicitly for easier debugging.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
Line range hint
257-344: Consolidate repeated success/failure handling.
The logic in theswitchstatement for “confident” vs. “exact” accuracy repeats file name updates, logging, and callback invocation for success/failure. Consider extracting the success/failure sequences into helper methods to reduce duplication.- await this.updateFileName('success', file.id, sheetName) - if (!this.isNil(this.options.onSuccess)) { - this.options.onSuccess(event) - } + await this.handleSuccess(file, sheetName, event) ... // similarly for failure🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
515-523: Use global replacement if placeholders can appear multiple times.
Currently,.replace('{{fileName}}', ...)only replaces the first occurrence. If the user supplies multiple{{fileName}}placeholders, only one is replaced.-fileName = fileName.replace('{{fileName}}', variables.fileName) +const re = new RegExp('{{fileName}}', 'g') +fileName = fileName.replace(re, variables.fileName)🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
525-541: Optional chaining for safer checks.
Instead ofif (match && match[1]), you can use optional chaining for more concise code.-if (match && match[1]) { +if (match?.[1]) { return match[1].trim(); }🧰 Tools
🪛 Biome (1.9.4)
[error] 535-535: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
plugins/automap/src/automap.plugin.ts (1)
Line range hint
1-88: Run Prettier to fix formatting.
A pipeline warning indicates code does not meet Prettier standards. Please runprettier --writeto resolve.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.json
📒 Files selected for processing (3)
.changeset/fair-cobras-flow.md(1 hunks)plugins/automap/src/automap.plugin.ts(3 hunks)plugins/automap/src/automap.service.ts(8 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/fair-cobras-flow.md
[uncategorized] ~6-~6: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...re is no unmatched columns (none, both, only-source, only-target) Add options to change fil...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~6-~6: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...tched columns (none, both, only-source, only-target) Add options to change file name in suc...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 GitHub Actions: Lint
plugins/automap/src/automap.plugin.ts
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
plugins/automap/src/automap.service.ts
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
🪛 Biome (1.9.4)
plugins/automap/src/automap.service.ts
[error] 535-535: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup
- GitHub Check: Nullify Dependencies
🔇 Additional comments (8)
plugins/automap/src/automap.service.ts (3)
133-137: File name update on job creation looks correct.
The logic to update the file name upon job initialization is consistent with the chosen “start” stage and destination sheet name.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
223-227: Verify correct file selection in multi-file scenarios.
This approach selects the first file found that matches the current workbook. If multiple files share the same workbook, ensure that this logic is indeed what you want (i.e., that you truly want the first match).Would you like me to generate a script to scan the repository for all references to
files.list()calls, to confirm consistent usage and potential filtering of multiple files?🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
421-447: Column matching approach looks good.
The logic inverifyMappedColumnsis straightforward, systematically checking for unmapped fields according to the configured mode.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
plugins/automap/src/automap.plugin.ts (4)
10-11: Instantiating service with defaulted options looks good.
CallingdefaultOptionsbefore creating theAutomapServiceensures consistent defaults.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
25-35: Documentation of new fields is clear.
The doc comments for the new properties (e.g.,allColumnsMustBeMapped,disableFileNameUpdateOnSuccess) are well structured. No major issues found.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
44-54: New interface properties align with usage.
The additions toAutomapOptionsmatch the code inautomap.service.ts. This satisfies the new feature requirements.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
57-86: Defaulting strategy is well implemented.
Applying defaults and ensuring{{fileName}}is included in each pattern is correct. Consider also verifying{{destinationSheetName}}presence if it's required.🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
.changeset/fair-cobras-flow.md (1)
1-8: Minor style note regarding hyphen usage.
Static analysis flags “only-source” and “only-target” for hyphenation, but these are valid identifiers in this context. It’s safe to ignore the “HYPHENATED_LY_ADVERB_ADJECTIVE” suggestion since these are literal config values.🧰 Tools
🪛 LanguageTool
[uncategorized] ~6-~6: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...re is no unmatched columns (none, both, only-source, only-target) Add options to change fil...(HYPHENATED_LY_ADVERB_ADJECTIVE)
[uncategorized] ~6-~6: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...tched columns (none, both, only-source, only-target) Add options to change file name in suc...(HYPHENATED_LY_ADVERB_ADJECTIVE)
| private async updateFileName( | ||
| stage: 'manual' | 'check' | 'start' | 'success' | 'failure', | ||
| fileId: string, | ||
| fileName: string | ||
| destinationSheetName?: string | ||
| ): Promise<Flatfile.FileResponse> { | ||
| return api.files.update(fileId, { name: fileName }) | ||
|
|
||
| const file = await api.files.get(fileId) | ||
| const currentFileName = file.data.name | ||
| let newFileName = '' | ||
| if(this.options.disableFileNameUpdate) { | ||
| return | ||
| } | ||
|
|
||
| if(stage === 'check') { | ||
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnCheck, {fileName: currentFileName}) | ||
| return api.files.update(fileId, { name: newFileName }) | ||
| } | ||
|
|
||
| if(stage === 'start') { | ||
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnCheck, currentFileName) | ||
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnStart, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | ||
| return api.files.update(fileId, { name: newFileName }) | ||
| } | ||
|
|
||
| if(stage === 'success' && !this.options.disableFileNameUpdateOnSuccess) { | ||
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnStart, currentFileName) | ||
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnSuccess, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | ||
| return api.files.update(fileId, { name: newFileName }) | ||
| } | ||
|
|
||
| if(stage === 'failure' && !this.options.disableFileNameUpdateOnFailure) { | ||
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnStart, currentFileName) | ||
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnFailure, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | ||
| return api.files.update(fileId, { name: newFileName }) | ||
| } | ||
|
|
||
| return |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return type mismatch could cause confusion.
This method defines a return type of Promise<Flatfile.FileResponse>, but there's a path that returns undefined. Consider widening the return type or always returning a FileResponse.
-): Promise<Flatfile.FileResponse> {
+): Promise<Flatfile.FileResponse | void> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async updateFileName( | |
| stage: 'manual' | 'check' | 'start' | 'success' | 'failure', | |
| fileId: string, | |
| fileName: string | |
| destinationSheetName?: string | |
| ): Promise<Flatfile.FileResponse> { | |
| return api.files.update(fileId, { name: fileName }) | |
| const file = await api.files.get(fileId) | |
| const currentFileName = file.data.name | |
| let newFileName = '' | |
| if(this.options.disableFileNameUpdate) { | |
| return | |
| } | |
| if(stage === 'check') { | |
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnCheck, {fileName: currentFileName}) | |
| return api.files.update(fileId, { name: newFileName }) | |
| } | |
| if(stage === 'start') { | |
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnCheck, currentFileName) | |
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnStart, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | |
| return api.files.update(fileId, { name: newFileName }) | |
| } | |
| if(stage === 'success' && !this.options.disableFileNameUpdateOnSuccess) { | |
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnStart, currentFileName) | |
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnSuccess, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | |
| return api.files.update(fileId, { name: newFileName }) | |
| } | |
| if(stage === 'failure' && !this.options.disableFileNameUpdateOnFailure) { | |
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnStart, currentFileName) | |
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnFailure, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | |
| return api.files.update(fileId, { name: newFileName }) | |
| } | |
| return | |
| private async updateFileName( | |
| stage: 'manual' | 'check' | 'start' | 'success' | 'failure', | |
| fileId: string, | |
| destinationSheetName?: string | |
| ): Promise<Flatfile.FileResponse | void> { | |
| const file = await api.files.get(fileId) | |
| const currentFileName = file.data.name | |
| let newFileName = '' | |
| if(this.options.disableFileNameUpdate) { | |
| return | |
| } | |
| if(stage === 'check') { | |
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnCheck, {fileName: currentFileName}) | |
| return api.files.update(fileId, { name: newFileName }) | |
| } | |
| if(stage === 'start') { | |
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnCheck, currentFileName) | |
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnStart, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | |
| return api.files.update(fileId, { name: newFileName }) | |
| } | |
| if(stage === 'success' && !this.options.disableFileNameUpdateOnSuccess) { | |
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnStart, currentFileName) | |
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnSuccess, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | |
| return api.files.update(fileId, { name: newFileName }) | |
| } | |
| if(stage === 'failure' && !this.options.disableFileNameUpdateOnFailure) { | |
| const oldFileName = this.getFileNameFromOldFileName(this.options.filenameOnStart, currentFileName) | |
| newFileName = this.resolveVariablesInFileName(this.options.filenameOnFailure, {fileName: oldFileName, destinationSheetName: destinationSheetName}) | |
| return api.files.update(fileId, { name: newFileName }) | |
| } | |
| return |
🧰 Tools
🪛 GitHub Actions: Lint
[warning] Code formatting does not meet Prettier standards. Run Prettier with --write to fix.
| ) | ||
|
|
||
| // Create a safe regex pattern that matches the exact structure | ||
| const safeRegex = new RegExp(`^${beforeRegex}(.*?)${afterRegex}$`) |
There was a problem hiding this comment.
Nullify Code Language: TypeScript 🔵 MEDIUM Severity CWE-185
Javascript dos rule non literal regexp
The RegExp constructor was called with a non-literal value. If an adversary were able to
supply a malicious regex, they could cause a Regular Expression Denial of Service (ReDoS)
against the application. In Node applications, this could cause the entire application to no
longer be responsive to other users' requests.
To remediate this issue, never allow user-supplied regular expressions. Instead, the regular
expression should be hardcoded. If this is not possible, consider using an alternative regular
expression engine such as node-re2. RE2 is a safe alternative
that does not support backtracking, which is what leads to ReDoS.
Example using re2 which does not support backtracking (Note: it is still recommended to
never use user-supplied input):
// Import the re2 module
const RE2 = require('re2');
function match(userSuppliedRegex, userInput) {
// Create a RE2 object with the user supplied regex, this is relatively safe
// due to RE2 not supporting backtracking which can be abused to cause long running
// queries
var re = new RE2(userSuppliedRegex);
// Execute the regular expression against some userInput
var result = re.exec(userInput);
// Work with the result
}
For more information on Regular Expression DoS see:
Reply with /nullify to interact with me like another developer
(you will need to refresh the page for updates)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
plugins/automap/src/automap.plugin.ts (3)
10-11: Consider using type assertion for defaulted options.The defaultOptions function returns a complete AutomapOptions object, but TypeScript doesn't track this. Consider using type assertion to maintain type safety:
- const optionsDefaulted = defaultOptions(options) + const optionsDefaulted = defaultOptions(options) as Required<AutomapOptions>
44-59: Consider grouping related options using nested interfaces.The interface has grown with many related properties. Consider organizing them into nested interfaces for better maintainability:
+interface FileNameOptions { + readonly disableFileNameUpdate?: boolean + readonly disableFileNameUpdateOnSuccess?: boolean + readonly disableFileNameUpdateOnFailure?: boolean + readonly filenameOnCheck?: string + readonly filenameOnStart?: string + readonly filenameOnSuccess?: string + readonly filenameOnFailure?: string +} export interface AutomapOptions { readonly accuracy: 'confident' | 'exact' readonly debug?: boolean readonly defaultTargetSheet?: | string | ((fileName?: string, event?: FlatfileEvent) => string | Promise<string>) readonly matchFilename?: RegExp readonly allColumnsMustBeMapped?: | 'none' | 'both' | 'only-source' | 'only-target' readonly onSuccess?: (event: FlatfileEvent) => void readonly onFailure?: (event: FlatfileEvent) => void readonly targetWorkbook?: string - readonly disableFileNameUpdate?: boolean - readonly disableFileNameUpdateOnSuccess?: boolean - readonly disableFileNameUpdateOnFailure?: boolean - readonly filenameOnCheck?: string - readonly filenameOnStart?: string - readonly filenameOnSuccess?: string - readonly filenameOnFailure?: string + readonly fileNameOptions?: FileNameOptions }
77-95: Reduce code duplication in filename pattern handling.The code for checking and appending {{fileName}} is repeated four times. Consider extracting this into a helper function:
+ private ensureFileNameVariable(pattern: string): string { + return pattern.includes('{{fileName}}') ? pattern : `${pattern} {{fileName}}` + } if (!defaultedOptions.filenameOnCheck.includes('{{fileName}}')) { - defaultedOptions.filenameOnCheck = - defaultedOptions.filenameOnCheck + ' {{fileName}}' + defaultedOptions.filenameOnCheck = ensureFileNameVariable(defaultedOptions.filenameOnCheck) } // Apply similar changes to other filename patternsplugins/automap/src/automap.service.ts (4)
46-47: Add error handling for file name updates.The file name updates could fail but errors are not handled. Consider wrapping these in try-catch blocks:
} else { - await this.updateFileName('check', file.id) + try { + await this.updateFileName('check', file.id) + } catch (error) { + logWarn('@flatfile/plugin-automap', `Failed to update file name: ${error}`) + } } // Similar changes for the other updateFileName callAlso applies to: 132-133
398-425: Simplify column mapping verification logic.The current implementation can be simplified using early returns and object mapping:
private verifyMappedColumns(plan: Flatfile.JobExecutionPlan): boolean { - let mappedColumnsVerified = false - - if (this.options.allColumnsMustBeMapped === 'none') { - mappedColumnsVerified = true - } - if ( - this.options.allColumnsMustBeMapped === 'both' && - plan.unmappedDestinationFields?.length === 0 && - plan.unmappedSourceFields?.length === 0 - ) { - mappedColumnsVerified = true - } - if ( - this.options.allColumnsMustBeMapped === 'only-source' && - plan.unmappedSourceFields?.length === 0 - ) { - mappedColumnsVerified = true - } - if ( - this.options.allColumnsMustBeMapped === 'only-target' && - plan.unmappedDestinationFields?.length === 0 - ) { - mappedColumnsVerified = true - } - - return mappedColumnsVerified + const verificationMap = { + 'none': () => true, + 'both': () => + plan.unmappedDestinationFields?.length === 0 && + plan.unmappedSourceFields?.length === 0, + 'only-source': () => + plan.unmappedSourceFields?.length === 0, + 'only-target': () => + plan.unmappedDestinationFields?.length === 0 + } + + return verificationMap[this.options.allColumnsMustBeMapped]?.() ?? false }
449-454: Improve return type clarity.The method's return type is confusing as flagged by static analysis. Consider using undefined instead of void:
private async updateFileName( stage: 'manual' | 'check' | 'start' | 'success' | 'failure', fileId: string, destinationSheetName?: string - ): Promise<Flatfile.FileResponse | void> { + ): Promise<Flatfile.FileResponse | undefined> {🧰 Tools
🪛 Biome (1.9.4)
[error] 453-453: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
512-526: Add input validation for file name variables.The method should validate inputs to prevent potential issues:
private resolveVariablesInFileName( fileName: string, variables: { destinationSheetName?: string; fileName?: string } ): string { + if (!fileName) { + throw new Error('fileName is required') + } // Rest of the code... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/automap/src/automap.plugin.ts(3 hunks)plugins/automap/src/automap.service.ts(7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
plugins/automap/src/automap.service.ts
[error] 453-453: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup
- GitHub Check: Nullify Dependencies
| private async getFileNameFromOldFileName( | ||
| pattern: string, | ||
| filename: string | ||
| ): Promise<string> { | ||
| try { | ||
| // Find the position of {{fileName}} in the pattern | ||
| const fileNameMatch = pattern.match(/{{fileName}}/) | ||
| if (!fileNameMatch) { | ||
| return filename | ||
| } | ||
|
|
||
| // Get the text before and after {{fileName}} | ||
| const beforePattern = pattern.substring(0, fileNameMatch.index) | ||
| const afterPattern = pattern.substring( | ||
| fileNameMatch.index + '{{fileName}}'.length | ||
| ) | ||
|
|
||
| // Escape special characters in the before/after patterns | ||
| const beforeRegex = this.escapeRegExp(beforePattern) | ||
| const afterRegex = this.escapeRegExp(afterPattern).replace( | ||
| this.escapeRegExp('{{destinationSheetName}}'), | ||
| '.*?' | ||
| ) | ||
|
|
||
| // Create a safe regex pattern that matches the exact structure | ||
| const safeRegex = new RegExp(`^${beforeRegex}(.*?)${afterRegex}$`) | ||
|
|
||
| // Add timeout protection | ||
| const MAX_EXECUTION_TIME = 100 | ||
| const timeoutPromise = new Promise<string>((_, reject) => { | ||
| setTimeout(() => reject(new Error('Regex timeout')), MAX_EXECUTION_TIME) | ||
| }) | ||
|
|
||
| const matchPromise = new Promise<string>((resolve) => { | ||
| const match = filename.match(safeRegex) | ||
| if (match?.[1]) { | ||
| resolve(match[1].trim()) | ||
| } else { | ||
| resolve(filename) | ||
| } | ||
| }) | ||
|
|
||
| return Promise.race([matchPromise, timeoutPromise]) | ||
| .then((result) => result) | ||
| .catch(() => filename) | ||
| } catch (error) { | ||
| return filename | ||
| } | ||
| } |
There was a problem hiding this comment.
Address potential ReDoS vulnerability.
The method constructs a regex from user input which could lead to ReDoS attacks. Consider adding input validation and using a safe regex library:
+ private validatePattern(pattern: string): boolean {
+ return pattern.length < 1000 && !/[^\w\s{{}}]/.test(pattern)
+ }
private async getFileNameFromOldFileName(
pattern: string,
filename: string
): Promise<string> {
try {
+ if (!this.validatePattern(pattern)) {
+ return filename
+ }
// Rest of the code...
}
}Also consider using a safe regex library like RE2 as suggested by the static analysis tool.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private async getFileNameFromOldFileName( | |
| pattern: string, | |
| filename: string | |
| ): Promise<string> { | |
| try { | |
| // Find the position of {{fileName}} in the pattern | |
| const fileNameMatch = pattern.match(/{{fileName}}/) | |
| if (!fileNameMatch) { | |
| return filename | |
| } | |
| // Get the text before and after {{fileName}} | |
| const beforePattern = pattern.substring(0, fileNameMatch.index) | |
| const afterPattern = pattern.substring( | |
| fileNameMatch.index + '{{fileName}}'.length | |
| ) | |
| // Escape special characters in the before/after patterns | |
| const beforeRegex = this.escapeRegExp(beforePattern) | |
| const afterRegex = this.escapeRegExp(afterPattern).replace( | |
| this.escapeRegExp('{{destinationSheetName}}'), | |
| '.*?' | |
| ) | |
| // Create a safe regex pattern that matches the exact structure | |
| const safeRegex = new RegExp(`^${beforeRegex}(.*?)${afterRegex}$`) | |
| // Add timeout protection | |
| const MAX_EXECUTION_TIME = 100 | |
| const timeoutPromise = new Promise<string>((_, reject) => { | |
| setTimeout(() => reject(new Error('Regex timeout')), MAX_EXECUTION_TIME) | |
| }) | |
| const matchPromise = new Promise<string>((resolve) => { | |
| const match = filename.match(safeRegex) | |
| if (match?.[1]) { | |
| resolve(match[1].trim()) | |
| } else { | |
| resolve(filename) | |
| } | |
| }) | |
| return Promise.race([matchPromise, timeoutPromise]) | |
| .then((result) => result) | |
| .catch(() => filename) | |
| } catch (error) { | |
| return filename | |
| } | |
| } | |
| private validatePattern(pattern: string): boolean { | |
| return pattern.length < 1000 && !/[^\w\s{{}}]/.test(pattern) | |
| } | |
| private async getFileNameFromOldFileName( | |
| pattern: string, | |
| filename: string | |
| ): Promise<string> { | |
| try { | |
| if (!this.validatePattern(pattern)) { | |
| return filename | |
| } | |
| // Find the position of {{fileName}} in the pattern | |
| const fileNameMatch = pattern.match(/{{fileName}}/) | |
| if (!fileNameMatch) { | |
| return filename | |
| } | |
| // Get the text before and after {{fileName}} | |
| const beforePattern = pattern.substring(0, fileNameMatch.index) | |
| const afterPattern = pattern.substring( | |
| fileNameMatch.index + '{{fileName}}'.length | |
| ) | |
| // Escape special characters in the before/after patterns | |
| const beforeRegex = this.escapeRegExp(beforePattern) | |
| const afterRegex = this.escapeRegExp(afterPattern).replace( | |
| this.escapeRegExp('{{destinationSheetName}}'), | |
| '.*?' | |
| ) | |
| // Create a safe regex pattern that matches the exact structure | |
| const safeRegex = new RegExp(`^${beforeRegex}(.*?)${afterRegex}$`) | |
| // Add timeout protection | |
| const MAX_EXECUTION_TIME = 100 | |
| const timeoutPromise = new Promise<string>((_, reject) => { | |
| setTimeout(() => reject(new Error('Regex timeout')), MAX_EXECUTION_TIME) | |
| }) | |
| const matchPromise = new Promise<string>((resolve) => { | |
| const match = filename.match(safeRegex) | |
| if (match?.[1]) { | |
| resolve(match[1].trim()) | |
| } else { | |
| resolve(filename) | |
| } | |
| }) | |
| return Promise.race([matchPromise, timeoutPromise]) | |
| .then((result) => result) | |
| .catch(() => filename) | |
| } catch (error) { | |
| return filename | |
| } | |
| } |
carlbrugger
left a comment
carlbrugger
left a comment
There was a problem hiding this comment.
Good work 👍🏼 Let make it a major release.
| @@ -0,0 +1,8 @@ | |||
| --- | |||
| '@flatfile/plugin-automap': minor | |||
There was a problem hiding this comment.
Let change this to a major
Please explain how to summarize this PR for the Changelog:
This PR adds a couple of new options for the automap plugin:
onSuccess?: (event: FlatfileEvent) => voidOption for callback in success case
allColumnsMustBeMapped?: 'none' | 'both' | 'only-source' | 'only-target'Option that enables a check prior to the accuracy verification, to check if there are any unmatched columns (source or target sheet, none or both)
Options to override the file name on update
Options to disbale updating the file name on success or failure of automap plugin
Tell code reviewer how and what to test:
These changes are backwards competible. So the above usage should result in the same behaviour as before.
Setting those new options should result in:
Mapping should fail for files where either the source sheet or the destination sheet have unmatched columns
Console messages should be outputted in success and failure case.