Harden command registration path handling#8
Closed
mnriem wants to merge 1 commit into
Closed
Conversation
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>
Owner
Author
|
Reopening against upstream github/spec-kit. |
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.
What this fixes
CommandRegistrar.register_commands()read each command body fromsource_dir / cmd_fileusing the manifest'sfilefield without validating it. Unlike the parallel skill and preset readers — which already reject absolute paths and..traversal and require the resolved path to stay within the extension directory — the shared command path applied no such check.As a result, a manifest with
file: ../../../etc/passwd(or a bare absolute path) could cause an arbitrary host file to be read verbatim into a generated agent command file at a predictable path under the project (e.g..claude/commands/..., mirrored for each detected agent).Changes
agents.py—register_commands(): add the same containment guard used by the skill/preset readers before reading the source file — reject absolute paths and ensure the resolved path stays withinsource_dir; skip the command otherwise.extensions.py—ExtensionManifest._validate(): reject an absolute or..-containingfilevalue at manifest-load time with a clearValidationError(defense-in-depth, surfaces a real error instead of a silent skip).test_registrar_path_traversal.py: newTestCommandFileTraversalcovering thefileread path (the existing tests only covered command name/alias output traversal). Asserts traversal/absolutefilevalues register nothing and never leak file contents into generated commands.test_extensions.py: parametrized manifest-load validation test for thefilefield.Verification
New and targeted suites pass. Full suite: 3975 passed, with 2 pre-existing failures in
test_init_dir.py(aSPECIFY_INIT_DIRcore-scripts environment issue) confirmed unrelated to these changes by stashing.