[PR #174] [CLOSED] fix(proxy): stop leaking the /proxy prefix and the sandbox address to clients #227

New issue

Closed

opened 2026-10-29 14:21:13 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-29 14:21:13 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/kvcache-ai/AgentENV/pull/174
Author: @suixinio
Created: 8/15/2026
Status: ❌ Closed

Base: main ← Head: fix/proxy-root-prefix-and-self-redirects

📝 Commits (2)

f3035a2 fix(proxy): strip the prefix for the bare /proxy/ path
bf54f6e fix(proxy): keep the sandbox address out of proxied redirects

📊 Changes

1 file changed (+274 additions, -3 deletions)

View changed files

📝 src/api/proxy.rs (+274 -3)

📄 Description

What

Two fixes on the sandbox proxy hop in src/api/proxy.rs, both about internal proxy details reaching the client:

/proxy/ — the prefix with nothing after it — matched neither prefix route, fell through to the header-routed fallback, and was forwarded to the sandbox unmodified, so the sandbox saw /proxy/ instead of /.
Location / Content-Location values pointing back at the sandbox's internal ip:port were passed through to the client verbatim.

Why

Bare /proxy/. The router matches /proxy exactly and /proxy/{*proxy_path} with a non-empty segment, so the empty-remainder shape reaches proxy_via_fallback, whose contract is to forward the path unchanged. The distributed gateway rewrites data-plane traffic to "/proxy" + path (upstreamTargetPath), so a client request for / always arrives at the node in exactly that shape. Consequences on a real deployment:

applications without an SPA fallback answer their own home page with 404;
dev servers normalize the trailing slash into Location: /proxy, which is relative, so it lands in the client's address bar and 404s on the next request;
root-path websockets (Vite HMR) never reach the application.

Every non-root path was fine, which makes this look like an application bug rather than a proxy one. sandbox_proxy_classifier already treats path == "/proxy" || path.starts_with("/proxy/") as proxy traffic, so the classifier and the router disagreed about this one shape.

Sandbox address in redirects. Requests are forwarded with Host set to the upstream authority, so an application that builds absolute URLs from Host returns the sandbox's internal address in Location. Passing it through both exposes the sandbox network layout and points the client's next request at an address that is only reachable from that node.

None. Scope is small and obvious, submitted directly per CONTRIBUTING.

Scope and non-goals

Included:

one routing-table entry for /proxy/, reusing the existing proxy_via_prefix handler;
rewriting self-referencing Location / Content-Location on the HTTP response path and on a rejected websocket handshake;
three unit tests.

Not included:

no change to request semantics, Host handling, or the fallback contract;
response bodies are not rewritten — an application that writes absolute URLs into HTML still emits them, which is outside what a reverse proxy should do;
host-based and header-based routing are otherwise untouched.

Design and behavior changes

Routing. .route("/proxy/", any(proxy_via_prefix::<I>)). The handler is unchanged: strip_proxy_prefix("/proxy/") already yields /. The set of paths the gateway can produce is closed — "/proxy" + <path starting with /> is either /proxy/ or /proxy/<non-empty> — and only the first was unrouted. Trailing slashes are not the criterion: /proxy/station/ (remainder station/) and /proxy// (remainder /) already matched the catch-all and keep their exact upstream paths.

Responses. rewrite_upstream_self_references() runs after hop-by-hop stripping in map_upstream_response and map_websocket_handshake_rejection_response. It rewrites a header only when the value parses as a URI whose authority equals the upstream authority, replacing it with path_and_query (/ when absent). Relative values and absolute URLs to any other host — an OAuth hop, a CDN — are left untouched.

Compatibility and operations

Public API or generated protocol: N/A — no schema or generated code touched.
Configuration or defaults: N/A.
Snapshot manifest, artifact layout, or storage format: N/A.
Upgrade and rollback: behavior-only, no persisted state; rolling back restores the previous behavior. A client that relied on receiving the sandbox's absolute address in Location now receives a path-relative value — that address was never reachable off-node.
Host requirements, permissions, ports, or dependencies: N/A.
Documentation: docs/src/concepts/proxy.md already documents ANY /proxy as forwarding to / inside the sandbox, so it describes the fixed behavior and needs no change.

Validation

make fmt
make clippy — scoped to the changed package, see below
make test-unit — first command of the target only, see below
Relevant Rust integration tests — not run: require root, /dev/kvm and network namespaces, unavailable in the container used for these checks
make -C services test — N/A, no services/ change
Generated clients/server regenerated with the documented make target — N/A, no generated code touched
Documentation updated — N/A, see above
Benchmarks or performance comparison completed — N/A, no performance-sensitive change

Commands and results:

$ cargo fmt --all -- --check
FMT_STATUS=0

$ cargo clippy -p agentenv --all-targets -- -D warnings
CLIPPY_STATUS=0   # no warnings from this package

$ cargo test -p agentenv --lib api::proxy
test result: ok. 36 passed; 0 failed; 0 ignored; 0 measured; 692 filtered out

$ cargo test -p agentenv --lib          # with this branch
test result: FAILED. 718 passed; 6 failed; 4 ignored

$ cargo test -p agentenv --lib          # unpatched c0c6bbb, same container, for comparison
test result: FAILED. 715 passed; 6 failed; 4 ignored

The same six tests fail before and after the change (privileges::tests::scoped_spawn_clears_child_capabilities_without_mutating_caller, sandbox::ublk::overlaybd::tests::compact_layers_mixed_input_and_configured_output, two snapshot::p2p::tests::local_overlaybd_layers_*, snapshot::repository::backends::common::acr::source_image::tests::plans_descriptorless_sparse_overlaybd_delta_for_dense_export, snapshot::repository::backends::posixfs::artifacts::tests::import_built_artifacts_preserves_zfile_memory_lower_bytes). They need privileges and real devices that the unprivileged container does not provide; the delta of this branch is exactly the three new passing tests.

Skipped checks and reasons: integration tests and the capability-runner steps of make test-unit need root, /dev/kvm and netns; the environment available for these checks is an unprivileged container.

Beyond unit tests, both fixes were verified on a two-node k3s deployment (gateway + node DaemonSet, one microVM sandbox):

# inside the sandbox: python3 -m http.server on 5173, which has no SPA fallback
client path                 sandbox access log        # after the fix
/                           GET /            200
/proxy                      GET /            200
/proxy/                     GET /            200      # was: GET /proxy/ 404
/proxy//                    GET //           200
/proxy/http.log             GET /http.log    200
/nope                       GET /nope        404      # fallback still forwards verbatim

# a Vite app behind the same gateway, seen from the browser
GET /    before: 307 Location: /proxy   ->  after: 200, application home page
GET /    websocket upgrade              ->  101 Switching Protocols, sec-websocket-protocol: vite-hmr
GET //   before: 308 Location: http://10.11.0.1:5173/  ->  after: 308 Location: /

Risks and reviewer notes

The new route only changes which handler serves /proxy/; a client that deliberately used header routing to reach a sandbox path literally named /proxy/ now reaches / instead. That path was already unreachable through the explicit prefix form.
The redirect rewrite compares authorities byte-wise. An application that echoes the same host in different case, or adds a default port, keeps its absolute URL — deliberately conservative, since a false negative leaks nothing that is not leaked today while a false positive would break a third-party redirect.
Both commits touch src/api/proxy.rs only, and they are independent: happy to split them into separate pull requests if you prefer to review or revert them separately.

Checklist

The PR contains one coherent change and no unrelated formatting or refactoring.
New behavior is covered by tests, or I explained why testing is impractical.
Logs and examples contain no credentials, tokens, or private registry information.
I did not manually edit generated code without updating its source and regenerating it.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/kvcache-ai/AgentENV/pull/174 **Author:** [@suixinio](https://github.com/suixinio) **Created:** 8/15/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/proxy-root-prefix-and-self-redirects` --- ### 📝 Commits (2) - [`f3035a2`](https://github.com/kvcache-ai/AgentENV/commit/f3035a2d82348d8e1540d3d5df6ee5842b6e1c65) fix(proxy): strip the prefix for the bare /proxy/ path - [`bf54f6e`](https://github.com/kvcache-ai/AgentENV/commit/bf54f6e60f675cda4568ce81cfb8fb8db4a5a3bc) fix(proxy): keep the sandbox address out of proxied redirects ### 📊 Changes **1 file changed** (+274 additions, -3 deletions) <details> <summary>View changed files</summary> 📝 `src/api/proxy.rs` (+274 -3) </details> ### 📄 Description ## What Two fixes on the sandbox proxy hop in `src/api/proxy.rs`, both about internal proxy details reaching the client: 1. `/proxy/` — the prefix with nothing after it — matched neither prefix route, fell through to the header-routed fallback, and was forwarded to the sandbox unmodified, so the sandbox saw `/proxy/` instead of `/`. 2. `Location` / `Content-Location` values pointing back at the sandbox's internal `ip:port` were passed through to the client verbatim. ## Why **Bare `/proxy/`.** The router matches `/proxy` exactly and `/proxy/{*proxy_path}` with a non-empty segment, so the empty-remainder shape reaches `proxy_via_fallback`, whose contract is to forward the path unchanged. The distributed gateway rewrites data-plane traffic to `"/proxy" + path` (`upstreamTargetPath`), so a client request for `/` always arrives at the node in exactly that shape. Consequences on a real deployment: - applications without an SPA fallback answer their own home page with 404; - dev servers normalize the trailing slash into `Location: /proxy`, which is relative, so it lands in the client's address bar and 404s on the next request; - root-path websockets (Vite HMR) never reach the application. Every non-root path was fine, which makes this look like an application bug rather than a proxy one. `sandbox_proxy_classifier` already treats `path == "/proxy" || path.starts_with("/proxy/")` as proxy traffic, so the classifier and the router disagreed about this one shape. **Sandbox address in redirects.** Requests are forwarded with `Host` set to the upstream authority, so an application that builds absolute URLs from `Host` returns the sandbox's internal address in `Location`. Passing it through both exposes the sandbox network layout and points the client's next request at an address that is only reachable from that node. ## Related issue None. Scope is small and obvious, submitted directly per CONTRIBUTING. ## Scope and non-goals Included: - one routing-table entry for `/proxy/`, reusing the existing `proxy_via_prefix` handler; - rewriting self-referencing `Location` / `Content-Location` on the HTTP response path and on a rejected websocket handshake; - three unit tests. Not included: - no change to request semantics, `Host` handling, or the fallback contract; - response bodies are not rewritten — an application that writes absolute URLs into HTML still emits them, which is outside what a reverse proxy should do; - host-based and header-based routing are otherwise untouched. ## Design and behavior changes **Routing.** `.route("/proxy/", any(proxy_via_prefix::<I>))`. The handler is unchanged: `strip_proxy_prefix("/proxy/")` already yields `/`. The set of paths the gateway can produce is closed — `"/proxy" + <path starting with />` is either `/proxy/` or `/proxy/<non-empty>` — and only the first was unrouted. Trailing slashes are not the criterion: `/proxy/station/` (remainder `station/`) and `/proxy//` (remainder `/`) already matched the catch-all and keep their exact upstream paths. **Responses.** `rewrite_upstream_self_references()` runs after hop-by-hop stripping in `map_upstream_response` and `map_websocket_handshake_rejection_response`. It rewrites a header only when the value parses as a URI whose authority equals the upstream authority, replacing it with `path_and_query` (`/` when absent). Relative values and absolute URLs to any other host — an OAuth hop, a CDN — are left untouched. ## Compatibility and operations - Public API or generated protocol: N/A — no schema or generated code touched. - Configuration or defaults: N/A. - Snapshot manifest, artifact layout, or storage format: N/A. - Upgrade and rollback: behavior-only, no persisted state; rolling back restores the previous behavior. A client that relied on receiving the sandbox's absolute address in `Location` now receives a path-relative value — that address was never reachable off-node. - Host requirements, permissions, ports, or dependencies: N/A. - Documentation: `docs/src/concepts/proxy.md` already documents `ANY /proxy` as forwarding to `/` inside the sandbox, so it describes the fixed behavior and needs no change. ## Validation - [x] `make fmt` - [x] `make clippy` — scoped to the changed package, see below - [x] `make test-unit` — first command of the target only, see below - [ ] Relevant Rust integration tests — not run: require root, `/dev/kvm` and network namespaces, unavailable in the container used for these checks - [ ] `make -C services test` — N/A, no `services/` change - [ ] Generated clients/server regenerated with the documented `make` target — N/A, no generated code touched - [ ] Documentation updated — N/A, see above - [ ] Benchmarks or performance comparison completed — N/A, no performance-sensitive change Commands and results: ```text $ cargo fmt --all -- --check FMT_STATUS=0 $ cargo clippy -p agentenv --all-targets -- -D warnings CLIPPY_STATUS=0 # no warnings from this package $ cargo test -p agentenv --lib api::proxy test result: ok. 36 passed; 0 failed; 0 ignored; 0 measured; 692 filtered out $ cargo test -p agentenv --lib # with this branch test result: FAILED. 718 passed; 6 failed; 4 ignored $ cargo test -p agentenv --lib # unpatched c0c6bbb, same container, for comparison test result: FAILED. 715 passed; 6 failed; 4 ignored ``` The same six tests fail before and after the change (`privileges::tests::scoped_spawn_clears_child_capabilities_without_mutating_caller`, `sandbox::ublk::overlaybd::tests::compact_layers_mixed_input_and_configured_output`, two `snapshot::p2p::tests::local_overlaybd_layers_*`, `snapshot::repository::backends::common::acr::source_image::tests::plans_descriptorless_sparse_overlaybd_delta_for_dense_export`, `snapshot::repository::backends::posixfs::artifacts::tests::import_built_artifacts_preserves_zfile_memory_lower_bytes`). They need privileges and real devices that the unprivileged container does not provide; the delta of this branch is exactly the three new passing tests. Skipped checks and reasons: integration tests and the capability-runner steps of `make test-unit` need root, `/dev/kvm` and netns; the environment available for these checks is an unprivileged container. Beyond unit tests, both fixes were verified on a two-node k3s deployment (gateway + node DaemonSet, one microVM sandbox): ```text # inside the sandbox: python3 -m http.server on 5173, which has no SPA fallback client path sandbox access log # after the fix / GET / 200 /proxy GET / 200 /proxy/ GET / 200 # was: GET /proxy/ 404 /proxy// GET // 200 /proxy/http.log GET /http.log 200 /nope GET /nope 404 # fallback still forwards verbatim # a Vite app behind the same gateway, seen from the browser GET / before: 307 Location: /proxy -> after: 200, application home page GET / websocket upgrade -> 101 Switching Protocols, sec-websocket-protocol: vite-hmr GET // before: 308 Location: http://10.11.0.1:5173/ -> after: 308 Location: / ``` ## Risks and reviewer notes - The new route only changes which handler serves `/proxy/`; a client that deliberately used header routing to reach a sandbox path literally named `/proxy/` now reaches `/` instead. That path was already unreachable through the explicit prefix form. - The redirect rewrite compares authorities byte-wise. An application that echoes the same host in different case, or adds a default port, keeps its absolute URL — deliberately conservative, since a false negative leaks nothing that is not leaked today while a false positive would break a third-party redirect. - Both commits touch `src/api/proxy.rs` only, and they are independent: happy to split them into separate pull requests if you prefer to review or revert them separately. ## Checklist - [x] The PR contains one coherent change and no unrelated formatting or refactoring. - [x] New behavior is covered by tests, or I explained why testing is impractical. - [x] Logs and examples contain no credentials, tokens, or private registry information. - [x] I did not manually edit generated code without updating its source and regenerating it. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-29 14:21:13 +00:00