fix(proxy): don't cross-deny sibling routes sharing an upstream by zbleness · Pull Request #1437 · nolabs-ai/nono

zbleness · 2026-07-17T13:10:44Z

select_intercept_route returned 403 as soon as any route for the upstream evaluated to Deny. A legacy endpoint_rules allow-list compiles to a non-explicit default-deny policy, so when several credential routes share one upstream host with disjoint endpoint_rules, the first route (by prefix order) whose rules didn't match the request path hard-denied the whole request, even though a sibling route would have authorized and injected a credential.

Treat a non-explicit default-deny as "this route does not apply" instead of a terminal 403, mirroring route::select_route: drop a managed-credential route, or let a credential-less ep route gate the request via has_endpoint_only_route so the post-loop check still 403s only when nothing authorized it. Explicit endpoint policies keep their authoritative hard-deny. Fix covers both the HTTP/1.1 and HTTP/2 intercept paths.

Add a regression test for two disjoint credential routes on one upstream.

Linked Issue

Closes #1436

Summary

Fixes #1436

Test Plan

make ci

Checklist

An issue exists and is linked above
All commits are signed-off, using DCO
All new code follows the project's coding standards (CLAUDE.md) and is covered by tests
Public-facing changes are paired with documentation updates
Release note has been added to CHANGELOG.md if needed

select_intercept_route returned 403 as soon as any route for the upstream evaluated to Deny. A legacy endpoint_rules allow-list compiles to a non-explicit default-deny policy, so when several credential routes share one upstream host with disjoint endpoint_rules, the first route (by prefix order) whose rules didn't match the request path hard-denied the whole request, even though a sibling route would have authorized and injected a credential. Treat a non-explicit default-deny as "this route does not apply" instead of a terminal 403, mirroring route::select_route: drop a managed-credential route, or let a credential-less _ep_ route gate the request via has_endpoint_only_route so the post-loop check still 403s only when nothing authorized it. Explicit endpoint policies keep their authoritative hard-deny. Fix covers both the HTTP/1.1 and HTTP/2 intercept paths. Add a regression test for two disjoint credential routes on one upstream. Signed-off-by: Zalan Blenessy <zalan.blenessy@volvocars.com>

github-actions · 2026-07-17T13:10:57Z

PR Review Summary

Size

Metric	Value
Lines added	+87
Lines removed	-0
Total changed	87
Classification	Medium (50–300 lines)

Affected crates

crates/nono-proxy — downstream consumers depend on this crate. API or behaviour changes will affect external callers; treat any breaking change with extra scrutiny.

Blast radius — Contained

This PR touches: source code

_{Updated automatically on each push to this PR.}

nogent-nolabs-ai

nogent code review

No blocking issues; 0 findings.

Findings: none flagged in scope.

_{Automated code + security review. CI already covers clippy, rustfmt, tests, cargo-audit and commit-lint.}

SequeI

Makes sense and LGTM, thanks for the fix!

