NEW @W-23766007@ Feature/h1 java command relative path fix - #496
Conversation
…ative java_command paths Adds a shared helper that accepts only bare PATH command names or absolute paths for java_command, rejecting repo-relative paths that could resolve to a repository-controlled executable. Bumps engine-api to 0.42.0-SNAPSHOT and cascades the dependency bump to all consuming packages. Part of the H1 bug bounty RCE fix.
… and cpd extractJavaCommand now validates the user-specified java_command via the shared ValueValidator.validateJavaCommand helper BEFORE calling identifyJavaVersion, so a repo-relative executable path in an auto-discovered config is never spawned. Bare command names and absolute paths continue to work unchanged. Part of the H1 bug bounty RCE fix.
SfgeConfigValueExtractor.extractJavaCommand now validates the user-specified java_command via the shared ValueValidator.validateJavaCommand helper BEFORE calling identifyJavaVersion, so a repo-relative executable path in an auto-discovered config is never spawned. Bare command names and absolute paths continue to work unchanged. Part of the H1 bug bounty RCE fix.
…e java_command Reproduces the H1 PoC using core stubs: an auto-discovered config sets a relative engines.pmd.java_command, and a JavaVersionIdentifier spy fails the test if the binary is ever spawned. Asserts the malicious PMD engine fails to instantiate without spawning, addEnginePlugin does not throw, and an eslint-only selectRules still succeeds (per-engine failure isolation). Part of the H1 bug bounty RCE fix.
Code review found 3 issues with the java_command security fix: - Added clarifying comment about absolute-path limitation: auto-discovered configs can still set absolute paths to repo-controlled binaries. This fix mitigates relative paths but doesn't fully close the threat model. - Fixed Windows drive-relative path bypass (e.g., 'C:foo') by checking for ':' in bare command names, preventing drive-relative path resolution. - Removed redundant path.sep check and replaced with ':' validation for cross-platform correctness. - Fixed missing newline at end of pmd-engine test file. Reviewed-by: Claude (code-review agent) Fixed-by: Claude (Anthropic)
|
Git2Gus App is installed but the |
aruntyagiTutu
left a comment
aruntyagiTutu
left a comment
There was a problem hiding this comment.
Reviewed against the Code Analyzer PR Review guidelines.
Summary: Fixes a relative-java_command PATH-hijack risk (CWE-427-adjacent, similar in spirit to the cwd-shadowing fix in #495) by rejecting relative paths in ValueValidator.validateJavaCommand before spawning java for PMD/CPD and SFGE. Validation runs before validateJavaCommandContainsValidVersion (which spawns java) in both engines' config extraction — no unguarded spawn path.
Verified the boundary is correct: bare PATH-resolved names (java) and absolute paths are allowed; ./java, ../java, and other relative paths are rejected — matches the intended threat model without breaking normal PATH-based java resolution.
Confirmed the follow-up "address code review findings" commit (9006b01) fixed a real cross-platform gap: the original check used !strValue.includes(path.sep), which never actually caught Windows drive-relative paths like C:foo (path.isAbsolute('C:foo') is false, and path.sep checks are redundant with the separator checks already in place). The fix added an explicit : check, closing that bypass. Good catch by whoever flagged it, and it's fully resolved now.
Test coverage is solid — java-command-security.test.ts uses a spy that fails the test if the java-version-check function is ever invoked, proving rejection happens pre-spawn, not just at a validation layer that could be bypassed later. CI is green across macOS/Ubuntu/Windows.
Two non-blocking items worth a follow-up (not blockers here):
- The validator's own docstring notes that absolute-but-repo-controlled paths, and auto-discovered configs overriding
java_commandfor unselected engines, are still unmitigated — worth a GUS ticket per the PR hygiene norm of tracking known-remaining scope rather than letting it go untracked. validateJavaCommand('')returns''as a "valid" bare command (no separators to trip the check) — not currently reachable since call sites guard on falsyjavaCommandfirst, but worth tightening the validator itself since callers could change.
Approving the direction; these are cleanup items, not blockers.
|
created this ticket : https://gus.lightning.force.com/lightning/r/ADM_Work__c/a07EE00002hEvriYAC/view |
…d-relative-path-fix
3 participants
No description provided.