ci(security): integrate Bright CI pipeline for security tests and remediation#817
ci(security): integrate Bright CI pipeline for security tests and remediation#817bright-security-golf[bot] wants to merge 16 commits intostablefrom
Conversation
skip-checks:true
skip-checks:true
76ffcc6 to
7850fa7
Compare
skip-checks:true
skip-checks:true
skip-checks:true
| throw new Error('cannot delete file from this location'); | ||
| } else { | ||
| file = path.resolve(process.cwd(), file); | ||
| await fs.promises.unlink(file); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, to fix uncontrolled path usage you must (1) decide on a safe root directory, (2) resolve the user-supplied path against that root, (3) normalize / canonicalize it (e.g., with path.resolve and optionally fs.realpath), and (4) reject the operation if the final path is outside the root. Alternatively, for very restrictive APIs, you can sanitize to a simple filename or use an allow‑list of patterns.
For this codebase, the least invasive fix that preserves existing behavior is to keep using process.cwd() as the logical root, but enforce that after resolving, the target path is still within that root. That means in FileService.deleteFile, after file = path.resolve(process.cwd(), file);, we compute const root = path.resolve(process.cwd()); and verify file.startsWith(root + path.sep) or equals root. If it does not, we throw an error (or a 400/403 via InternalServerErrorException is already used). This prevents ../ tricks from escaping the working directory while keeping relative paths functional. Optionally, we can apply the same containment check in getFile for the non-absolute, non-HTTP branch to make reading consistent and safe.
Concretely:
- Edit
src/file/file.service.ts. - In
deleteFile, introduce aconst rootDir = path.resolve(process.cwd());before resolvingfile, then resolvefileagainstrootDir, and add a containment check before callingfs.promises.unlink. - Similarly, in
getFile, for theelsebranch (relative paths), resolve againstrootDirand add the same containment check beforefs.promises.access/createReadStream. - No new methods or external libraries are required; we only use Node’s built‑in
path(already imported) andfs.
| @@ -26,8 +26,13 @@ | ||
| throw new Error(`no such file or directory, access '${file}'`); | ||
| } | ||
| } else { | ||
| file = path.resolve(process.cwd(), file); | ||
| const rootDir = path.resolve(process.cwd()); | ||
| file = path.resolve(rootDir, file); | ||
|
|
||
| if (!file.startsWith(rootDir + path.sep) && file !== rootDir) { | ||
| throw new Error('access to this location is not allowed'); | ||
| } | ||
|
|
||
| await fs.promises.access(file, R_OK); | ||
|
|
||
| return fs.createReadStream(file); | ||
| @@ -41,7 +45,13 @@ | ||
| } else if (file.startsWith('http')) { | ||
| throw new Error('cannot delete file from this location'); | ||
| } else { | ||
| file = path.resolve(process.cwd(), file); | ||
| const rootDir = path.resolve(process.cwd()); | ||
| file = path.resolve(rootDir, file); | ||
|
|
||
| if (!file.startsWith(rootDir + path.sep) && file !== rootDir) { | ||
| throw new Error('cannot delete file from this location'); | ||
| } | ||
|
|
||
| await fs.promises.unlink(file); | ||
| return true; | ||
| } |
Note
Fixed 15 of 19 vulnerabilities.
Please review the fixes before merging.
GET /api/partners/searchPartnersGET /api/partners/partnerLoginPOST /api/renderGET /api/file/googleGET /api/users/id/1GET /api/fileGET /api/fileGET /api/file/digital_oceanGET /api/file/awsGET /api/file/azureGET /api/secretsPOST /graphqlPOST /graphqlDELETE /api/fileGET /api/fileGET /api/products/latestGET /api/configPOST /graphqlPOST /graphqlWorkflow execution details