main gained a disjoint-credential-routes regression test (nolabs-ai#1437) written against the pre-refactor select_intercept_route signature. Update it to use InterceptRouteRequest/SelectedRoute so it compiles after the rebase. Signed-off-by: Christine Le <christine.le@datadoghq.com>

…ercept routes (#1443) * feat(proxy): add authenticated WebSocket tunneling for CONNECT/TLS-intercept routes Codex-style credential-provider routes hang for ~1min on WebSocket upgrades because they route through the CONNECT/TLS-intercept path, not the reverse-proxy path Part 1 covered. Adds fail-fast upgrade classification, a declarative per-route upgrade allow-list (UpgradeProtocol/UpgradeRuleConfig, compiled into LoadedRoute and CredentialRouteDef), and handle_websocket_upgrade in tls_intercept/handle.rs, which reuses the existing header filtering, credential injection, and phantom-nonce resolution pipeline before dialing the upstream raw and relaying bytes via copy_bidirectional after a validated 101 handshake. Signed-off-by: Christine Le <christine.le@datadoghq.com> * feat(proxy): route-integrated WebSocket upgrade gating and strict HTTP/1 framing Move WebSocket upgrade-rule matching into route selection itself so an unmatched upgrade path is rejected before any upstream is contacted, add an explicit 501 for SPIFFE/AWS-authenticated routes (WebSocket tunneling is unsupported for those credential mechanisms), and make phantom-nonce resolution fail closed: an unresolved phantom nonce in a forwarded header now yields a 403 instead of leaking the unresolved token upstream. Simplify the declarative upgrade-rule schema to just a path (protocol and method are implied for classic WebSocket), and split handshake parsing and strict, framing-aware response relaying into new http1.rs/websocket.rs modules shared between the WS and non-WS intercept paths. Signed-off-by: Christine Le <christine.le@datadoghq.com> * fix(proxy): update route-selection test for rebase onto main main gained a disjoint-credential-routes regression test (#1437) written against the pre-refactor select_intercept_route signature. Update it to use InterceptRouteRequest/SelectedRoute so it compiles after the rebase. Signed-off-by: Christine Le <christine.le@datadoghq.com> * fix(proxy): add missing upgrades field to Linux-only server.rs tests Two RouteConfig literals in tests gated by #[cfg(not(target_os = "macos"))] were missing the new upgrades field, so they only failed to compile on Linux CI (invisible on a macOS dev machine). Signed-off-by: Christine Le <christine.le@datadoghq.com> * fix(proxy): address PR #1443 review findings for WS tunnel intercept Forward client bytes buffered immediately after the WS upgrade headers to upstream instead of silently dropping them, verify Sec-WebSocket-Accept against RFC 6455, reject h2 extended-CONNECT WebSocket upgrades that would otherwise skip upgrade_rules, validate resolved nonce values for CRLF/NUL before header injection, collapse dot-segments in normalize_path, and collapse the duplicate ad-hoc header parser onto the shared strict parser. Signed-off-by: Christine Le <christine.le@datadoghq.com> * fix(cli): add upgrades to credential_routes JSON schema kipz reported that profiles using the new upgrades field on credential_routes parse and pass `nono profile validate --strict` but fail validation against the published schema, since CredentialRouteDef there still only lists [base_url_env_var, endpoint_policy, env_var, name, provider]. Add upgrades (CredentialWebSocketRuleDef: origin, path) to match the Rust model, plus a schema_shape regression test. * fix(cli): tighten upgrades origin/path schema to match runtime validation The prior commit's CredentialWebSocketRuleDef used bare unconstrained strings for origin/path, looser than both the sibling CredentialProviderTokenEndpoint schema (host/path) and the actual runtime checks in validate_provider_origin/validate_provider_path, which require an https-only origin with no path/query/fragment and an absolute path starting with '/'. Use the existing UrlOrigin $ref for origin and add minLength/pattern for path so malformed profiles fail schema validation instead of only failing later at `nono profile validate --strict`. * fix(proxy): update select_intercept_route tests to current signature The approval-flow tests in tls_intercept::handle were written against a pre-merge version of select_intercept_route/SelectedRoute/RouteConfig and no longer compiled after merging main: the function now takes an InterceptRouteRequest struct instead of positional method/path args, SelectedRoute needs Debug for panic! formatting, and RouteConfig gained a required `upgrades` field. Signed-off-by: christine.le <christine.le@datadoghq.com> --------- Signed-off-by: Christine Le <christine.le@datadoghq.com> Signed-off-by: christine.le <christine.le@datadoghq.com> Co-authored-by: Luke Hinds <lukehinds@gmail.com>

github-actions Bot added bug Something isn't working nono-proxy size/medium labels Jul 17, 2026

nogent-nolabs-ai Bot reviewed Jul 17, 2026

View reviewed changes

SequeI approved these changes Jul 17, 2026

View reviewed changes

SequeI merged commit 23d93fc into nolabs-ai:main Jul 17, 2026
17 checks passed

zbleness deleted the fix/disjoint-credentials-on-upstream-host branch July 20, 2026 17:07

BrewTestBot mentioned this pull request Jul 20, 2026

nono 0.69.0 Homebrew/homebrew-core#294224

Merged

BrewTestBot mentioned this pull request Jul 29, 2026

nono 0.70.0 Homebrew/homebrew-core#295925

Merged

scottames-github-bot Bot mentioned this pull request Aug 3, 2026

chore(deps): update ⬆️ mise-packages scottames/dots#1002

Merged

1 task

Navigation Menu

Uh oh!

fix(proxy): don't cross-deny sibling routes sharing an upstream - #1437

fix(proxy): don't cross-deny sibling routes sharing an upstream#1437
SequeI merged 1 commit into
nolabs-ai:mainfrom
zbleness:fix/disjoint-credentials-on-upstream-host

zbleness commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

nogent-nolabs-ai Bot left a comment

Uh oh!

SequeI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

zbleness commented Jul 17, 2026

Linked Issue

Summary

Test Plan

Checklist

Uh oh!

github-actions Bot commented Jul 17, 2026

PR Review Summary

Size

Affected crates

Blast radius — Contained

Uh oh!

nogent-nolabs-ai Bot left a comment

Choose a reason for hiding this comment

nogent code review

Uh oh!

SequeI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants