Feat/sp 3755 configurable scan path#55
Merged
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR bumps the extension to 1.7.0, adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
agustingroh
force-pushed
the
feat/SP-3755-configurable-scan-path
branch
2 times, most recently
from
e57c7b3 to
4b7d503
Compare
![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 (1)
codescantask/services/scan.service.ts (1)
299-301: PreferscanPathinOptionsover module-globalSCAN_PATH.Line 300 currently reads from a global input constant, which makes
ScanServiceharder to unit-test/configure via constructor options.♻️ Suggested refactor
export interface Options { @@ /** * Absolute path of the folder or file to scan. Required. */ inputFilepath: string; + /** + * Relative path inside the mounted repository to scan. Default [.] + */ + scanPath: string; @@ this.options = options || { @@ inputFilepath: REPO_DIR, + scanPath: SCAN_PATH, runtimeContainer: RUNTIME_CONTAINER, @@ - return ['run','-v',`${this.options.inputFilepath}:/scanoss`, - this.options.runtimeContainer, 'scan', SCAN_PATH, '--output', `./${OUTPUT_FILEPATH}`, + return ['run','-v',`${this.options.inputFilepath}:/scanoss`, + this.options.runtimeContainer, 'scan', this.options.scanPath, '--output', `./${OUTPUT_FILEPATH}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codescantask/services/scan.service.ts` around lines 299 - 301, The command builder uses the module-global SCAN_PATH instead of the instance configuration; update the code in ScanService that constructs the docker args (the function returning ['run','-v',... this.options.runtimeContainer, 'scan', SCAN_PATH, ...]) to use this.options.scanPath (or add scanPath to the Options type if missing) so the service reads the scan path from its constructor options rather than the global SCAN_PATH; ensure Options interface/type includes scanPath and adjust any callers/tests to pass the new option and keep buildDependenciesArgs() usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@codescantask/services/scan.service.ts`:
- Around line 299-301: The command builder uses the module-global SCAN_PATH
instead of the instance configuration; update the code in ScanService that
constructs the docker args (the function returning ['run','-v',...
this.options.runtimeContainer, 'scan', SCAN_PATH, ...]) to use
this.options.scanPath (or add scanPath to the Options type if missing) so the
service reads the scan path from its constructor options rather than the global
SCAN_PATH; ensure Options interface/type includes scanPath and adjust any
callers/tests to pass the new option and keep buildDependenciesArgs() usage
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d69b0247-f445-4c48-94ec-265df7a1199b
⛔ Files ignored due to path filters (1)
codescantask/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
CHANGELOG.mdOVERVIEW.mdcodescantask/app.input.tscodescantask/package.jsoncodescantask/services/scan.service.tscodescantask/task.jsonvss-extension-dev.jsonvss-extension.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
codescantask/utils/path.utils.ts (1)
36-50:⚠️ Potential issue | 🟠 Major
scanPathvalidation can be bypassed after normalization.
On Line 47, checkingnormalizedPath.includes('..')afterpath.normalizeallows inputs likesrc/../pkgto pass, despite the contract forbidding any..segment. Also, absolute checks should run on normalized slash form, not only raw input.🛠️ Proposed fix
export function validateScanPath(scanPath: string | undefined): string { - if (!scanPath) { + const raw = scanPath?.trim(); + if (!raw) { return '.'; } - // Normalize and convert to forward slashes for consistency - const normalizedPath = path.normalize(scanPath).replace(/\\/g, '/'); + // Normalize separators first, then normalize as posix for stable checks + const slashPath = raw.replace(/\\/g, '/'); + const normalizedPath = path.posix.normalize(slashPath); // Reject absolute paths (Unix-style and Windows-style) - const windowsAbsolutePattern = /^[a-zA-Z]:/; - if (path.isAbsolute(scanPath) || windowsAbsolutePattern.test(normalizedPath)) { + const windowsAbsolutePattern = /^[a-zA-Z]:\//; + if ( + path.posix.isAbsolute(normalizedPath) || + windowsAbsolutePattern.test(slashPath) || + slashPath.startsWith('//') + ) { console.warn(`Absolute scan paths not allowed: ${scanPath}. Using default: .`); return '.'; } // Reject directory traversal attempts - if (normalizedPath.includes('..')) { + const hasParentSegment = slashPath.split('/').includes('..'); + if (hasParentSegment || normalizedPath === '..' || normalizedPath.startsWith('../')) { console.warn(`Invalid scan path detected: "${scanPath}". Using default: .`); return '.'; }#!/bin/bash # Verifies current behavior against known bypass/edge cases. node - <<'NODE' const path = require('path'); function currentValidate(scanPath) { if (!scanPath) return '.'; const normalizedPath = path.normalize(scanPath).replace(/\\/g, '/'); const windowsAbsolutePattern = /^[a-zA-Z]:/; if (path.isAbsolute(scanPath) || windowsAbsolutePattern.test(normalizedPath)) return '.'; if (normalizedPath.includes('..')) return '.'; const cleaned = normalizedPath.startsWith('./') ? normalizedPath.slice(2) : normalizedPath; return cleaned || '.'; } [ 'src/../pkg', './a/../b', '../outside', '\\etc', '/etc/passwd', 'C:\\Windows\\System32' ].forEach(p => console.log(`${JSON.stringify(p)} => ${currentValidate(p)}`)); NODE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@codescantask/utils/path.utils.ts` around lines 36 - 50, The current validation examines the raw input and then inspects normalizedPath with string.includes, which lets inputs like "src/../pkg" bypass the rule; update the logic to operate on the normalized form for all checks: compute const normalized = path.normalize(scanPath).replace(/\\/g, '/'), then run path.isAbsolute(normalized) and test the windows drive pattern (windowsAbsolutePattern) against normalized; split normalized by '/' and reject if any segment === '..' (instead of using includes); finally strip a leading './' from the normalized result and return '.' for empty/invalid cases. Use the existing symbols normalizedPath (or normalized), windowsAbsolutePattern, path.isAbsolute and the '..' segment check to locate and replace the current checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@codescantask/utils/path.utils.ts`:
- Around line 36-50: The current validation examines the raw input and then
inspects normalizedPath with string.includes, which lets inputs like
"src/../pkg" bypass the rule; update the logic to operate on the normalized form
for all checks: compute const normalized =
path.normalize(scanPath).replace(/\\/g, '/'), then run
path.isAbsolute(normalized) and test the windows drive pattern
(windowsAbsolutePattern) against normalized; split normalized by '/' and reject
if any segment === '..' (instead of using includes); finally strip a leading
'./' from the normalized result and return '.' for empty/invalid cases. Use the
existing symbols normalizedPath (or normalized), windowsAbsolutePattern,
path.isAbsolute and the '..' segment check to locate and replace the current
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60c14f16-15d6-4d50-be16-fd9437375b3d
⛔ Files ignored due to path filters (1)
codescantask/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
CHANGELOG.mdOVERVIEW.mdcodescantask/app.input.tscodescantask/package.jsoncodescantask/services/scan.service.tscodescantask/task.jsoncodescantask/tests/path-utils.test.tscodescantask/utils/path.utils.tsvss-extension-dev.jsonvss-extension.json
🚧 Files skipped from review as they are similar to previous changes (3)
- codescantask/package.json
- vss-extension-dev.json
- codescantask/services/scan.service.ts
agustingroh
force-pushed
the
feat/SP-3755-configurable-scan-path
branch
from
4b7d503 to
fa821b9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
scanPathinput to specify a relative repository path to scan (default "."); path validation added and unit tests included.Changed
Documentation