fix(security): harden local analysis boundaries - #35
Conversation
Make cloc entrypoint detection path-safe, reject symbolic-link source patches, fail closed on incomplete secret scans, and extend session credential redaction. Spec: docs/specs/2026-07-31-security-reliability-boundaries.md Test: npm test Test: npm run pack:verify Test: node --test test/doc-link-graph.test.mjs
phodal
left a comment
phodal
left a comment
There was a problem hiding this comment.
Requesting changes on head 3f6a1e9. The overall direction and traceability spec are sound, but the current implementation still leaves three security/privacy boundary failures and one silent CLI reliability failure. Each issue below was reproduced against this exact commit even though the focused tests and CI pass.
Please address all four threads and add regression coverage for each scenario. Also amend the commit with the repository-required single Co-authored-by line using the actual AI agent/model, then rerun npm test, npm run pack:verify, the docs build, and the final changed-file secret scan.
|
|
||
| if (stat.isSymbolicLink()) { | ||
| stats.skippedFiles += 1; | ||
| stats.errors.push(`${absPath}: symbolic-link scan targets are not inspected`); |
There was a problem hiding this comment.
[P1] Record nested symbolic links as incomplete coverage
This branch is reached only when collectFiles() is called directly with a symbolic-link path. During directory traversal, the Dirent loop handles only directories and regular files, so nested symbolic links are silently ignored. A directory containing only a symlink currently reports coverageStatus: \"complete\", scannedFiles: 0, no errors, and exits 0.
Please record nested links as skipped/incomplete (and do the same for safety-related read skips), then add a regression that scans a directory containing a symbolic link rather than passing the link as the explicit CLI path.
| const relativeBackup = backupName(sourceRef, options.now); | ||
| const backupPath = path.join(backupRoot, relativeBackup); | ||
| await mkdir(path.dirname(backupPath), { recursive: true }); | ||
| await assertSafeSourceTarget(resolved); |
There was a problem hiding this comment.
[P1] Validate the backup write path as well as the source path
assertSafeSourceTarget(resolved) protects the source file, but backupPath is still created and written without canonical containment or component-level symlink checks. If .better-harness-checkup-backups is a symlink to a directory outside the workspace, this patch succeeds and copyFile() writes the backup outside the authorized root.
Please create/validate the backup tree without following symbolic-link components, verify its real path remains under the canonical workspace, and add a regression for a symlinked backup root.
| .replace(/\b(api[_-]?key|access[_-]?token|auth[_-]?token|password|secret)\s*[:=]\s*[^\s,;]+/giu, "$1=<redacted>") | ||
| .replace(/\b(?:sk|ghp|github_pat|xox[abprs])[-_][A-Za-z0-9_-]{8,}\b/giu, "<secret>") | ||
| .replace(/\bglpat-[A-Za-z0-9_-]{20,}\b/giu, "<secret>") | ||
| .replace(/\b((?:https?|ftp):\/\/)[^\s/@:]+:[^\s/@]+@/giu, "$1<redacted>@") |
There was a problem hiding this comment.
[P1] Redact userinfo credentials for non-HTTP URL schemes
This pattern only covers HTTP(S)/FTP URLs with a user:password@ form. Common credential-bearing values such as postgresql://db-user:db-password@... and ssh://build-user:ssh-password@... keep both credentials in the review-safe result; later path redaction removes only the host/path.
Please redact userinfo for all supported hierarchical URL schemes (including username/token-only forms where applicable) and add cases for database URLs, SSH URLs, and percent-encoded userinfo.
| } | ||
|
|
||
| if (process.argv[1] && import.meta.url.endsWith(process.argv[1].replaceAll("\\", "/"))) { | ||
| if (process.argv[1] && path.resolve(process.argv[1]) === path.resolve(fileURLToPath(import.meta.url))) { |
There was a problem hiding this comment.
[P2] Canonicalize both entrypoint paths before comparing them
path.resolve() is lexical and does not resolve symbolic links. On macOS, invoking this exact file through the /tmp alias while the module resolves under /private/tmp still produces no output and exits 0. The current spaced-path test places only the analyzed --cwd under a spaced path; the CLI file itself remains at the normal repository path.
Please compare canonical real paths and add a regression where the CLI installation path itself contains spaces and is invoked through an equivalent symlinked path.
Summary
clocdirect-entrypoint check filesystem-safe for installations in paths containing spacescomplete,partial, orfailed, and return exit code3for incomplete scans while preserving2for findingsMotivation and context
These fixes close confirmed reliability and local security boundary failures:
scripts/cloc/cli.mjscompared an encoded module URL with an unencoded filesystem path, so a valid install path containing spaces exited successfully without running the CLI.No host support, scoring, report-model, Preview, or release automation behavior is changed.
Test plan
npm test— 1028/1028 passednpm run pack:verify— npm 359 entries, runtime zip 382 entriesnode --test test/doc-link-graph.test.mjs— 6/6 passedcd docs && npm run build— English and Simplified Chinese builds passedCompatibility and downstream impact
3when scanning is incomplete. Exit code0remains complete-and-clean; exit code2remains findings at or above the configured threshold.Risk and rollback
AI assistance
AI-assisted implementation and review were used. All changes were validated with focused RED/GREEN regression tests, the full test suite, package verification, documentation builds, and a final changed-file secret scan.
Checklist
AGENTS.mdand the relevant architecture/specification guidance.CHANGELOG.mdfor user-visible behavior.