feat(app-router): admit rendered App pages only with a negative searchParams proof - #3461
james-elicx wants to merge 8 commits into
Conversation
|
/bigbonk review for issues Please review exact head |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
LGTM! |
42faaa1 to
421ed11
Compare
|
/bigbonk review for issues Please review exact head |
|
|
LGTM! |
421ed11 to
d122a5c
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
d122a5c to
14e43e7
Compare
|
/bigbonk review for issues Please review exact head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| if (completed) return; | ||
| completed = true; | ||
|
|
||
| const observationState = options.consumeRenderObservationState?.(); |
There was a problem hiding this comment.
The observation consumer is now security-significant, but it runs from deferUntilStreamConsumed's stream pull/flush context. renderAppPageLifecycleImpl explicitly binds the analogous dynamic-usage reader because that context is no longer the render's unified request scope; this raw callback can therefore consume the fallback/current state and return an empty requestApis, incorrectly producing searchParamsUnread: true. Bind consumeRenderObservationState to the originating request context before passing it into deferred finalization (and add an integration test using the real request context rather than a closure returning a literal observation).
There was a problem hiding this comment.
Fixed in 7a3f1a9. The observation consumer is now bound to the render's request context, like the dynamic-usage reader. The new test uses the real request context and cancels the probe's body outside it; it fails without the fix.
|
Posted 1 actionable inline finding. Focused tests pass: 85/85. |
14e43e7 to
7a3f1a9
Compare
|
/bigbonk review for issues Please review exact head |
| // in Next.js. | ||
| if ( | ||
| state.route.kind === "app-page" && | ||
| outcome === rendererOutcome && |
There was a problem hiding this comment.
Require an actual cache-policy replacement before skipping the proof. recordResponseStageCachePolicy sets explicitConfigCachePolicy for any nonempty transported policy, while resolveResponseStageCachePolicy can return only Vary. In that case inferFinalAppPageCacheability creates a fresh outcome with the renderer's unchanged cache-control, so outcome !== rendererOutcome and an otherwise cacheable App render lacking searchParamsUnread is admitted (notably with a verbatim-Vary adapter). This defeats the fail-closed missing-observation case. Please base the exemption on CDN cache-policy provenance/value rather than object identity, or only mark the config policy explicit when it contains a CDN response-policy header.
There was a problem hiding this comment.
Fixed in e97ed44. The exemption now needs the outcome's CDN cache-control to differ from the renderer's snapshot. A Vary-only config policy therefore still needs the renderer's searchParamsUnread proof. I kept explicitConfigCachePolicy as it is: it comes from main and also drives Route Handler admission.
|
Posted 1 actionable inline finding. Focused tests pass: 180/180. |
7a3f1a9 to
e97ed44
Compare
|
/bigbonk review for issues Please review exact head |
| // rule) replaces nothing. | ||
| const replacesRendererPolicy = | ||
| outcome !== rendererOutcome && | ||
| readCdnResponseCacheControl(response.headers) !== |
There was a problem hiding this comment.
[P2] Track config cache-policy provenance instead of comparing values. If the renderer snapshots s-maxage=60 and an explicit matching next.config rule also sets Cache-Control: s-maxage=60, inferFinalAppPageCacheability correctly creates the config-owned outcome, but this equality makes replacesRendererPolicy false and rejects a render without searchParamsUnread. That response is safe to admit because the config policy is cached per request URL, and the PR explicitly exempts such policies; this currently turns a valid configured cache hit into no-store. Please distinguish policy headers from Vary-only transport when recording config provenance, rather than requiring the final value to differ.
There was a problem hiding this comment.
Fixed in 26c0a7d: the exemption now comes from provenance. A config rule that transports a CDN cache-policy header is recorded as owning the policy, even when the value matches the renderer's. A Vary-only transport still requires the proof. There are tests for a matching s-maxage=60 rule and for Vary-only.
|
Posted 1 actionable inline finding. Focused tests pass: 185/185. |
85d2804 to
606a4a8
Compare
606a4a8 to
de1cee4
Compare
de1cee4 to
c202a8c
Compare
3b7bc7c to
2f6de9e
Compare
2f6de9e to
12f343f
Compare
…hParams proof Response-stage caches will key App pages without the query, so completed- response admission now refuses the renderer's policy unless the render's observation proves searchParams went unread. The finalizer used to discard that observation; the outcome now carries the proof. The check runs after the static-to-dynamic 500, and a later next.config policy, which replaces the renderer's and is cached per URL, doesn't need it.
…text A disconnecting client's cancel finishes the cacheability probe outside the render's request scope, where the observation consumer read no request APIs and proved searchParams unread.
…renderer's policy A response-stage config policy marks the config policy explicit even when it holds only Vary, so admission built a fresh outcome from the renderer's own cache-control and skipped the searchParams proof by object identity. The exemption now requires the final CDN cache-control to differ from the framework's, and otherwise checks the renderer's proof.
…roof by its provenance Admission compared the final cache-control with the renderer's to tell a replacing next.config policy from a Vary-only one, so a config rule matching the renderer's value was rejected without a searchParams proof. The config recording now notes whether the policy set a cache policy header, and that exempts the render.
…stage path Single-stage App renders apply next.config headers through applyConfigHeadersToResponse, which marked only explicitConfigCachePolicy. A config rule matching the renderer's cache-control was then rejected without a searchParams proof. The config marker now also records that a cache policy header was set, as the response-stage path does; a Vary-only rule still requires the proof.
…precedence Admission exempted a render from the searchParams proof whenever next.config set any cache policy header. With the Cloudflare adapter, a renderer's Cloudflare-CDN-Cache-Control outranks a config Cache-Control or CDN-Cache-Control, so the renderer's policy was admitted for every query. Config now records the policy headers it set by name and value, and the exemption applies only when the header the adapter reads carries config's value, including when it matches the renderer's.
…pter's own header name Admission found the header an adapter reads by probing it with synthetic max-age values, but an adapter that interprets opaque values falls back past them, so a config Cache-Control could be credited with a renderer-owned edge policy and skip the searchParams proof. CdnResponsePolicy gains an optional readCacheControlHeaderName that the Cloudflare adapter implements; an adapter without it leaves the policy unattributed and the proof required. Config policy values are also recorded as Headers.set applies them, so a whitespace-padded value matching the response still counts as config-owned.
…config policy A changed effective policy value is not provenance: an adapter without readCacheControlHeaderName can map a renderer-owned header to a new value, which the value-difference fallback treated as a config replacement. Grant the exemption only when the adapter-reported (or default cache-control) effective header carries the value next.config recorded for it.
12f343f to
6f63f80
Compare
1 participant
Stacked on #3460. Second half of plan PR 4 (transport contract): the admission safety net.
Problem
Response-stage caches (Response Store, Workers Cache) are going to key App pages without the query. Completed-response admission trusted the renderer's cache policy without checking that the render left
searchParamsunread. The finalizer did have that observation, but it threw it away.Change
searchParamsUnread: true, set only when the render's observation shows nosearchParamsread.await searchParamson astatic-candidatepath still gets the 500.next.configheaders, is exempt. That policy replaces the renderer's and is cached per URL, as in Next.js.Tests
tests/cacheability-admission.test.ts: finalizer to admission, end to end. A render that leftsearchParamsunread is admitted. One that read it, or has no observation, is kept private (these fail without the gate). A final config policy is still admitted without the proof.