make local builds respect "ignore" files when uploading the built output#10438
Conversation
…put to GCS for zip deploys
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the ignore pattern logic in src/deploy/apphosting/util.ts by centralizing it into a new resolveIgnorePatterns function, which is now utilized by both createLocalBuildTarArchive and createSourceDeployArchive. Additionally, it introduces unit tests to ensure that ignore patterns from configuration and .gitignore files are correctly respected. Feedback includes a recommendation to avoid using 'as any' in test files to comply with the repository's style guide and a suggestion to remove the export keyword from the new utility function if it is only intended for internal use.
… before we actually build it
…builds_ignore_files
…ase-tools into local_builds_ignore_files
…builds_ignore_files
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
![wiz-9635d3485b[bot]](https://avatars.githubusercontent.com/u/1335026?s=60&v=4){kind=small}
…ild" folder already exists
| const rootDir = options.projectRoot || process.cwd(); | ||
| for (const cfg of Object.values(context.backendConfigs)) { | ||
| if (cfg.localBuild) { | ||
| const localBuildDir = path.join(rootDir, "local_build"); |
There was a problem hiding this comment.
Path Traversal Vulnerability in Node.js (CWE-22)
More Details
This rule detects potential path traversal vulnerabilities in Node.js applications. Path traversal vulnerabilities occur when user input is passed unsanitized to file system operations, allowing attackers to access arbitrary files and directories on the server.
This issue presents a significant security risk as it can lead to unauthorized access to sensitive data, code execution, and complete system compromise. If exploited, an attacker could read confidential files, modify system files, or even execute malicious code on the server.
| Attribute | Value |
|---|---|
| Impact |  |
| Likelihood | |
Remediation
Path traversal vulnerabilities allow an attacker to access arbitrary files on the system, potentially exposing sensitive data or enabling further attacks. This vulnerability arises when user input is passed unsanitized to path manipulation functions like path.join or path.resolve, allowing an attacker to traverse the file system using patterns like ../.
To fix this issue, user input should be sanitized or validated before being passed to path manipulation functions. This can be done by using allowlists, removing or encoding special characters, or validating the resulting path against a set of allowed directories.
Code examples
// VULNERABLE CODE - User input is passed directly to path.join
const userInput = req.query.file;
const filePath = path.join(__dirname, userInput);
fs.readFile(filePath, (err, data) => { ... });// SECURE CODE - User input is sanitized before path manipulation
const userInput = req.query.file;
const sanitizedInput = sanitizeInput(userInput); // Implement sanitization logic
const filePath = path.join(__dirname, sanitizedInput);
fs.readFile(filePath, (err, data) => { ... });Additional recommendations
- Follow the principle of least privilege and restrict file access as much as possible.
- Use allowlists instead of denylist-based input validation when possible.
- Consider using libraries like
path-sanitizerorsanitize-filenamefor input sanitization. - Adhere to the OWASP Input Validation Cheat Sheet and other relevant security standards.
- As an alternative approach, consider using a virtual file system or sandboxing techniques to isolate file operations from the main system.
Rule ID: WS-I013-JAVASCRIPT-00098
To ignore this finding as an exception, reply to this conversation with #wiz_ignore reason
If you'd like to ignore this finding in all future scans, add an exception in the .wiz file (learn more) or create an Ignore Rule (learn more).
To get more details on how to remediate this issue using AI, reply to this conversation with #wiz remediate
| await injectAutoInitEnvVars(cfg, backends, buildEnv, runtimeEnv); | ||
|
|
||
| const rootDir = options.projectRoot || process.cwd(); | ||
| const localBuildDir = path.join(rootDir, "local_build"); |
There was a problem hiding this comment.
Path Traversal Vulnerability in Node.js (CWE-22)
More Details
This rule detects potential path traversal vulnerabilities in Node.js applications. Path traversal vulnerabilities occur when user input is passed unsanitized to file system operations, allowing attackers to access arbitrary files and directories on the server.
This issue presents a significant security risk as it can lead to unauthorized access to sensitive data, code execution, and complete system compromise. If exploited, an attacker could read confidential files, modify system files, or even execute malicious code on the server.
| Attribute | Value |
|---|---|
| Impact | |
| Likelihood | |
Remediation
Path traversal vulnerabilities allow an attacker to access arbitrary files on the system, potentially exposing sensitive data or enabling further attacks. This vulnerability arises when user input is passed unsanitized to path manipulation functions like path.join or path.resolve, allowing an attacker to traverse the file system using patterns like ../.
To fix this issue, user input should be sanitized or validated before being passed to path manipulation functions. This can be done by using allowlists, removing or encoding special characters, or validating the resulting path against a set of allowed directories.
Code examples
// VULNERABLE CODE - User input is passed directly to path.join
const userInput = req.query.file;
const filePath = path.join(__dirname, userInput);
fs.readFile(filePath, (err, data) => { ... });// SECURE CODE - User input is sanitized before path manipulation
const userInput = req.query.file;
const sanitizedInput = sanitizeInput(userInput); // Implement sanitization logic
const filePath = path.join(__dirname, sanitizedInput);
fs.readFile(filePath, (err, data) => { ... });Additional recommendations
- Follow the principle of least privilege and restrict file access as much as possible.
- Use allowlists instead of denylist-based input validation when possible.
- Consider using libraries like
path-sanitizerorsanitize-filenamefor input sanitization. - Adhere to the OWASP Input Validation Cheat Sheet and other relevant security standards.
- As an alternative approach, consider using a virtual file system or sandboxing techniques to isolate file operations from the main system.
Rule ID: WS-I013-JAVASCRIPT-00098
To ignore this finding as an exception, reply to this conversation with #wiz_ignore reason
If you'd like to ignore this finding in all future scans, add an exception in the .wiz file (learn more) or create an Ignore Rule (learn more).
To get more details on how to remediate this issue using AI, reply to this conversation with #wiz remediate
|
|
||
| for (const file of filesToCopy) { | ||
| const relativePath = path.relative(rootDir, file.name); | ||
| const destPath = path.join(localBuildDir, relativePath); |
There was a problem hiding this comment.
Path Traversal Vulnerability in Node.js (CWE-22)
More Details
This rule detects potential path traversal vulnerabilities in Node.js applications. Path traversal vulnerabilities occur when user input is passed unsanitized to file system operations, allowing attackers to access arbitrary files and directories on the server.
This issue presents a significant security risk as it can lead to unauthorized access to sensitive data, code execution, and complete system compromise. If exploited, an attacker could read confidential files, modify system files, or even execute malicious code on the server.
| Attribute | Value |
|---|---|
| Impact | |
| Likelihood | |
Remediation
Path traversal vulnerabilities allow an attacker to access arbitrary files on the system, potentially exposing sensitive data or enabling further attacks. This vulnerability arises when user input is passed unsanitized to path manipulation functions like path.join or path.resolve, allowing an attacker to traverse the file system using patterns like ../.
To fix this issue, user input should be sanitized or validated before being passed to path manipulation functions. This can be done by using allowlists, removing or encoding special characters, or validating the resulting path against a set of allowed directories.
Code examples
// VULNERABLE CODE - User input is passed directly to path.join
const userInput = req.query.file;
const filePath = path.join(__dirname, userInput);
fs.readFile(filePath, (err, data) => { ... });// SECURE CODE - User input is sanitized before path manipulation
const userInput = req.query.file;
const sanitizedInput = sanitizeInput(userInput); // Implement sanitization logic
const filePath = path.join(__dirname, sanitizedInput);
fs.readFile(filePath, (err, data) => { ... });Additional recommendations
- Follow the principle of least privilege and restrict file access as much as possible.
- Use allowlists instead of denylist-based input validation when possible.
- Consider using libraries like
path-sanitizerorsanitize-filenamefor input sanitization. - Adhere to the OWASP Input Validation Cheat Sheet and other relevant security standards.
- As an alternative approach, consider using a virtual file system or sandboxing techniques to isolate file operations from the main system.
Rule ID: WS-I013-JAVASCRIPT-00098
To ignore this finding as an exception, reply to this conversation with #wiz_ignore reason
If you'd like to ignore this finding in all future scans, add an exception in the .wiz file (learn more) or create an Ignore Rule (learn more).
To get more details on how to remediate this issue using AI, reply to this conversation with #wiz remediate
|
|
||
| for (const file of filesToCopy) { | ||
| const relativePath = path.relative(rootDir, file.name); | ||
| const destPath = path.join(localBuildDir, relativePath); |
There was a problem hiding this comment.
Path Traversal Vulnerability in Node.js (CWE-22)
More Details
This rule detects potential path traversal vulnerabilities in Node.js applications. Path traversal vulnerabilities occur when user input is passed unsanitized to file system operations, allowing attackers to access arbitrary files and directories on the server.
This issue presents a significant security risk as it can lead to unauthorized access to sensitive data, code execution, and complete system compromise. If exploited, an attacker could read confidential files, modify system files, or even execute malicious code on the server.
| Attribute | Value |
|---|---|
| Impact | |
| Likelihood | |
Remediation
Path traversal vulnerabilities allow an attacker to access arbitrary files on the system, potentially exposing sensitive data or enabling further attacks. This vulnerability arises when user input is passed unsanitized to path manipulation functions like path.join or path.resolve, allowing an attacker to traverse the file system using patterns like ../.
To fix this issue, user input should be sanitized or validated before being passed to path manipulation functions. This can be done by using allowlists, removing or encoding special characters, or validating the resulting path against a set of allowed directories.
Code examples
// VULNERABLE CODE - User input is passed directly to path.join
const userInput = req.query.file;
const filePath = path.join(__dirname, userInput);
fs.readFile(filePath, (err, data) => { ... });// SECURE CODE - User input is sanitized before path manipulation
const userInput = req.query.file;
const sanitizedInput = sanitizeInput(userInput); // Implement sanitization logic
const filePath = path.join(__dirname, sanitizedInput);
fs.readFile(filePath, (err, data) => { ... });Additional recommendations
- Follow the principle of least privilege and restrict file access as much as possible.
- Use allowlists instead of denylist-based input validation when possible.
- Consider using libraries like
path-sanitizerorsanitize-filenamefor input sanitization. - Adhere to the OWASP Input Validation Cheat Sheet and other relevant security standards.
- As an alternative approach, consider using a virtual file system or sandboxing techniques to isolate file operations from the main system.
Rule ID: WS-I013-JAVASCRIPT-00098
To ignore this finding as an exception, reply to this conversation with #wiz_ignore reason
If you'd like to ignore this finding in all future scans, add an exception in the .wiz file (learn more) or create an Ignore Rule (learn more).
To get more details on how to remediate this issue using AI, reply to this conversation with #wiz remediate
…builds_ignore_files
* For local builds, we apply all ignore filtering (user firebase.json ignores and .gitignore) * BEFORE the build runs, right here during the directory copying phase. This creates a clean, * isolated workspace in the `.local_build` folder that contains exactly the same source files * that would be uploaded to Cloud Build.
| ): Promise<void> { | ||
| // Resolve ignores for local builds, including default node_modules ignore | ||
| const ignore = resolveIgnorePatterns(cfg); | ||
| ignore.push(path.basename(localBuildScratchDir)); // Always ignore the build directory itself |
There was a problem hiding this comment.
it looks like you're creating this in the project rootDir (where the firebase.json lives) (or cwd)
A single firebase.json and a single project directory can be associated with multiple app hosting backends. I believe this was useful for:
- monorepo projects => in which case the cfg.rootDir would be different
- but also for when a single app has both staging/prod app hosting backends associated with it => in which case their cfg.rootDir would be the same
also IIUC the CLI allows for deploying multiple backends at once. (Though I forget if this happens sequentially or async, i'd hope the latter for perf reasons)
tl;dr there might be a chance of another FAH backend's scratch dir getting included.
i think just updating this to ignore all dirs with the scratchdir prefix would cleanly solve this.
There was a problem hiding this comment.
ah good catch, we might include the other build dirs.
to support parallel builds I actually changed it so the local build folder is unique to each backend. it's .local_build_{backend_id}. I'm going to also explicitly skip the other local build folders.
There was a problem hiding this comment.
Done, I added the local build ignore lists by default
| if (fs.existsSync(localBuildScratchDir)) { | ||
| throw new FirebaseError( | ||
| `The local build scratch directory '${localBuildScratchDir}' already exists. Please delete it and try again.`, | ||
| ); |
There was a problem hiding this comment.
I only see code to cleanup the scratchDir in release.ts
do we want to clean up scratchDir if the actual build here fails too? or was it not deleted to let the user debug the failure? if so, i think it's worth noting in a comment somewhere (maybe closer to the local build? no strong opinion)
a p2 optimization in that case is we ask the user for a simple "y" confirmation before deleting it for them.
There was a problem hiding this comment.
Ah, so prepare.ts is the first thing that runs.
If you run local builds but this folder already exists, we fail it so that we dont accidentally clobber user data. It's unlikely, but it's just in case they already had a folder named something like ".local_build...".
However, if it doesn't exist, we will correctly create/set it up in prepare.ts. For the rest of the deployment, we expect the folder to exist.
I like the idea of allowing them to confirm it if it's interactive mode. I might land that after this PR separately though
annajowang
left a comment
annajowang
left a comment
There was a problem hiding this comment.
I'm gonna stamp this to reduce roundtrips and because this is experimental.
3 participants
Description
We already respect the "ignore" entry in firebase.json for source deploys and we want to do the same for local builds.
One crucial difference. For local builds, we apply this filter AFTER the build, on the built output. We can not prevent files from being included in the build.
Scenarios Tested