Skip to content

fix(security): The approval snapshot and the integrity hash must apply one shared exclusion predicate so unreviewed files cannot be frozen. - #411

Open
github-actions[bot] wants to merge 4 commits into
mainfrom
fix/security-20261024-080239-scheduled-ok-plan-03
Open

github-actions[bot] wants to merge 4 commits into
mainfrom
fix/security-20261024-080239-scheduled-ok-plan-03

Conversation

@github-actions

Copy link
Copy Markdown

Issue: Refs #321

What

  • The approval snapshot and the integrity hash now apply one shared exclusion predicate, so no file can be frozen into a snapshot without also being covered by the approved hash.
  • Snapshot file copies are opened with O_NOFOLLOW; a path that turns into a symlink between walk and copy is dropped instead of followed.

Why

  • The snapshot copy previously decided on its own which files to skip. A file the approved hash ignores could therefore end up in the snapshot that actually runs, giving it trusted status it never received. See Security hardening #321.

How

  • IsExcludedFileName is exported from internal/config as the single definition of the skip list (internal/config/meta.go).
  • copyTree in internal/skilltrust/snapshot.go consults that predicate, and copyFile opens with syscall.O_NOFOLLOW, dropping the file on ELOOP rather than retrying the same race.
  • A regression test (internal/skilltrust/snapshot_exclusion_parity_security_test.go) asserts the frozen set equals the hashed set.

Verification

  • go build ./... — pass
  • go vet ./... — pass
  • gofmt -l . — clean
  • staticcheck ./internal/config/... ./internal/skilltrust/... — pass
  • Security regression tests (go test ./internal/skilltrust/) — pass

Follow-up

  • None.

Test Writer Agent and others added 2 commits October 24, 2026 09:51
Signed-off-by: Test Writer Agent <test-writer@security-remediate.invalid>
…y one shared exclusion predicate so unreviewed files cannot be frozen.

Signed-off-by: Fix Writer Agent <fix-writer@security-remediate.invalid>
@github-actions github-actions Bot added the do-not-merge Hold: automated fix PR, needs a human pass label Oct 24, 2026
@NoRiceToday

Copy link
Copy Markdown
Contributor

[AI] Independent R1 on 5e6e3c5a53c553c130774bf1bb02d26e41998853: P1, changes needed.

Legacy snapshots bypass the new filter (internal/skilltrust/snapshot.go:202-208): existing directories return unchanged at :91-94, and lookup/spawn still accepts them. Reproducer: create an approval with the exact base's copier, upgrade to this head, remove ignored module.pyc from the source, then re-approve the same hash. The old snapshot still executes that bytecode; both hashes match and both spawn gates accept it. Version/invalidate legacy snapshots at lookup/spawn and rebuild safely on host re-registration. Rehashing alone cannot detect excluded bytes.

Keep #411 over duplicate #349: equivalent exclusions, plus working terminal-symlink protection. Retain #349's Thumbs.db and behavioral file-set checks; fix this upgrade case before fresh independent review.

Go 1.26 targeted/race tests, focused CLI/supervisor checks, build/vet, and copy/symlink controls passed. One cold-start /tmp failure also reproduces on the untouched base. Linux was cross-compiled, not executed. CI still requires workflow approval. Reproducers/reports are retained under campaign #454; not merge-ready.

@NoRiceToday

Copy link
Copy Markdown
Contributor

[AI] Independent R2, LOCAL PATCH / NOT PUSHED: F1 resolved; no actionable P0–P3 findings in the cumulative original PR plus repair. Spec and quality verdicts: PASS for candidate tree 902e6f78c405cb8cb1a5013f18e28e8aa1740d33. Repair SHA-256: 42137c14036d620135e2da1356d1d25d121e2a34e5d3d3e1aa9e079a5df70f74.

Fresh real-old-Approve control executes ignored bytecode on the original head; the candidate refuses it at lookup/preflight/backstop, and same-hash rebuilding strips it. Legacy skill named v2, both supervisor refusal entry points, current-snapshot execution/revocation/idempotence, focused race tests and Staticcheck v0.8.2 pass; no green-run skips. Useful #349 coverage is consolidated without importing both suites.

Not merge approval: integrate/review with #410/#412 (including migration/docs), then human push, exact-head native Linux/macOS CI and human approval. Remote head 5e6e3c5a53c553c130774bf1bb02d26e41998853 is unchanged; CI remains action_required. Full local report: .review-tracking/pr-411-review-r2.md.

Legacy snapshots may contain unhashed files. Require host re-registration to rebuild them under the current copy policy.

Refs #321

🤖 Generated with OpenCode

Signed-off-by: Sajjad Ahmad <sajjad.ahmad@tngtech.com>
Preserve #411's legacy-snapshot refusal alongside current-main guidance.

Refs #321

🤖 Generated with OpenCode

Signed-off-by: Sajjad Ahmad <sajjad.ahmad@tngtech.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge Hold: automated fix PR, needs a human pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant