Repository navigation
fix(009): close Phase 14 convergence gaps — symlink rejection, vacuous e2e assertion, contract deviation - #290
Merged
Merged
Conversation
TestAPI_ModArchiveConfinement_SymlinkEscapeRejected checked the mods listing for `symlink-escape.zip`, but the upload API strips the archive extension -- the same file asserts exactly that in TestAPI_ModArchiveConfinement_ValidArchiveExtracts. The comparison could never match, so the test passed whether or not the symlink entry was rejected, leaving US1/AC1 unverified for the symlink case. Mirror the two-form check the sibling traversal test already uses, and name both spellings in the failure message. Feature 009, task T077. Same class of defect as T067. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpaDsTZWHcS4BDY3PvbKKi Signed-off-by: valgulnecron <39313199+ValgulNecron@users.noreply.github.com>
spec.md's "Zip Slip Extraction of Symlinks" edge case requires unzipInto to reject symlinks or confine their resolved targets. It did neither: it never inspected the entry mode, so a symlink-flagged member was written through os.OpenFile as a regular file holding the link text. Safe in effect -- no symlink is created, nothing is followed -- but incidental, undocumented, and pinned by no test. Reject such entries outright instead. A symlink's target is resolved at use time, so a path check at extraction time cannot confine it; there is nothing to gain by materialising one. The check sits after the ConfineRelPath gate and before the IsDir branch, and returns the new errSymlinkEntry sentinel wrapped in the loop's existing "zip-slip:" form. TestUnzipInto_RejectsSymlinks builds a zip whose entry carries os.ModeSymlink, asserts errors.Is against the sentinel, and Lstats the destination to prove nothing was written. Feature 009, task T078. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpaDsTZWHcS4BDY3PvbKKi Signed-off-by: valgulnecron <39313199+ValgulNecron@users.noreply.github.com>
The path-confinement contract's Migration Pattern rule 4 says the old inline Join+Clean+HasPrefix guards MUST be deleted, not duplicated. Four call sites in agent/internal/mods/mods.go keep them after calling the helper, each commented "Re-check inline so the guard is visible at the point of use". Resolve the divergence toward the code rather than the rule. Deleting the re-checks is the riskier half: they are plausibly what made the guards legible to CodeQL and cleared alerts #7-#13, and they buy no security either way since each only re-asserts a postcondition the helper already guarantees. Migration Pattern rule 3 is still honoured in full -- the helper's return value is the only path ever passed to a file operation. Rule 4's text is left verbatim; the contracts are an append-only record, so the deviation goes in a new section below it. Also appends Phase 14 (T077-T079) from the fourth /speckit-converge run and marks it complete. T055/T056 stay open, blocked upstream. Feature 009, task T079. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpaDsTZWHcS4BDY3PvbKKi Signed-off-by: valgulnecron <39313199+ValgulNecron@users.noreply.github.com>
The symlink guard tested only f.Mode()&os.ModeSymlink. archive/zip decodes the Unix type bits into FileMode only when CreatorVersion declares a Unix creator; with the creator byte left at zero, FileHeader.Mode falls through to msdosModeToFileMode and returns 0666, so the guard never fired. The e2e fixture in api_mods_confinement_e2e_test.go builds exactly that shape -- ExternalAttrs set to 0o120777<<16, CreatorVersion untouched -- and caught this: the archive extracted and landed in the mods listing on both arches, while the unit test passed because SetMode stamps the creator byte. The archive is untrusted input, so the fixture is the adversarial case, not a malformed test. Leave it as written and read the raw type bits from ExternalAttrs alongside Mode(). A real MS-DOS-created zip keeps its FAT attributes in the low 16 bits, so this adds no false positives. TestUnzipInto_RejectsSymlinksWithoutUnixCreator covers the fallback path and asserts the premise, so it fails loudly rather than silently stops covering anything if a future Go release changes Mode()'s behaviour. Feature 009, task T078. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XpaDsTZWHcS4BDY3PvbKKi Signed-off-by: valgulnecron <39313199+ValgulNecron@users.noreply.github.com>
CI Report - 1a37ab8run 33310101144 - attempt 1 0 failed - 12 passed - 4 skipped - 20m34s elapsed Coverageno coverage/* commit statuses found on 1a37ab8 Changed
Slowest jobs
e2e buckets run
|
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
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.
Closes the three gaps found by the fourth
/speckit-convergerun against feature009-remediate-security-dependabot(master at 7fccf00). Everything else in that feature re-verified clean: 0 open CodeQL alerts, 0 open Dependabot security alerts, all FR-008–FR-020 version targets met across all 14go.modfiles andweb/package.json, no in-source suppression anywhere, all 111 e2e tests bucketed, no coverage threshold lowered.T077 (HIGH) —
test/e2e/api_mods_confinement_e2e_test.goTestAPI_ModArchiveConfinement_SymlinkEscapeRejectedchecked the mods listing forsymlink-escape.zip, but the upload API strips the archive extension — the same file asserts exactly that inTestAPI_ModArchiveConfinement_ValidArchiveExtracts:382-383. The comparison could never match, so the test passed whether or not the symlink entry was rejected. Now mirrors the two-form check the sibling traversal test already uses at:167. Same class of defect as T067.T078 (MEDIUM) —
agent/internal/mods/mods.gounzipIntonever inspected the entry mode, so a symlink-flagged member was written throughos.OpenFileas a regular file containing the link text. Safe in effect, but incidental and untested — spec.md's "Zip Slip Extraction of Symlinks" edge case asks for it to be explicit. Symlink entries are now rejected outright: a symlink's target resolves at use time, so an extraction-time path check cannot confine it. NewerrSymlinkEntrysentinel, wrapped in the loop's existingzip-slip: %wform.TestUnzipInto_RejectsSymlinkscovers the branch (the agent module's 90% gate has a thin margin).T079 (MEDIUM) —
contracts/path-confinement.mdMigration Pattern rule 4 says the inline
Join+Clean+HasPrefixguards MUST be deleted, not duplicated; four call sites keep them after calling the helper. Resolved toward the code, not the rule — deleting them is the riskier half, since they are plausibly what made the guards legible to CodeQL and cleared alerts #7–#13, and they buy nothing either way (each only re-asserts a postcondition the helper already guarantees). Rule 3 is still honoured in full. Rule 4's text is left verbatim and the deviation is recorded in a new section below it, since the contracts are an append-only record.Verification
Per the repo's CI-only rule, nothing was built, tested or linted locally. The relevant gates are
lint (agent),go (agent),go-e2e-unitande2e api-mods.Note for T079: if a subsequent CodeQL run on master shows the inline re-checks are not what keeps alerts #7–#13 clear, the deviation section names that as the trigger to revisit and delete them.
🤖 Generated with Claude Code
https://claude.ai/code/session_01XpaDsTZWHcS4BDY3PvbKKi