feat(proxy): K1 path-level redirect within an ingress - #195
Merged
Merged
Conversation
A route declared on a service can answer with a redirect instead of being
served by a pod: `route("/v1/login").redirect("/api/login", 308)`. The
prefix is bound within a hostname that otherwise proxies, which neither the
vhost-level HTTP to HTTPS redirect nor the whole-hostname site-ingress
attachment could express.
A target names the parts of the request that carry over as `<tail>` and
`<query>`, translated by the runtime rather than written in the proxy's own
placeholder syntax: a target is served as a `Location`, where a braced word
would be substituted from the proxy's state, its environment among it.
Redirect routes are declared on the service and reach the emitter without a
pod binding them, so a service carrying one no longer takes the `/` fallback.
Contributor
Code Coverage OverviewLanguages: TypeScript, Rust TypeScript / code-coverage/vitestThe overall line coverage in commit a7f37d0 in the Rust / code-coverage/rustThe overall line coverage in commit a7f37d0 in the Show a line coverage summary of the most impacted files.
Updated |
![review-hero[bot]](https://avatars.githubusercontent.com/in/2896273?s=60&v=4){kind=small}
|
🦸 Review Hero Summary (round 1) Below consensus threshold (10 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
|
🦸 Review Hero Summary (round 2) Below consensus threshold (7 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
|
🦸 Review Hero Summary (round 3) Below consensus threshold (10 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
| } else { | ||
| resolved.headers.request.into_grouped().into() | ||
| }, | ||
| response: resolved.headers.response.into_grouped().into(), |
There was a problem hiding this comment.
[Bugs & Correctness] suggestion
app.describe reports a Location response operation on a redirect route that the runtime silently drops. collect_redirect_routes strips it (resolved.headers.response.without(REDIRECT_OWNED_HEADER), reconcile/proxy.rs) because a service-declared Location op would displace the computed target, but the summary passes resolved.headers.response through unfiltered for redirect routes. A service declaring headers(#{ response: #{ replace: #{ "Location": ... } } }) for its proxied routes therefore reads, in describe, as having that operation in force on its redirect routes when it does not — the same failure mode the adjacent compress / rate_limit filtering was added to avoid. Apply .without("Location") when redirect.is_some().
| /// | ||
| /// The two are defined together because they are one cut of one string. | ||
| // r[impl service.http.route.redirect] | ||
| fn request_parts_handler(prefix: &str) -> Value { |
There was a problem hiding this comment.
[Design & Architecture] critical
Two different emission designs have landed in one PR, and the code and its tests disagree. request_parts_handler implements request-part extraction as a single map handler over {http.request.uri} with an input_regexp, producing a chain of ["map", "static_response"] and a Location built from {seedling.redirect.tail} / {seedling.redirect.query}. The tests assert the other design entirely: crates/core/src/system/caddy/tests.rs:1549 expects ["rewrite", "static_response"] with handle[0].strip_path_prefix == "/v1/login" and location == "/api/login{http.request.uri}", :1595 expects a map whose source is {http.request.uri.query} with defaults[0] == "?{http.request.uri.query}", and :1619 expects {http.request.uri.path}. .workhorse/plans/k1/plan.md ("What landed") describes the rewrite+map design too, and claims http.handlers.rewrite was added to docker/caddy/required-modules.txt — it was not; only http.handlers.map is listed. So the redirect caddy tests cannot pass as committed, and whichever design is intended, the module contract and the plan notes need to match it. Pick one emission strategy and make config.rs, tests.rs, required-modules.txt and the plan agree.
| /// The header a redirect computes for itself, and which therefore takes no | ||
| /// operation from anywhere. | ||
| // r[impl service.http.route.redirect] | ||
| pub(super) const REDIRECT_OWNED_HEADER: &str = "Location"; |
There was a problem hiding this comment.
[Design & Architecture] suggestion
REDIRECT_OWNED_HEADER = "Location" duplicates LOCATION = "Location" at crates/core/src/defs/service/proxy.rs:1377, with near-identical doc comments ("the header a redirect computes for itself"). This is one wire contract — the header a redirect owns — defined twice in two layers, which is exactly the kind of split the repo rules call out. Export the single constant (alongside HeaderRules::names/without, which already live in the defs proxy module) and have the reconciler use it.
| /// `/` rather than as nothing, so the tail would never be empty. | ||
| /// | ||
| /// The two are defined together because they are one cut of one string. | ||
| // r[impl service.http.route.redirect] |
There was a problem hiding this comment.
[Performance] suggestion
The production redirect shape (redirect("/api/login"), which desugars to Literal + Tail + Query) emits a map handler with an input_regexp that is evaluated against {http.request.uri} on every request under the prefix. Adjacent <tail><query> is exactly {http.request.uri} after the prefix is stripped, so this case can be served by a single rewrite/strip_path_prefix handler with no regexp at all — which is what the plan doc claims ("the production case costs one strip-prefix handler and nothing else") and what the tests in crates/core/src/system/caddy/tests.rs assert (handlers(...) == ["rewrite", "static_response"], location == "/api/login{http.request.uri}"). As written, redirect_location always emits TAIL_VAR/QUERY_VAR and needs_request_parts always pulls in the regexp map, so the default form of the feature pays a per-request regular-expression match on the proxy hot path where a prefix-strip would do. Special-case a target ending in adjacent Tail, Query (and a Tail-only target, which maps to {http.request.uri.path}) to emit the rewrite instead, falling back to the map only when a query is named apart from the tail.
| "source": "{http.request.uri}", | ||
| "destinations": [TAIL_VAR, QUERY_VAR], | ||
| "mappings": [{ | ||
| "input_regexp": format!("^{prefix}([^?]*)(\\?.*)?$"), |
There was a problem hiding this comment.
[Security] critical
The runtime tail/query extraction places raw, client-controlled request text into a Location template without excluding the proxy's placeholder syntax. Declaration-time validation refuses { in a target precisely because a braced word in a Location is substituted from the proxy's own state (its environment among it) — but input_regexp: ^{prefix}([^?]*)(\?.*)?$ happily captures braces from {http.request.uri} into {seedling.redirect.tail} / {seedling.redirect.query}. A client can send GET /v1/login/{env.SECRET}?x={env.SECRET} (Go's HTTP server accepts braces in the request target and {http.request.uri} is the raw escaped form), so the extracted tail contains a placeholder. Whether it is expanded depends on whether Caddy's map handler runs its outputs through the replacer after regexp expansion — it documents placeholder support in outputs, so this is at least one replacer pass over attacker text, and the payoff is daemon environment disclosure in a header served to that client. Don't rely on that ordering: exclude braces from the captures so a brace-carrying request falls to defaults and contributes nothing, e.g. ^{prefix}([^?{}]*)(\?[^{}]*)?$ (same fail-safe already used for a URI that doesn't begin with the prefix).
| /// each other about. | ||
| // l[impl service.http.route.redirect] | ||
| fn declare_redirect(&mut self, redirect: RouteRedirect) -> Result<(), Box<EvalAltResult>> { | ||
| if self.prefix == "/" { |
There was a problem hiding this comment.
[Security] critical
The / guard compares the raw prefix string while the emitter normalises trailing slashes, so it is trivially bypassed. route("//") passes route()'s only validation (non-empty, starts with /), is not equal to "/", and then redirect_prefix("//") trims to "", emitting path: ["", "/*"] — a matcher that answers every request on the hostname. That is exactly the whole-hostname redirect the rule declares an operator's to make, and because the raw prefix "//" is longer than "/" it sorts ahead of the pod-bound root route and swallows it. Normalise the prefix (the same trim_end_matches('/') the emitter uses) before the == "/" comparison, or reject a prefix containing // in route() so declaration and emission agree on what a prefix means.
|
|
||
| let prefix = self.prefix.clone(); | ||
| self.http.service.with_http_def(|d| { | ||
| if d.bound_prefixes.contains(&prefix) { |
There was a problem hiding this comment.
[Security] suggestion
The redirect/binding clash check keys on the raw prefix string (bound_prefixes.contains(&prefix) and is_redirect(prefix)), but the emitter treats /v1/login and /v1/login/ as the same claim (redirect_prefix trims trailing slashes, and the proxied matcher is /v1/login*). So web.route("/v1/login/").redirect(...) alongside a pod bound at web.route("/v1/login") passes both directions of the check, then emits a redirect whose matcher covers the pod's prefix and, being the longer raw prefix, sorts ahead of it — every request intended for the pod is redirected instead. Normalise the prefix once (on route(), so redirects, routes and bound_prefixes all key on the canonical form) rather than trimming only at emission.
|
🦸 Review Hero Summary (round 4) Below consensus threshold (7 unique issues not confirmed by majority)
Local fix prompt (copy to your coding agent) |
The caddy tests still asserted the strip-prefix design the emitter had moved off, which is what CI was failing on. The map design is the one that stands: `strip_path_prefix` re-anchors an emptied path to `/`, so a request for exactly the prefix would gain a trailing slash the spec forbids, and reading the tail off the decoded path would let a `%3F` in a path segment reach a client as the `?` that starts a query. A prefix now has one spelling from `route()` onwards, and one kind. Three prefix-keyed collections whose disjointness four checks defended are one map to a `RouteKind`, and a trailing slash is trimmed at declaration rather than at emission — without which `/v1/login/` and `/v1/login` passed the either-redirected-or-proxied check as two prefixes and then claimed the same requests, and `//` slipped past the root guard onto every request of the host. The `/` fallback is restored for a service that declares a redirect: a redirect sits above it, not in place of it.
Merged
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
Review Hero