Allow ESI shared-template caching behind basic auth - #1070
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
The edge-auth marker is carried consistently through all four adapters, and the focused cache and authentication tests pass. I found two medium-risk follow-ups around preserving the marker's meaning and documenting the new cache exception. The remaining comments are cleanup nitpicks for the repeated and overly long explanations.
Operator documentation
docs/guide/configuration.md:1612 still says authorization bypasses the template cache unconditionally. It should distinguish pass-through and repeated values, which still bypass, from one edge-validated Basic credential, which may now share a template. Please also document that the credential remains forwarded and that an origin using it must declare Vary: Authorization.
aram356
left a comment
There was a problem hiding this comment.
Summary
Allows ESI shared-template caching when Trusted Server itself terminated the request's Basic Auth credential at the edge. The design is sound: the EdgeTerminatedAuthorization marker is unforgeable outside auth.rs (private unit field, cleared on entry, inserted only on the validated path), the publisher gate independently disqualifies repeated Authorization values even when a marker is present, the template-cache key's raw path_and_query is the same raw path the handler regex gated (so every reader of a gated key passed the same first-match handler and credential), and template_fingerprint hashing the complete typed settings means a credential rotation or gate removal rotates every key and orphans gated-era templates. Regression coverage on both sides of the seam is thorough.
1 of the inline comments below carries a one-click GitHub
suggestion(use Commit suggestion to apply it as a commit on the PR branch). The remaining comments describe the fix in prose because the change touches lines outside the diff or other files.
Non-blocking
♻️ refactor
request_had_authorizationno longer means what its name says - see inline atcrates/trusted-server-core/src/publisher.rs:4354
🤔 thinking
- The
Vary: Authorizationrefusal claim is conditional on operator config - see inline atcrates/trusted-server-core/src/auth.rs:76-81
⛏ nitpick
- The
handlerscoping block is unnecessary and its comment states a borrow constraint that does not exist - see inline atcrates/trusted-server-core/src/auth.rs:97-110(suggestion)
Cross-cutting / body-level findings
-
🌱 The middleware → router → publisher marker seam is untested end-to-end -
auth.rstests prove the marker is inserted, and the publisher tests inject it withfor_test(). The seam between them, extensions survivingctx.into_request()through each adapter's dispatch path, is exactly the contract the design depends on, and no test exercises it. One adapter dispatch-path test (the fastlyapp.rsfull-dispatch test module or the parity suite already have the scaffolding) asserting that a gated ESI request stores on the cold pass and hits on the warm pass would lock the contract against a future edgezero change that rebuilds the request and drops extensions. -
📌 Path-regex basic auth is alias-sensitive, and this PR's premise leans on the gate - pre-existing, not introduced here: handler regexes match the raw path (
^/securedoes not match/%73ecure/x) while most origins decode, so an unauthenticated request can fetch gated origin content through the proxy under a percent-encoded alias. The shared-template keys also use the raw path, so no stored gated template is readable cross-key and the caching addition itself is safe, but the doc's "every reader able to look up a template has authenticated" premise is only as strong as the gate. Worth a docs note that whole-site staging gates should use an alias-proof pattern (^/), which is the intended use case anyway.
CI Status
- cargo fmt: PASS (required)
- cargo test: PASS (required)
- format-docs: PASS (required)
- format-typescript: PASS (required)
- cargo test (axum native): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- vitest: PASS
- browser integration tests: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
|
Addressed the body-level review findings as well:
Verification completed: Fastly, Axum, Cloudflare, and Spin test suites; integration parity; formatting; and all adapter clippy aliases. |
717dccb to
265df48
Compare
265df48 to
0c47533
Compare
Summary
Changes
Test plan
Checklist
No linked issue for this PR, per request.