fix(sandbox): canonicalize HTTP request-targets before L7 policy evaluation

[johntmyers]

Summary

• The L7 REST proxy evaluates OPA path rules against the raw HTTP request-target and forwards the raw header block byte-for-byte to the upstream. The upstream normalizes .., %2e%2e, and // before dispatching, so a compromised agent inside the sandbox can bypass any path-based allow rule — e.g. GET /public/../secret matches /public/** at the proxy but the upstream serves /secret.
• The inference-routing detection (normalize_inference_path) had the same normalization gap: it only stripped scheme+authority and preserved dot-segments verbatim, so the same class of payload could also dodge inference routing.
• This PR introduces a single URL-path canonicalizer used at every L7 enforcement site. The canonical path is both the input to OPA and the bytes written onto the wire — closing the parser-differential between the policy engine and the upstream.

closes OS-99

Changes

• crates/openshell-sandbox/src/l7/path.rs (new)
  • canonicalize_request_target(target, opts) -> Result<(CanonicalPath, Option<String> /* raw query */), CanonicalizeError>.
  • Percent-decodes (uppercase hex on re-emit), resolves . / .. per RFC 3986 5.2.4 (rejects underflow rather than silently clamping to /), collapses doubled slashes, strips trailing ;params, handles origin-form and absolute-form targets, enforces a 4 KiB length bound, rejects fragments / raw non-ASCII / control bytes / encoded slashes (unless explicitly opted in per-endpoint via allow_encoded_slash).
  • Returns a rewritten flag so callers know whether the outbound request line needs to be rebuilt.
• crates/openshell-sandbox/src/l7/rest.rs
  • parse_http_request canonicalizes the request-target before returning the L7Request. If the canonical form differs from the raw input, the request line in raw_header is rebuilt via rewrite_request_line_target so the upstream dispatches on the same bytes the policy evaluated.
  • RestProvider now carries a CanonicalizeOptions field. Callers that need per-endpoint options construct via RestProvider::with_options(opts); default strict behavior via RestProvider::default().
  • parse_query_params is now pub(crate) so proxy.rs can reuse it.
• crates/openshell-sandbox/src/l7/relay.rs
  • relay_rest builds its RestProvider from the endpoint's allow_encoded_slash, so L7 endpoints backed by upstreams that legitimately embed %2F (e.g. GitLab namespaced paths) can opt in per endpoint.
  • relay_passthrough_with_credentials uses the strict default.
• crates/openshell-sandbox/src/l7/mod.rs
  • L7EndpointConfig gains allow_encoded_slash: bool (default false).
  • parse_l7_config reads it from the Rego endpoint object.
• crates/openshell-policy/src/lib.rs + proto/sandbox.proto
  • NetworkEndpointDef (YAML), NetworkEndpoint (proto) gain the allow_encoded_slash field; to_proto copies it. Operators set allow_encoded_slash: true on an endpoint in YAML to opt in.
• crates/openshell-sandbox/src/proxy.rs
  • The forward (plain HTTP proxy) L7 evaluation site canonicalizes the request-target. The canonical form is reassigned to the outer path so the later rewrite_forward_request call writes canonical bytes to the upstream (closes the parser-differential for this site — previously only the L7 tunnel was closed). Non-canonical input is denied with 400 Bad Request and an OCSF NetworkActivity Fail event.
  • normalize_inference_path is now a thin wrapper over the canonicalizer (falls back to the raw path on canonicalization error so the normal forward path can reject properly).
• crates/openshell-sandbox/data/sandbox-policy.rego
  • Comment documenting the invariant: input.request.path is pre-canonicalized, so rules must not attempt defensive matching against .. or %2e%2e.
• architecture/sandbox.md
  • Adds the new l7/path.rs module and notes the allow_encoded_slash per-endpoint opt-in.

Testing

• cargo test -p openshell-sandbox — 524 lib + integration tests pass.
• cargo test -p openshell-policy — 47 tests pass.
• mise run pre-commit — passes (lint, format, license headers, rust tests).
• Unit tests in l7::path::tests (24) cover dot segments, percent-encoded dot segments, double-slash collapse, encoded-slash reject/opt-in, null/control byte rejection, fragment rejection, absolute-form stripping, legitimate percent-encoded bytes round-tripping, mixed-case percent normalization, length bound, non-ASCII rejection, query-suffix separation, rewritten-flag semantics.
• Integration tests in l7::rest::tests (9 new):
  • parse_http_request_canonicalizes_target_and_rewrites_raw_header asserts both L7Request.target (what OPA sees) and raw_header (what the upstream receives) are canonical.
  • parse_http_request_canonicalization_preserves_query_string, parse_http_request_leaves_canonical_input_byte_for_byte, parse_http_request_preserves_http_10_version_on_rewrite.
  • parse_http_request_accepts_encoded_slash_when_endpoint_opts_in / _rejects_encoded_slash_by_default verify the allow_encoded_slash gate end-to-end.
  • parse_http_request_rejects_traversal_above_root.
