fix(sandbox-lint): normalise every trailing slash, not just one - #1149
Conversation
_normalise() stripped a single trailing slash so a config path and its
slash variant compare equal against FORBIDDEN_ALLOW_READ /
FORBIDDEN_ALLOW_WRITE. A path with two or more trailing slashes (e.g.
"~/.ssh//") only lost one, so it never matched the forbidden entry and
check_invariants() reported no violation for a config that grants read
or write access into a credential directory.
Strip all trailing slashes instead, keeping the bare-root case ("/")
intact. Added a double-slash case to the existing parametrized
allowRead/allowWrite tests; both fail on main and pass with this
change.
Generated-by: Claude Code (Sonnet 5)
ef09a63 to
28dd733
Compare
potiuk
left a comment
There was a problem hiding this comment.
Correct fix, and the bypass it closes is real: ~/.ssh// survived the strip-one logic and so slipped the credential-path forbidden list entirely.
path.rstrip("/") or "/" is the right form — the or "/" guard matters, since a bare / would otherwise normalise to the empty string and stop matching the root entry.
I checked the two things that could have gone wrong:
- Edge cases —
/→/,//→/,~/→~,/a/b//→/a/b, and""→/. The last one is a behaviour change from the old code (which returned""), but it fails closed: a malformed empty entry now matches the forbidden root rather than silently comparing equal to nothing. - Call sites — all four uses are set-membership tests that normalise both sides (
_normalised_seton the config,_normaliseon the required/forbidden constant). Nothing does prefix matching that depended on a retained trailing slash, so stripping more is safe here.
Test coverage matches the fix: ~/.ssh// on the read side and ~/.aws// on the write side.
🤖 This review was drafted by an AI-assisted tool and may contain mistakes. It has been reviewed and confirmed by an Apache Magpie maintainer before submission. See CONTRIBUTING.md for what this project considers a maintainer review.
|
Thanks for checking the call sites too, that was the part I hadn't fully convinced myself of. Good catch that the empty-string case is a behavior change, glad it fails closed. |
The marker sat at a1cff44, 17 commits behind main. Bumping it alone would claim those commits are described by the specs, so the drift is closed first. Specs updated for what actually shipped: - meta-and-quality-tooling: skill-evals errors, rather than passing, when a case's CLI produced no gradeable output (#1161). - security-reporting: the tracker dashboard projects the current partial bucket to its end-of-bucket value, splitting RATE series (accumulate from zero) from LEVEL series (carry over), and deliberately not projecting mean-based signals (#1158). - project-agnosticism: <PROJECT> and <project> are two placeholders holding different values, and the lint carries both spellings plus spaced variants (#1154). - adapters: the forwarder relay's contact_handle defaults to an org-level shared inbox rather than a named individual (#1135). The multi-hop coordinator case is designed in RFC-AI-0008 and unimplemented. - issue-management-family: the family's eval suites, and the note that --cli runs belong outside a credential-denying sandbox (#1145). Commits needing no spec change: #1152, #1143 and #1156 updated their own specs in-commit; #1149, #1147, #1151 are behaviour-preserving bug fixes; #1155 and #1141 are CI and dependency chores; #1159's spec edits landed with it; #1144 removes hardcoded literals that no spec asserted. One genuine gap recorded rather than papered over: no spec covers marketplace distribution or the dev-version stamping rule from #1160, which is load-bearing because `claude plugin update` compares version strings, not commit SHAs. Logged in adoption-and-setup as wanting its own spec. Generated-by: Claude Code (Opus 5)
Summary
_normalise()only stripped one trailing slash, so~/.ssh//(two slashes) never matched the normalised~/.sshforbidden entry andcheck_invariants()passed a config it should have rejected.Type of change
tools/*/withpyproject.toml)Test plan
allowRead/allowWriteparametrized tests; both are red on main, green on this branch.ruff check,ruff format --checkandmypyontools/sandbox-lintare clean (mypy needspytestinstalled to resolve the test-file imports; not a change this PR introduces).tools/sandbox-lintsuite: 55 passed, in a cleanpython:3.11-slimcontainer. Did not runprekitself (not available in this sandbox), ran the underlying ruff/mypy/pytest commands by hand instead.RFC-AI-0004 compliance
Linked issues
Fixes #1148.