Repository navigation
feat(web-fetch): opt-in fake-IP allowance with DoH screening for local networks - #1146
Conversation
loveRhythm1990
changed the title
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed the current head against the fake-IP issue, transport/SSRF ownership, configuration layering, and the new tests. I found three remaining concerns inline. The resolver URL disclosure from the earlier head is addressed by the new host/index-only diagnostics and credential rejection.
The issue also describes proxy-resolved destinations, while this PR explicitly defers that path: a name that cannot be resolved locally still fails before the proxy is used. Please keep that acceptance gap open or narrow the closing claim. The existing offline tests primarily exercise validation helpers; the fake-IP transport smoke is ignored and requires a real VPN, so they do not establish that the verified DoH answer is the VPN's actual destination.
loveRhythm1990
changed the title
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for two reproduced gaps in the new fake-IP opt-in: a workspace can select the supposedly user-only policy when HOME is absent, and the DoH screen can accept incomplete or contradictory answers. See the inline comments.
The earlier project-layer validation and duplicate first-hop lookups are addressed. The remaining VPN-versus-DoH destination mismatch is now explicitly documented as an opt-in relaxation, rather than a verified private-network boundary. Proxy-only remote DNS remains outside this PR's scope.
ca317e1 to
f8dc1ae
Compare
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed f8dc1aeb6323fc6af0ba122a828613850dcb3511. No remaining PR-introduced blocker found; approving the source changes.
Verified all five earlier review threads against the implementation and tests: the VPN/DoH destination mismatch is explicitly an opt-in boundary relaxation; user-only sections are removed before typed project validation; first-hop resolution is reused; workspace dotenv cannot inject HOME or the state-root overrides; truncated and contradictory DNS answers cannot admit the host.
Implementation review: network screening stays in shared web-fetch, local owners select the policy, and Server remains direct/pinned. CNAME traversal, DNS payloads and query time are bounded, A/AAAA remain parallel, private-address evidence cannot be outvoted by fallback, and redirects retain independent validation. Configuration protection extends the existing editor/version-store owners without a parallel store, SQL/schema change or lifecycle state machine. The new user configuration fields do add configuration surface, but their scope and trust trade-off are documented.
Tests and documentation were reviewed separately. Local verification passed: web-fetch 86; astra-config library 90; config versions 15; real proxy/redirect/deadline transport 2; config editor 6; real CLI missing-HOME/state-root regression 1 (200 total). Two explicit live-network tests remain ignored. Formatting and diff checks pass. I ran the CLI regression explicitly because the current required CLI CI command does not select the config_version_cli integration target.
CI is NOT entirely green: the current head's live harness run completed 16/18, with (1) introspection_reflection_source_boundary failing the introspect depth/horizon argument predicates and (2) parallel_readonly_batching lacking a judge verdict due to server_error at completion. The ordinary Test Suite and Static Checks pass. I found no evidence tying those two failures to this PR's web-fetch changes, but the safe artifacts do not expose the exact mismatching arguments, so I am not claiming the first failure is proven flaky or resolved. This approval does not waive the remaining CI failures.
Real fake-IP VPN behavior was not independently rerun. Proxy-only remote DNS and mixed/IPv6 fake-IP answers remain explicitly outside this PR's supported scope.
✅ Merged into
|
| Condition | Required by | |
|---|---|---|
| ✅ | #review-threads-unresolved = 0 |
GitHub branch protection |
| ✅ | github-review-approved |
GitHub branch protection |
The batch, tested in place
| Condition | Required by | |
|---|---|---|
| ✅ | any of: check-success = Check PR title, check-neutral = Check PR title, check-skipped = Check PR title |
GitHub branch protection |
| ✅ | any of: check-success = check, check-neutral = check, check-skipped = check |
GitHub branch protection |
| ✅ | any of: check-success = Test: core crates + bridge hooks, check-neutral = Test: core crates + bridge hooks, check-skipped = Test: core crates + bridge hooks |
GitHub branch protection |
| ✅ | any of: check-success = Test: astra-runtime, check-neutral = Test: astra-runtime, check-skipped = Test: astra-runtime |
GitHub branch protection |
| ✅ | any of: check-success = Test: astra-cli (edge-fs-tools), check-neutral = Test: astra-cli (edge-fs-tools), check-skipped = Test: astra-cli (edge-fs-tools) |
GitHub branch protection |
| ✅ | any of: check-success = Test: astra-cli (edge-git-gix), check-neutral = Test: astra-cli (edge-git-gix), check-skipped = Test: astra-cli (edge-git-gix) |
GitHub branch protection |
| ✅ | any of: check-success = Test: astra-cli (edge-rest), check-neutral = Test: astra-cli (edge-rest), check-skipped = Test: astra-cli (edge-rest) |
GitHub branch protection |
| ✅ | any of: check-success = Test: astra-cli (edge-shell), check-neutral = Test: astra-cli (edge-shell), check-skipped = Test: astra-cli (edge-shell) |
GitHub branch protection |
| ✅ | any of: check-success = Test: astra-cli (non-edge), check-neutral = Test: astra-cli (non-edge), check-skipped = Test: astra-cli (non-edge) |
GitHub branch protection |
| ✅ | any of: check-success = Test: online (core), check-neutral = Test: online (core), check-skipped = Test: online (core) |
GitHub branch protection |
| ✅ | any of: check-success = Test: online (integration), check-neutral = Test: online (integration), check-skipped = Test: online (integration) |
GitHub branch protection |
| ✅ | any of: check-success = Test: turn-core + services + plan, check-neutral = Test: turn-core + services + plan, check-skipped = Test: turn-core + services + plan |
GitHub branch protection |
| ✅ | any of: check-skipped = web (typecheck, test, build), check-neutral = web (typecheck, test, build), check-success = web (typecheck, test, build) |
GitHub branch protection |
| ✅ | any of: check-skipped = Astra SDK (typecheck, test+coverage, build), check-neutral = Astra SDK (typecheck, test+coverage, build), check-success = Astra SDK (typecheck, test+coverage, build) |
GitHub branch protection |
| ✅ | any of: check-success = Test: macOS workspace coordination, check-neutral = Test: macOS workspace coordination, check-skipped = Test: macOS workspace coordination |
GitHub branch protection |
Times in UTC · dashboard · how the merge queue works
Edge/CLI web_fetch and web_search blocked every destination behind a fake-IP VPN (Clash/mihomo, Surge, sing-box), because the system resolver answers from 198.18.0.0/15. Users can now opt in with a user-level [web_fetch] section: when every answer is in the fake range, configured DNS-over-HTTPS resolvers supply the real addresses and the fetch is admitted only if all of them are public. The pinned connection still uses the fake address. Mixed or literal fake answers, empty answers, private real addresses and unreachable resolvers stay blocked. Only the user-level file or --settings can enable it: project runtime.toml drops the section, there is no environment variable (.env is loaded from the working directory), and only FetchNetwork::local_environment can carry the policy, so Server transport is unchanged. Blocked fake-IP fetches now explain the cause and the opt-in instead of a bare SSRF message. Closes matrixorigin#1141 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Reject a workspace .env that sets ASTRA_LOCAL_STATE_ROOT, reusing the MOI_AUTH_DIR boundary. Otherwise a repository could relocate the user root to its own .astra and have its runtime.toml, permissions and workspace trust load as user-level state. - Keep a private A or AAAA answer even when the other query fails, so a later resolver cannot admit a host one resolver already reported as private. - Identify resolvers only by position and host in diagnostics and reject resolver URLs with embedded credentials. - Block IANA IPv6 special-purpose ranges that are not globally reachable, including 2001:2::/48 used by sing-box for IPv6 fake-IP (pre-existing gap that let such answers pass as public). - Report IPv6 fake-IP answers and mixed fake/real answers as unsupported instead of as plain private addresses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oH cost - Rename the opt-in to `fake_ip = "block" | "allow_doh_screened"` and describe it everywhere as a relaxation of the private-network boundary: the VPN picks the real destination, and DoH only screens hosts whose public answer is private. Add a test that makes the DoH/VPN divergence explicit. - Remove user-only sections from a project runtime.toml before typed validation, so a malformed [web_fetch] there cannot discard the rest of the project layer. - Pin the first hop to the address set validated before the cache lookup instead of resolving and screening it twice; redirect hops are still resolved and validated individually. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e DoH answers - Add HOME to the shell-only variables the CLI checks after loading a workspace .env. Without a shell HOME, a .env could point it at the workspace and have its .astra load unfiltered as the user layer. - Treat a truncated DoH response, or NXDOMAIN for one address type next to records for the other, as a resolver failure; keep any private address such a response carried as blocking evidence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
f8dc1ae to
9827c16
Compare
Capability assessment: Unchanged: This changes the existing default-off local web-fetch policy and its user-facing diagnostics; Server retains direct pinned execution and the existing User Runner execution and permissions assessment remains accurate.
Summary
Fake-IP VPNs can cause local web fetch and search to reject public domains before the local network transport is used. Users may explicitly allow IPv4 fake-IP answers with configured DNS-over-HTTPS resolvers. This is a private-network boundary relaxation: the VPN chooses the actual destination, which the DNS screen cannot verify.
The screen validates DNS response questions, record ownership and bounded CNAME chains, rejects malformed or incomplete responses, preserves private-address evidence, and tries configured resolvers in order. A and AAAA remain parallel. Empty answers cannot allow a request. Server transport remains direct and pinned; fake-IP policy is local-only. Invalid local policy is reported at the affected request without exposing resolver secrets. The shared address guard also blocks 0.0.0.0/8, including embedded IPv4 forms.
User-only web-fetch settings are excluded from project config saves. User config files and local config-version blobs use private Unix file permissions; concurrent version publication is serialized using the existing index lock and refuses unsafe existing blob targets. Project saves reject a destination that aliases the user config instead of overwriting it.
Related issue
Refs #1141; this PR addresses IPv4 fake-IP DNS and intentionally leaves proxy-only DNS and mixed/IPv6 fake-IP support for follow-up. Broader special-purpose address classification remains tracked by #1140.
Change type
User and compatibility impact
Default behavior remains blocked; the optional local mode relies on VPN routing and does not guarantee a public destination. A user who chooses a shell-owned state root explicitly still chooses its trusted configuration directory. Project config saves cannot write user-only settings; if user and project config are the same file, use SaveUser. Existing local config-version blobs are protected when reused.
Architecture and complexity delta
astra-toolsweb fetch owns network screening and failure results;astra-configowns config versions; the CLI config editor uses the existing user-only section list.Verification
cargo fmt --all --checkandgit diff --check: passed.cargo test -p astra-tools --lib web_fetch::: 86 passed.cargo test -p astra-tools --test web_transport: 2 passed; 2 live-network tests ignored.cargo test -p astra-config --test config_versions: 15 passed.cargo test -p astra-cli --lib tui::config_edit_router::tests::: 6 passed.cargo clippy -p astra-tools -p astra-config -p astra-edge -p astra-cli --all-targets -- -D warnings: passed.Final checklist