• proxy::tests::test_rewrite_forward_request_uses_canonical_path_on_the_wire — regression test asserting that once the caller canonicalizes, rewrite_forward_request produces canonical bytes on the wire.
• l7::tests::parse_l7_config_allow_encoded_slash_{defaults_false,opt_in} verify the YAML/proto/Rego wiring of the opt-in.
• Regression tests for the documented bypass payloads:
  • /public/../secret → /secret
  • /public/%2e%2e/secret → /secret
  • /public/%2E%2E%2Fsecret → EncodedSlashNotAllowed
  • //public/../secret → /secret
  • /public/./../secret → /secret
  • /public/%00/secret → NullOrControlByte
  • /.. → TraversalAboveRoot
• Legitimate traffic continues to pass: /files/hello%20world.txt, /users/caf%C3%A9, /v1/chat/completions?stream=true, paths with : and @ in segments.

Trade-offs

• %2F inside a segment is rejected by default. Operators who need it for artifact-registry-style APIs (e.g. GitLab namespaced project paths) set allow_encoded_slash: true on the endpoint in their YAML policy.
• Requests that fail canonicalization are hard-rejected regardless of EnforcementMode::Audit — audit mode is for policy-tuning, not for tolerating protocol violations. Requests that succeed canonicalization are rewritten (if non-canonical) and then handled per the endpoint's enforcement mode as usual.
• %20 and similar legitimate percent-encoded bytes are preserved in the canonical form (only unreserved pchars get decoded), so policy patterns that currently match /files/hello%20world.txt continue to work.

Checklist

• Conventional Commits format
• No secrets or credentials in diff
• Scoped to the issue at hand (no unrelated refactors)
• Architecture and rego documentation updated

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[mjamiv]

+1 on security merit. Path-based OPA rules are load-bearing for binary-scoped network policies in real deployments, and .. / %2e%2e / // bypass on the L7 path is an active concern for anyone using path-level egress allowlisting on inference proxies. Worth landing.

[pimlock]

From description:

Non-canonical requests are always hard-rejected regardless of EnforcementMode::Audit — audit mode is for policy-tuning, not for tolerating protocol violations.

From what I can tell, only requests that the canonicalization fails for are rejected, but non-canonical requests that pass canonicalization are accepted.

This would be more accurate: "Requests that fail canonizalication are hard-rejected regardless of EnforcementMode::Audit..."

[pimlock]

