Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds framework-agnostic embedding hooks, configurable probe endpoints, router collision checks, and supporting documentation, tests, and wiring across the proxy. ChangesEmbedded Tier-2 hooks and configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
252-324: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep configurable probe and verify paths exempt from maintenance.
maintenance_exemptis still seeded from the static defaults (/health/**,/metrics,/auth/verify). After this change, relocatinghealth.*,metrics.path, or the decider’s verify path means maintenance mode will return503for those endpoints, which breaks probe traffic and custom forward-auth even though the default paths were intentionally exempt before. Please derive the exemption list from the resolved configured paths when building the router state, and add a maintenance-mode regression test for the relocated paths.Also applies to: 380-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 252 - 324, The maintenance exemption list is still based on the static defaults, so custom health, metrics, and verify endpoints built in the router can be blocked during maintenance. Update the router-state setup around the health/metrics routes and the decider verify path so maintenance_exempt is derived from the resolved configured paths (health.path, health.live_path, health.ready_path, health.startup_path, metrics.path, and the configured verify path) instead of hardcoded defaults. Add a regression test covering relocated probe and verify paths to confirm they stay reachable in maintenance mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 201-243: The embedding example currently implies only http, bytes,
and serde_json are needed, but the hook implementation also uses async-trait via
#[async_trait::async_trait]. Update the README section around the embedding
hooks and the AuthDecider example to explicitly mention async-trait as a
required dependency for embedders, or otherwise note that the macro crate must
be added alongside the existing foundational types. Keep the guidance aligned
with the hook traits and ProxyServer::from_config example so readers can locate
the dependency requirement quickly.
In `@src/embed.rs`:
- Around line 253-259: The bearer_token helper only accepts "Bearer " and
"bearer ", so it still rejects other casing like "BEARER token". Update
bearer_token to match the authorization scheme case-insensitively before
trimming the token, while keeping the existing empty-token check and returning
the token from the same helper.
- Around line 153-161: The UserInfo 401 response handling in this branch
currently returns only the JSON body, so update the `backend.userinfo` match in
`src/embed.rs` to add a `WWW-Authenticate` header on every unauthorized
response. Use a plain `Bearer` challenge when `bearer_token(&headers)` yields no
credential, and use `Bearer error="invalid_token"` when
`backend.userinfo(&token)` returns `None`, while keeping the existing
`deny_response` path and JSON error payloads.
- Around line 191-193: The extra-route body buffering in handle() is currently
swallowing failures with unwrap_or_default(), which can pass an empty payload to
the handler. Update the body read path around axum::body::to_bytes in
src/embed.rs to return an error response immediately when buffering fails or
exceeds MAX_EXTRA_ROUTE_BODY, and only continue to the route handler when the
body is successfully read.
---
Outside diff comments:
In `@src/lib.rs`:
- Around line 252-324: The maintenance exemption list is still based on the
static defaults, so custom health, metrics, and verify endpoints built in the
router can be blocked during maintenance. Update the router-state setup around
the health/metrics routes and the decider verify path so maintenance_exempt is
derived from the resolved configured paths (health.path, health.live_path,
health.ready_path, health.startup_path, metrics.path, and the configured verify
path) instead of hardcoded defaults. Add a regression test covering relocated
probe and verify paths to confirm they stay reachable in maintenance mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 47f72a28-572b-4e0e-954e-00cfcb5148f5
📒 Files selected for processing (10)
Cargo.tomlREADME.mdsrc/config.rssrc/embed.rssrc/embed/tests.rssrc/hooks.rssrc/lib.rssrc/shield/store.rstests/embedded.rstests/hooks.rs
|
| Filename | Overview |
|---|---|
| src/embed.rs | Adds the axum bridge for embedded hooks and rejects oversized or unreadable extra-route bodies before invoking handlers. |
| src/lib.rs | Wires embedded hooks into the router, validates mounted route conflicts, and exempts configured probe and verify paths during maintenance. |
| src/config.rs | Adds health and metrics endpoint configuration with defaults and path validation. |
| src/transcode/mod.rs | Shares route binding data between transcoded route registration and collision detection. |
Reviews (10): Last reviewed commit: "fix: normalize route shape for collision..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
441-458: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the verify-path collision guard to config-driven forward-auth too.
This precheck only runs in the injected-decider branch. A deployment that uses plain JWT forward-auth still merges
forward_auth.routes()without it, soauth.forward_auth.pathcolliding withhealth.*ormetrics.pathwill still hit Axum’s duplicate-route panic instead of returning the clean error this PR adds.🛠️ Proposed fix
let forward_auth = auth.as_ref().and_then(|built| { auth::forward::ForwardAuth::build(self.config.auth.as_ref()?, built.clone()) }); + + if (self.auth_decider.is_some() || forward_auth.is_some()) + && self.builtin_get_paths().iter().any(|p| p == &verify_path) + { + anyhow::bail!( + "verify path {verify_path:?} collides with a health/metrics endpoint path" + ); + } // Auth runs inside Shield (added first = inner): rate limiting sheds // load before any signature verification work. if let Some(auth) = auth { router = router.layer(axum::middleware::from_fn_with_state(auth, auth::middleware)); @@ // Forward-auth `/verify` endpoint. An injected AuthDecider owns it when // present (in-process PDP); otherwise the config-driven JWT ForwardAuth // backs it. Mounted after the auth layer so it is not itself JWT-gated. if let Some(decider) = &self.auth_decider { - // Guard against the verify path colliding with a built-in GET route - // (axum panics on duplicate path registration); fail with a clear - // error instead. `verify_path` was resolved once above. - if self.builtin_get_paths().iter().any(|p| p == &verify_path) { - anyhow::bail!( - "verify path {verify_path:?} collides with a health/metrics endpoint path" - ); - } let decider = decider.clone(); router = router.route( &verify_path, axum::routing::any(move |req: axum::extract::Request| { let decider = decider.clone();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 441 - 458, The verify-path collision check is only applied in the injected-decider branch, so plain JWT forward-auth can still merge `forward_auth.routes()` with a conflicting `auth.forward_auth.path`. Update the routing setup in `src/lib.rs` so the same `builtin_get_paths()` vs `verify_path` precheck runs before both the decider route registration and the `forward_auth.routes()` merge, returning the same clear `anyhow::bail!` error instead of letting Axum panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/embed.rs`:
- Around line 153-170: Short-circuit missing Bearer credentials in the
`/userinfo` flow before calling `backend.userinfo`. In the `bearer_token`
handling inside `src/embed.rs`, avoid passing `token.as_deref().unwrap_or("")`
to `userinfo`; instead return the same unauthorized challenge path when
`bearer_token(&headers)` yields `None`, and only invoke `backend.userinfo(...)`
when a real token is present. Keep the existing `invalid_token` vs plain
`Bearer` challenge behavior in the match around `userinfo`.
---
Outside diff comments:
In `@src/lib.rs`:
- Around line 441-458: The verify-path collision check is only applied in the
injected-decider branch, so plain JWT forward-auth can still merge
`forward_auth.routes()` with a conflicting `auth.forward_auth.path`. Update the
routing setup in `src/lib.rs` so the same `builtin_get_paths()` vs `verify_path`
precheck runs before both the decider route registration and the
`forward_auth.routes()` merge, returning the same clear `anyhow::bail!` error
instead of letting Axum panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 90cbdb7f-1485-4c2a-8b8b-1b3e063eed24
📒 Files selected for processing (6)
README.mdsrc/config.rssrc/embed.rssrc/embed/tests.rssrc/lib.rstests/hooks.rs
Add axum-free Tier-2 embedding hooks so a service gets the full stateless REST/OIDC/forward-auth edge by adding the crate, with no axum in its own Cargo.toml: - AuthDecider: in-process forward-auth/PDP gate, runs inline on proxied routes and backs /verify (path set via with_verify_path) - OidcBackend: stateless discovery/JWKS/userinfo, paths consumer-supplied - with_extra_routes: framework-agnostic route adapter (request/response parts) - Make health and metrics endpoint paths configurable and disablable - Fix RedisStore intra-doc link in no-redis builds; docs.rs builds with the redis feature so the type is documented Hook contracts use only http/bytes/serde_json, so AuthDecider adds zero new dependencies. Standalone binary behavior is unchanged. BREAKING CHANGE: ProxyConfig gains `health` and `metrics` fields; code that builds ProxyConfig with a struct literal must add them. YAML configs are unaffected (serde defaults fill them in). Closes #66
…scheme - Reject oversized/unreadable extra-route bodies with 413 instead of passing an empty payload to the handler (a body-parsing handler would misread it) - Add RFC 6750 WWW-Authenticate Bearer challenge to userinfo 401 responses (plain Bearer when no credentials, error="invalid_token" when rejected) - Match the Authorization Bearer scheme case-insensitively
…sion-free - Reject duplicate built-in endpoint paths at config load (e.g. health.path equal to the default live_path) instead of panicking on duplicate route registration - Derive the maintenance exemption list from the resolved configured health, metrics, and verify paths so relocating them does not 503 probe/forward-auth traffic that was exempt at the default paths - Fail with a clear error when the verify path collides with a built-in GET endpoint path
The hook traits are #[async_trait], so embedders need async-trait alongside http/bytes/serde_json. None is an HTTP framework.
…llision - /userinfo now returns the 401 Bearer challenge without invoking the OIDC backend when no credentials are present (never call it with an empty token) - Apply the verify-path / built-in-GET collision guard before both the injected decider route and the config-driven JWT forward-auth merge, so plain forward-auth also returns a clean error instead of an axum duplicate-route panic
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config.rs`:
- Around line 646-662: validate_edge_paths() only checks for duplicate built-in
endpoint paths, but Router::route expects valid route shapes. Update
validate_edge_paths in src/config.rs to also reject invalid health and metrics
paths before they reach Router::route, using the existing path fields on
self.health and self.metrics so bad config fails fast at validation time instead
of during startup routing.
In `@src/hooks.rs`:
- Around line 116-119: The `Hooks::userinfo` documentation still says `bearer`
may be an empty string, but `src/embed.rs` now bypasses the hook when
credentials are missing. Update the `userinfo` contract comment in `Hooks` to
state that it is only called with a present bearer token after the `Bearer `
prefix is stripped, and remove the empty-string/missing-credentials wording so
the doc matches the actual `userinfo` call path.
In `@src/lib.rs`:
- Around line 298-306: The middleware layering in the route setup is reversed:
auth_decider_gate is currently applied after auth::authz::middleware, so it
executes before authz instead of after it. Update the layering in src/lib.rs
where transcode_routes is built so the AuthDecider middleware is inserted before
the authz layer, preserving the intended authz -> AuthDecider -> handler order;
use the existing auth_decider_gate and auth::authz::middleware symbols to adjust
the wrapping sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a48eb3b2-a81d-4752-965d-3e02cea645a6
📒 Files selected for processing (10)
Cargo.tomlREADME.mdsrc/config.rssrc/embed.rssrc/embed/tests.rssrc/hooks.rssrc/lib.rssrc/shield/store.rstests/embedded.rstests/hooks.rs
…cider - builtin_get_paths now includes the enabled OpenAPI spec/docs paths, so a verify path colliding with /openapi.json or /docs is rejected with a clear error instead of an axum duplicate-route panic - validate_edge_paths rejects paths without a leading '/' (axum would reject the route shape at construction otherwise) - order the proxied-route gates authz -> AuthDecider -> handler (the decider layer is added inner, authz outer), so ext_authz runs first and the decider sees headers it injected; the previous order was inverted - correct the OidcBackend::userinfo doc: it only ever receives a present, non-empty bearer token (missing credentials are rejected before the call)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib.rs (2)
443-467: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
verify_pathbefore route registration.with_verify_path()and theauth.forward_auth.pathfallback can both supply a path without a leading/; when auth/forward-auth is enabled, that reachesrouter.route(...)and can panic instead of returningErr.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 443 - 467, Validate verify_path before calling router.route in the routing setup inside src/lib.rs, because with_verify_path() and the auth.forward_auth.path fallback can yield a path without a leading slash and cause a panic. Add normalization/validation right after the verify_path is chosen and before the collision check and route registration, so invalid paths are rejected with an error instead of reaching the AuthDecider/forward-auth verify endpoint mount.
203-222: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftInclude all pre-mounted routes in the verify collision check.
builtin_get_paths()only covers health, metrics, and OpenAPI, butoidc_routes,embed::extra_routes_router(...), andtranscode_routesare merged beforeverifyis registered. A custom/userinfo, extra route, or transcoded GET path can still collide and panic at router construction; guard the full reserved path set instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 203 - 222, The verify collision guard is incomplete because builtin_get_paths only returns health, metrics, and OpenAPI paths, while oidc_routes, embed::extra_routes_router(...), and transcode_routes are mounted before verify and can still conflict. Update the reserved-path collection used by the verify registration check to include every pre-mounted GET route from those route builders, and ensure the guard in builtin_get_paths (or its caller) reflects the full set of paths that can be registered before verify.src/config.rs (1)
647-666: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate OpenAPI paths with the other built-in edge paths.
openapi.pathandopenapi.docs_pathare also mounted, so a collision with health/metrics (or between the two OpenAPI routes) can still panic when the router is assembled.Proposed fix
if self.metrics.enabled { check("metrics.path", &self.metrics.path)?; } + if let Some(openapi) = self.openapi.as_ref().filter(|o| o.enabled) { + check("openapi.path", &openapi.path)?; + check("openapi.docs_path", &openapi.docs_path)?; + } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config.rs` around lines 647 - 666, The built-in path validation in validate_edge_paths currently checks only health and metrics, so openapi.path and openapi.docs_path can still collide with those routes or with each other and later panic during router setup. Extend the existing HashSet-based duplicate/path-prefix checks in validate_edge_paths to include both OpenAPI routes when openapi is enabled, using the same check helper and distinct labels so all built-in edge paths are validated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/config.rs`:
- Around line 647-666: The built-in path validation in validate_edge_paths
currently checks only health and metrics, so openapi.path and openapi.docs_path
can still collide with those routes or with each other and later panic during
router setup. Extend the existing HashSet-based duplicate/path-prefix checks in
validate_edge_paths to include both OpenAPI routes when openapi is enabled,
using the same check helper and distinct labels so all built-in edge paths are
validated consistently.
In `@src/lib.rs`:
- Around line 443-467: Validate verify_path before calling router.route in the
routing setup inside src/lib.rs, because with_verify_path() and the
auth.forward_auth.path fallback can yield a path without a leading slash and
cause a panic. Add normalization/validation right after the verify_path is
chosen and before the collision check and route registration, so invalid paths
are rejected with an error instead of reaching the AuthDecider/forward-auth
verify endpoint mount.
- Around line 203-222: The verify collision guard is incomplete because
builtin_get_paths only returns health, metrics, and OpenAPI paths, while
oidc_routes, embed::extra_routes_router(...), and transcode_routes are mounted
before verify and can still conflict. Update the reserved-path collection used
by the verify registration check to include every pre-mounted GET route from
those route builders, and ensure the guard in builtin_get_paths (or its caller)
reflects the full set of paths that can be registered before verify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc5a97b5-f553-42e2-95b8-1e968c27c04f
📒 Files selected for processing (4)
src/config.rssrc/hooks.rssrc/lib.rstests/hooks.rs
|
T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings |
- reserved_get_paths now collects every route mounted before verify: health, metrics, OpenAPI, the OIDC surface (injected backend or config-driven static discovery), embedder extra routes, and the transcoded REST routes (transcode::route_paths). A verify path colliding with any of them is now a clear error instead of an axum duplicate-route panic - reject a verify path without a leading '/' before route registration - validate_edge_paths also checks OpenAPI spec/docs paths for shape and duplicates
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
203-241: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the reserved-path check to every mounted edge route. The helper already collects health, metrics, OpenAPI, OIDC, extra-route, and transcode paths, but it is only used for
/verify. Any collision among mounted routes can still reach Axum and abort startup with a duplicate-route panic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 203 - 241, The reserved-path guard in reserved_get_paths currently covers all mounted edge route categories, but the check is only being applied for /verify, so other mounted routes can still collide and trigger an Axum duplicate-route panic at startup. Update the route registration flow in lib.rs to run this reserved-path validation for every mounted edge route (not just verify), reusing reserved_get_paths along the mounting path so any conflicting path is rejected with the existing clear error before registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib.rs`:
- Around line 203-241: The reserved-path guard in reserved_get_paths currently
covers all mounted edge route categories, but the check is only being applied
for /verify, so other mounted routes can still collide and trigger an Axum
duplicate-route panic at startup. Update the route registration flow in lib.rs
to run this reserved-path validation for every mounted edge route (not just
verify), reusing reserved_get_paths along the mounting path so any conflicting
path is rejected with the existing clear error before registration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 55df3552-868b-4e16-b326-b6a9498d1787
📒 Files selected for processing (6)
src/config.rssrc/hooks.rssrc/lib.rssrc/oidc/mod.rssrc/transcode/mod.rstests/hooks.rs
Run the reserved-path duplicate scan across the whole mounted edge (built-in routes, OIDC surface, extra routes, transcoded paths, and the verify endpoint) up front, before any router is constructed. Any collision now returns a clear error instead of reaching axum and panicking at .route/.merge during startup.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib.rs`:
- Around line 261-277: The mounted verify path and the path used for
validation/exemptions are diverging in the JWT forward-auth flow, so collisions
and maintenance exemptions can apply to the wrong route. Update the logic around
self.resolved_verify_path(), ForwardAuth::build, and forward_auth.routes() so
the same path that is actually mounted is also the one added to mounted and used
for maintenance exemptions; keep the decider-driven route using the override,
but for config-driven forward auth validate/exempt auth.forward_auth.path unless
the override is explicitly threaded through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6474cf96-98b6-42f6-b0af-1c166c091965
📒 Files selected for processing (2)
src/lib.rstests/hooks.rs
The with_verify_path override applies only to an injected AuthDecider's verify route; config-driven JWT forward-auth always mounts at auth.forward_auth.path. Introduce mounted_verify_path() that returns the branch-correct path (decider → override/config/default; config forward-auth → forward_auth.path, only when mode is jwt), and use it for both the duplicate-route guard and the maintenance-exempt list. Previously a with_verify_path override hid a config forward_auth.path collision and exempted the wrong path under maintenance.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib.rs (1)
294-306: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate every mounted path before router construction.
reserved_get_paths()includes OIDC and extra-route paths, and those are later handed toRouter::route(), which panics on paths that don’t start with/. The current guard only checksverify_path, so a malformed backend or extra-route path can still turn startup into a panic instead of a build error.Proposed fix
if let Some(vp) = &verify_path { if !vp.starts_with('/') { anyhow::bail!("verify path {vp:?} must start with '/'"); } mounted.push(vp.clone()); } + for path in &mounted { + if !path.starts_with('/') { + anyhow::bail!("route path {path:?} must start with '/'"); + } + } let mut seen = std::collections::HashSet::with_capacity(mounted.len());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib.rs` around lines 294 - 306, The startup path validation only checks verify_path, but mounted also includes values from reserved_get_paths() that later go into Router::route(). Add a pre-router validation pass over every entry in mounted to ensure each path starts with '/' and return an anyhow::bail! build error if any do not. Keep the existing duplicate-path check, and make the fix in the mounting logic around reserved_get_paths(), mounted, and Router::route() so malformed backend or extra-route paths can’t cause a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/hooks.rs`:
- Around line 420-433: The YAML snippet in `ProxyConfig::from_yaml_str` is
embedding `pem_path.display()` directly into a double-quoted scalar, which can
break on Windows because backslashes are treated as escapes. Update the test to
pass an escaped/serialized path string when interpolating the
`public_key_pem_file` value so the generated YAML remains valid across
platforms.
---
Outside diff comments:
In `@src/lib.rs`:
- Around line 294-306: The startup path validation only checks verify_path, but
mounted also includes values from reserved_get_paths() that later go into
Router::route(). Add a pre-router validation pass over every entry in mounted to
ensure each path starts with '/' and return an anyhow::bail! build error if any
do not. Keep the existing duplicate-path check, and make the fix in the mounting
logic around reserved_get_paths(), mounted, and Router::route() so malformed
backend or extra-route paths can’t cause a panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6368cf48-ba68-4c4b-9d0f-b0b1b0ea912d
📒 Files selected for processing (2)
src/lib.rstests/hooks.rs
reserved_get_paths() includes consumer-supplied OIDC-backend and extra-route paths that are handed to Router::route(), which panics on a path without a leading '/'. Validate the leading '/' for every mounted path (not only the verify path) before building the router, so a malformed backend/extra-route path is a clean build error. Also embed the temp PEM path in test YAML with forward slashes so the config parses on Windows runners.
Same-path routes with different methods (e.g. an embedder registering GET /c and POST /c through the extra-route adapter, which merges them by method) are a legal shape. The collision guard now keys on (method, path): only a repeated (method, path) is a conflict. The verify endpoint answers all methods, so it is tracked as '*' and conflicts with any route on its path. route_paths and reserved_routes now carry the HTTP method for each route.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib.rs`:
- Around line 229-276: `reserved_routes` is only comparing raw `(method, path)`
strings, so dynamic routes with the same shape can still collide at router build
time. Update the guard in `reserved_routes` (and any helper it uses) to
normalize each route’s shape before deduping/comparing, so equivalent templated
paths like different parameter names are treated as the same conflict. Keep the
existing method/path pairing logic, but base the collision check on normalized
route structure rather than literal path text.
In `@src/transcode/mod.rs`:
- Around line 195-226: `route_paths` is duplicating the route/alias iteration
from `routes`, which can drift and break the pre-mount collision guard. Refactor
the shared unary/alias/streaming enumeration into one internal helper (for
example, a builder or iterator used by `routes` and `route_paths`) and have both
functions consume it so the `(method, path)` list always matches what `routes`
mounts. Use the existing `route_paths`, `routes`, `extract_routes`, and
`extract_streaming_routes` symbols to keep the logic centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a69e9ce-ccae-4e1e-b369-a7e61405d0ca
📒 Files selected for processing (3)
src/lib.rssrc/transcode/mod.rstests/hooks.rs
…eration
- The pre-mount collision guard now keys on normalized route shape, so two
structurally identical dynamic routes that differ only in parameter name
(e.g. /v1/x/{a} and /v1/x/{b}) collide as axum/matchit would, instead of
slipping past and panicking at router construction.
- transcode: extract route_bindings() as the single source of truth for the
routes routes() mounts (unary + aliases + streaming). Both routes() and
route_paths() consume it, so the mounted set and the enumerated set used by
the collision guard cannot drift apart. This also fixes a latent case where
a streaming-only service registered no routes.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
1 participant
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.
Summary
Adds the axum-free Tier-2 embedding hooks so a service gets the full
stateless REST/OIDC/forward-auth edge by adding the crate, with no
axuminits own
Cargo.toml(cargo tree -i axumshows axum only understructured-proxy). Also makes the previously hardcoded edge routesconfigurable. The default build stays a stateless data plane (README Non-goals
hold).
Hooks (
structured_proxy::hooks, all axum-free)AuthDecider— in-process forward-auth / PDP decision. Runs inline onevery proxied request and backs
/verify(path viawith_verify_path).Contract uses only
http/bytes(zero new deps);Allowinjects headers(client-forged copies stripped),
Denyshort-circuits,Redirect302 inline/ 401+
Locationon/verify.OidcBackend— stateless discovery / JWKS / userinfo; every path isconsumer-supplied; supersedes config-driven static discovery.
with_extra_routes— framework-agnostic route adapter (request parts in,response parts out); merges methods on a shared path.
Configurable edge routes
health(path / live / ready / startup) andmetrics(path) are nowconfigurable and disablable via YAML; defaults unchanged.
Fixes (in scope)
RedisStoreintra-doc link no longer warns in no-redisbuilds(
cfg_attr-gated); docs.rs builds with theredisfeature so the type isdocumented.
Breaking change
feat!—ProxyConfiggainshealthandmetricsfields. Code that buildsProxyConfigwith a struct literal must add them. YAML configs areunaffected (serde defaults). Title carries
!so squash-merge yields a MAJORbump.
Testing
cargo nextest run --workspace: 160 passed (+28), incl. negative / edge /security paths (forged-header strip, malformed location, unsupported method
skip, no-forwarding-header fallback, disabled/relocated routes).
cargo clippy --all-targets -- -D warningsclean (default andredis).cargo doc -D warningsclean (default andredis); doc-tests pass;aws_lc_rsbackend builds.Follow-ups (separate issues)
bfffeature: stateful BFF + OIDC authorization server, default-off, composesvia
with_extra_routes.Closes #66