Repository navigation
fix(quarantine): harden provenance and write path - #59
Conversation
Reject symlink layouts and forged .json sidecars; stage atomic writes with full UUID ids; fail closed before eviction on pressure. Co-authored-by: Cursor <[email protected]>
Dedupe _safe_existing_layout_dir; skip redundant mkdir; short-circuit validate when eviction is deferred to enforce. Co-authored-by: Cursor <[email protected]>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a8949e1e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| quarantine_limits.enforce_quarantine_limits( | ||
| root, | ||
| layout_subdirs=_ALLOWED_QUARANTINE_LAYOUTS, | ||
| ) |
There was a problem hiding this comment.
When quota pressure requires eviction and existing pairs have mtimes later than the current clock (for example after a restore or clock rollback), the newly committed pair is the global oldest entry. enforce_quarantine_limits therefore deletes the new .txt and .json pair, but quarantine() still returns success with its now-nonexistent path; registry fetch can consequently report a usable quarantine ID that cannot be merged. Exclude the pair being written from post-commit eviction (or otherwise verify it survives) before returning success.
Useful? React with 👍 / 👎.
1 participant
Summary
.jsonself-sidecars; require.txtmerge paths.quarantine_failed, local SSOT unchanged).main(independent of SSRF PR fix(http_policy): allowlist dial host for SSRF #58).Test plan
PYTHONPATH=src pytest tests/test_security_regressions.py tests/test_registry_fetch_r4.py tests/test_registry_ssot.py -qregistry_fetchlayout is refused for write/list/merge.jsonenvelope cannot self-certify as sidecarUnapplied review findings
src/scp/scp_utils.pylist_quarantine— forged sidecarquarantine_idcan point outside the quarantine root. Suggested: derive path frommeta_path.stemonly and requirecontent_path.resolve().is_relative_to(pair_dir.resolve()).src/scp/scp_utils.py_new_quarantine_paths+os.replace— concurrent writers can still overwrite after the existence check. Suggested: exclusive create (O_CREAT|O_EXCL) with retry onEEXIST.src/scp/quarantine_limits.pyprepare_quarantine_write— legacy path still evicts before durable commit. Suggested: thin wrapper tovalidate_quarantine_writeonly, or deprecate.src/scp/registry_ssot.py— merge trusts dual reason-strings, not a fetch-binding token. Optional follow-up if quarantine dir is not operator-trusted FS.