%2F inside a segment is rejected by default. Operators who need it for artifact-registry-style APIs can opt in per-endpoint via allow_encoded_slash: true (the plumbing is in place — endpoint config wire-up can land in a follow-up once there's a consumer).

I was looking for things this could potentially break. One example is GitLab that uses %2F for encoding paths (https://docs.gitlab.com/api/rest/#namespaced-paths).

That would mean mean that:

• if I have a L4-only policy for GitLab -> it will work
• if I switch to L7 (e.g. want to enforce things on more granular level) -> the request will be rejected if it contains %2F

[pimlock]

Security review — one finding I'd like to raise before merge, plus a note on tests.

Finding: handle_forward_proxy evaluates OPA on the canonical path but forwards the raw path to the upstream

The PR's stated invariant (l7/path.rs:4-12) is that the canonical path is both what OPA evaluates and the bytes written on the wire. That holds in l7/rest.rs:parse_http_request — which calls rewrite_request_line_target when canonical.rewritten is true — but is only half-implemented in proxy.rs:handle_forward_proxy.

Trace:

1. path extracted raw from parse_proxy_uri(target_uri) at proxy.rs:1984.
2. Inside the if let Some(l7_config) block at proxy.rs:2163-2202, the raw path is canonicalized into a new local canonical_path and fed to OPA via request_info.target. Policy sees canonical.
3. The canonical_path binding goes out of scope at the L7 block close around proxy.rs:2291. Only the outer raw path from step 1 is live after that point.
4. At proxy.rs:2523, rewrite_forward_request(buf, used, &path, …) is called with the raw path. That path is written byte-for-byte into the outbound request line at proxy.rs:1894 (output.extend_from_slice(path.as_bytes())).

Impact: For the plain-HTTP forward-proxy path, OPA decides on canonical_path while the upstream receives the raw path and re-normalizes independently. If upstream normalization matches this canonicalizer byte-for-byte, no harm. If it diverges on any edge (double-decoding, ;params handling, percent-slash treatment, different .. semantics), the parser-differential this PR is meant to close is still open for this call site.

Contrast with l7/rest.rs:171-179 where the same canonicalization is carried through to the wire via rewrite_request_line_target.

Fix options:

• Option A (smaller): mark path as mut and reassign path = canon.path inside the Ok branch at proxy.rs:2167-2173 instead of binding a fresh canonical_path. The later rewrite_forward_request(buf, used, &path, …) then picks up the canonical form naturally.
• Option B: call rewrite_request_line_target (or its forward-proxy equivalent) against buf inside the L7 block when canonical.rewritten is true. Mirrors what rest.rs does.

Either closes the finding.

Missing integration tests

All 24 new tests in l7::path::tests exercise canonicalize_request_target in isolation — none drive a full request through parse_http_request → rewrite_request_line_target, or through handle_forward_proxy → rewrite_forward_request, to assert on the bytes that actually reach the upstream. That assertion is what the PR's core invariant hinges on (what OPA sees == what the upstream receives), and it would have caught the gap above immediately.

Suggest adding at minimum:

• A test that drives rest.rs:parse_http_request with a non-canonical input, inspects the returned L7Request.raw_header, and asserts the request line is canonical.
• A test that drives handle_forward_proxy with a non-canonical input, captures what's written to a mock upstream, and asserts the same. This would fail against the current code.

[johntmyers]

Thanks for the careful review — all three points addressed in cb87e28. Replying inline so each fix maps back to what it's responding to.

---

From description:

Non-canonical requests are always hard-rejected regardless of EnforcementMode::Audit — audit mode is for policy-tuning, not for tolerating protocol violations.

From what I can tell, only requests that the canonicalization fails for are rejected, but non-canonical requests that pass canonicalization are accepted.

This would be more accurate: "Requests that fail canonizalication are hard-rejected regardless of EnforcementMode::Audit..."

Good catch — my wording conflated the two cases. Updated the PR description to "Requests that fail canonicalization are hard-rejected regardless of EnforcementMode::Audit...". Requests that canonicalize successfully to a non-canonical input (e.g. /a/./b → /a/b) are rewritten and continue through the normal enforcement path.

---

%2F inside a segment is rejected by default. Operators who need it for artifact-registry-style APIs can opt in per-endpoint via allow_encoded_slash: true (the plumbing is in place — endpoint config wire-up can land in a follow-up once there's a consumer).

I was looking for things this could potentially break. One example is GitLab that uses %2F for encoding paths [...]

That would mean mean that:

• if I have a L4-only policy for GitLab -> it will work
• if I switch to L7 (e.g. want to enforce things on more granular level) -> the request will be rejected if it contains %2F

Fair — "follow-up" was the wrong answer given you can hit this today. Wired through end to end:

• NetworkEndpointDef (YAML) and NetworkEndpoint (proto) gain allow_encoded_slash: bool (default false).
• L7EndpointConfig carries it; parse_l7_config reads it from the Rego endpoint object.
• relay_rest constructs RestProvider::with_options(...) using the per-endpoint value so the parser honors the operator's setting.
• Forward-proxy path in proxy.rs uses the same l7_config.allow_encoded_slash.

GitLab usage: set allow_encoded_slash: true on the endpoint in YAML and /api/v4/projects/group%2Fproject round-trips verbatim. Default-strict preserved; nothing changes for existing policies. Covered by parse_http_request_accepts_encoded_slash_when_endpoint_opts_in / _rejects_encoded_slash_by_default and parse_l7_config_allow_encoded_slash_{defaults_false,opt_in}.

---

Finding: handle_forward_proxy evaluates OPA on the canonical path but forwards the raw path to the upstream

[...]

• Option A (smaller): mark path as mut and reassign path = canon.path inside the Ok branch at proxy.rs:2167-2173 instead of binding a fresh canonical_path. The later rewrite_forward_request(buf, used, &path, …) then picks up the canonical form naturally.

Confirmed the gap, thanks — this was real. Went with Option A:

• path is now mut at the parse_proxy_uri destructuring.
• Inside the L7 canonicalize Ok branch, path = canon.path so every downstream use (OPA input via request_info.target, the OCSF decision log, and critically rewrite_forward_request(buf, used, &path, ...)) sees the canonical form.
• Non-canonical input continues to be rejected with 400 Bad Request + OCSF NetworkActivity::Fail.

Missing integration tests

All 24 new tests in l7::path::tests exercise canonicalize_request_target in isolation [...]

Also fair. Added end-to-end assertions:

• l7::rest::tests::parse_http_request_canonicalizes_target_and_rewrites_raw_header — drives GET /public/../secret HTTP/1.1 through parse_http_request and asserts both L7Request.target (what OPA sees) and raw_header (what the upstream receives) are /secret. This is the core invariant assertion.
• proxy::tests::test_rewrite_forward_request_uses_canonical_path_on_the_wire — the equivalent assertion for the forward-proxy flow. Fails against the prior code; passes with the fix above.
• Plus rest-side tests for query-string preservation on rewrite, byte-for-byte pass-through when the input is already canonical, HTTP/1.0 version preservation on rewrite, traversal-above-root rejection, and the allow_encoded_slash opt-in gate.

---

524 sandbox + 47 policy tests pass. mise run pre-commit clean.