mirror of
https://github.com/github/spec-kit.git
synced 2026-07-03 12:28:06 +08:00
main
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Manfred Riem
|
902f5431f9 |
Harden command registration path handling (#3088)
* fix: validate command 'file' field against path traversal in registrar CommandRegistrar.register_commands() read each command body from source_dir / cmd_file without validating the manifest 'file' field, unlike the parallel skill and preset readers which already reject absolute paths and '..' traversal. A malicious extension/preset/bundle manifest with file: ../../../etc/passwd (or an absolute path) could read arbitrary host files verbatim into a generated agent command at a predictable path (GHSA-w5fv-7w9x-7fc5, CWE-22). Add the same containment guard at the command read site and reject a traversal/absolute 'file' at manifest-load time in ExtensionManifest._validate() for defense-in-depth, plus regression tests for both the read path and the manifest validator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test/fix: address review — robust absolute-path test and tolerant reads - register_commands(): use is_file() instead of exists() and skip the command if read_text() raises (directory or non-UTF8 file), aligning with the other command/skill readers. - Traversal tests: point the absolute-path payload at the real temp secret.txt (guaranteed to exist on all platforms) instead of /etc/passwd, so the absolute-path guard is genuinely exercised and the test fails if it regresses, rather than passing because the target happens not to exist (e.g. on Windows runners). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: rename traversal fixtures to avoid CodeQL secret-storage false positive The regression fixtures named an out-of-tree file secret.txt with TOP-SECRET-CREDENTIAL content. CodeQL's clear-text-storage heuristic treated that read content as sensitive and followed the static path into the pre-existing write_text sinks in _write_registered_output, raising false 'clear-text storage of sensitive information' alerts on PR 3088. Rename the fixtures to neutral outside.txt / OUTSIDE-FILE-MARKER and drop /etc/passwd payloads; the test semantics (a file outside source_dir must never be read into a generated command) are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reject Windows drive-relative 'file' values in traversal guards is_absolute() is False for Windows drive-relative paths like C:outside.txt, which contain no '..' yet resolve against the process CWD on that drive — bypassing the containment guard on Windows. Evaluate the 'file' value under PureWindowsPath as well so both the registrar runtime guard and the manifest-load validator reject drive letters (and backslash '..' segments) cross-platform. Extend the regression tests with drive-relative cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use anchor under both path flavors so POSIX-absolute is rejected on Windows On a Windows runner WindowsPath('/abs/outside.md').is_absolute() is False (no drive), so the prior native-Path check let a leading-slash 'file' value through and the manifest validator did not raise. Evaluate the value under both PurePosixPath and PureWindowsPath and reject any non-empty anchor — covering POSIX-absolute, Windows drive-relative, Windows absolute, and rooted-without-drive — in both the registrar guard and the manifest validator. The registrar join now uses the raw 'file' string so native separators are handled by the resolve()/relative_to() containment check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: validate command 'file' field against path traversal in registrar CommandRegistrar.register_commands() read each command body from source_dir / cmd_file without validating the manifest 'file' field, unlike the parallel skill and preset readers which already reject absolute paths and '..' traversal. A malicious extension/preset/bundle manifest with file: ../../../etc/passwd (or an absolute path) could read arbitrary host files verbatim into a generated agent command at a predictable path (GHSA-w5fv-7w9x-7fc5, CWE-22). Add the same containment guard at the command read site and reject a traversal/absolute 'file' at manifest-load time in ExtensionManifest._validate() for defense-in-depth, plus regression tests for both the read path and the manifest validator. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test/fix: address review — robust absolute-path test and tolerant reads - register_commands(): use is_file() instead of exists() and skip the command if read_text() raises (directory or non-UTF8 file), aligning with the other command/skill readers. - Traversal tests: point the absolute-path payload at the real temp secret.txt (guaranteed to exist on all platforms) instead of /etc/passwd, so the absolute-path guard is genuinely exercised and the test fails if it regresses, rather than passing because the target happens not to exist (e.g. on Windows runners). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: rename traversal fixtures to avoid CodeQL secret-storage false positive The regression fixtures named an out-of-tree file secret.txt with TOP-SECRET-CREDENTIAL content. CodeQL's clear-text-storage heuristic treated that read content as sensitive and followed the static path into the pre-existing write_text sinks in _write_registered_output, raising false 'clear-text storage of sensitive information' alerts on PR 3088. Rename the fixtures to neutral outside.txt / OUTSIDE-FILE-MARKER and drop /etc/passwd payloads; the test semantics (a file outside source_dir must never be read into a generated command) are unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: reject Windows drive-relative 'file' values in traversal guards is_absolute() is False for Windows drive-relative paths like C:outside.txt, which contain no '..' yet resolve against the process CWD on that drive — bypassing the containment guard on Windows. Evaluate the 'file' value under PureWindowsPath as well so both the registrar runtime guard and the manifest-load validator reject drive letters (and backslash '..' segments) cross-platform. Extend the regression tests with drive-relative cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: use anchor under both path flavors so POSIX-absolute is rejected on Windows On a Windows runner WindowsPath('/abs/outside.md').is_absolute() is False (no drive), so the prior native-Path check let a leading-slash 'file' value through and the manifest validator did not raise. Evaluate the value under both PurePosixPath and PureWindowsPath and reject any non-empty anchor — covering POSIX-absolute, Windows drive-relative, Windows absolute, and rooted-without-drive — in both the registrar guard and the manifest validator. The registrar join now uses the raw 'file' string so native separators are handled by the resolve()/relative_to() containment check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: harden register_commands inputs and tighten manifest 'file' validation Address review feedback on #3088: - register_commands(): skip non-string/empty 'file' values instead of raising TypeError, and hoist source_dir.resolve() out of the per-command loop. - ExtensionManifest._validate(): reject 'file' values with leading/trailing whitespace with a clear ValidationError instead of a confusing missing-file failure later. - tests: add non-string 'file' and whitespace cases; use yaml.safe_dump with explicit utf-8 encoding in the manifest validation test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor: align runtime '..' policy, correct comment, dedupe test helper Address review feedback on #3088: - register_commands(): also reject '..' segments under both POSIX and Windows semantics, keeping runtime policy consistent with ExtensionManifest._validate() and the skill/preset readers (not just relying on the resolve()/relative_to() containment backstop). - Replace the version-dependent is_absolute() claim in the extensions.py comment with the actual portability rationale (native Path is OS- dependent; C:foo is anchored but not absolute). - Extract the duplicated leak-detection assertion into _assert_no_marker_leak() and add an in-bounds '..' payload that exercises the new runtime '..' rejection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Extract shared path-safety policy and warn on unreadable command files Introduce relative_extension_path_violation() in _utils.py as the single source of truth for the extension-relative `file` path-safety policy, and use it from both the runtime registrar guard (agents.py) and the manifest-load validator (extensions.py) so the two cannot drift. Warn (instead of silently skipping) when an in-bounds command file exists but cannot be read/decoded, surfacing misconfigured extensions. Add unit tests for the shared helper, a read-skip warning test, and make the in-bounds `..` test create its target file so the skip is attributable to the `..` rejection rather than file absence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Retrigger CI Empty commit to re-trigger code scanning / CodeQL analysis on the PR merge ref. Assisted-by: GitHub Copilot CLI (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
김준호
|
569d18a59d |
fix(agents): block directory traversal in command write paths (#2229) (#2296)
Extend the alias containment guard from
|