fix(backend): review-pass hardening (SSRF, delete guard, git timeout, RAII, import, dedupe)#88
Merged
Merged
Conversation
Addresses the two in-scope security minors from the PR #88 static review: - Refuse https->http redirects. fetch_public_url now records whether the original request was https and rejects any redirect that downgrades the transport, so a redirect can't strip TLS. - Handle NAT64 addresses in the IPv6 private check. The well-known prefix 64:ff9b::/96 embeds a target IPv4 in its low 32 bits, so 64:ff9b::7f00:1 would reach 127.0.0.1. Extract the embedded IPv4 and apply the IPv4 blocklist (only the embedded address decides, so public NAT64 targets like 64:ff9b::808:808 still resolve). The local-use prefix 64:ff9b:1::/48 (RFC 8215) is site-internal by definition and is blocked wholesale. Tests: NAT64 embedded-loopback + local-use added to the private-literal set, NAT64 embedded-public added to the allowed set. cargo fmt/clippy/test green (726 lib tests).
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.
What
A backend hardening + cleanup pass from the senior-engineer code review of
src-tauri/. One commit per finding, all on this PR:fix(tools): harden web_fetch against private targets— parse withreqwest::Url, disable auto-redirects and follow manually, revalidate every hop's host against the private/loopback/link-local/reserved/CGNAT blocklist (IPv4 + IPv6), pin the validated address into reqwest (resolve_to_addrs) to defeat DNS rebinding, and stream the body into a bounded buffer before HTML→markdown so a hostile public server can't OOM the process.fix(workspace): block deletion while runs are active—workspace_has_active_run(queued/running/waiting_for_tool across all sessions) now gatesworkspace_deletebefore index removal +remove_dir_all, mirroring the existing session-delete guard.fix(skills): bound git source sync commands— every skill-sourcegitinvocation runs non-interactive (GIT_TERMINAL_PROMPT=0,GIT_ASKPASS=true, forwarded throughflatpak-spawn --env) under a 120s timeout with drained stdout/stderr, converting network/credential hangs into fast failures.refactor(runtime): unregister active runs on guard drop—register_runreturns aRunRegistrationRAII guard (generation-tagged so a duplicate registration can't evict a newer token); drop unregisters on both normal exit and panic. Removed the manualunregister_runfrom all three spawn sites and the now-dead public fn.fix(import): avoid overwriting file collisions— replacedexists()+copy()with an exclusive-create (O_EXCL) copy loop that cleans up partial files, closing a TOCTOU.refactor(compaction): share context-limit recovery helper— extractedcompact_for_context_limit_recovery, the one shared op between the HTTP and CLI recovery paths (behavior identical, CLI keeps its session-reset hook locally).fix(tools): close web_fetch scheme-downgrade + NAT64 SSRF gaps— from the review of this stack: refuse https→http redirect downgrades, and treat NAT64 (64:ff9b::/96well-known,64:ff9b:1::/48local-use) by extracting the embedded IPv4 and applying the IPv4 blocklist.Review
Independent static review of the full
main..HEADrange (Code Reviewer Minimax):production_quality— no blockers, no majors. Commit 7 above folds in the two in-scope security minors it surfaced. Remaining minors are documented follow-ups, deliberately out of scope: an end-to-end redirect-loop test (needs a mock-server dev-dep), a reviewer-acknowledged-benign queued-run TOCTOU inworkspace_delete(the comment already calls the guard "coarse"; the RAII guard closes the in-flight case), and two cosmetic cleanups (redundantinput.rewind(),#[cfg(test)]unique_destination).Verification
cargo fmt --check·cargo clippy --lib --no-deps -- -D warnings·cargo test --lib(726) ·cargo test --test cancel_run— all green on Linux. New unit/integration tests: workspace-delete active-run guard, git env/timeout arg composition (native + flatpak), RAII register→cancel→drop + double-register + stale-drop, import collision + O_EXCL, NAT64 private/public classification.