Skip to content

fix: deny the page cache on the header alone - #5

Merged
Decipher merged 2 commits into
1.0.xfrom
feature/1-page-cache-path-prefix
Aug 26, 2026
Merged

fix: deny the page cache on the header alone#5
Decipher merged 2 commits into
1.0.xfrom
feature/1-page-cache-path-prefix

Conversation

@Decipher

@Decipher Decipher commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Summary

The page cache request policy compared the request path to the JSON:API base path with ===, so any path prefix defeated it. Behind a language prefix, the internal page cache stored a consumer-specific response under a URL every consumer shares, and served it to requests that name no consumer.

A first pass switched to a suffix match, and review found that was still not enough: a trailing slash, upper case and percent-encoding all leak too, because the router lowercases, decodes, trims slashes and follows aliases before it matches, while a request policy runs before routing and only sees the raw path. There is no reliable way to name "the settings resource" at that point, and each miss is a cached response served to the wrong consumer.

Changes

  • src/PageCache/DenyOnConsumerHeader.php: the header is the whole test. A request carrying X-Consumer-ID never uses the internal page cache, on any path. It keeps the dynamic page cache, which varies on the header correctly. This is the shape the policy had before the round-2 review narrowed it to the settings path.
  • decoupled_settings.services.yml: the base path argument goes, the policy no longer needs it.
  • tests/src/Unit/DenyOnConsumerHeaderTest.php: a data provider over every spelling the router accepts (prefix, subdirectory, trailing slash, upper case, percent-encoded, repeated slashes, alias), plus the no-header and other-path cases.
  • tests/src/Functional/JsonApiSettingsResourceLanguagePrefixTest.php: new class with language installed. For each spelling, requests the endpoint as a header-identified consumer, then anonymously, and asserts the anonymous response carries the global values and "consumer": null. The percent-encoded spelling is unit-only, the test client re-encodes the percent sign.

Verified live on every spelling: all now answer X-Drupal-Cache: UNCACHEABLE (request policy).

Summary by CodeRabbit

  • Bug Fixes

    • Requests containing a consumer identification header are now consistently excluded from shared page caching, regardless of endpoint path or URL formatting.
    • Prevents consumer-specific responses from being reused for anonymous requests.
  • Tests

    • Added coverage for language prefixes, trailing slashes, uppercase paths, aliases, encoded paths, and persistent cache behavior.

@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.86%. Comparing base (5465239) to head (497d155).

Additional details and impacted files
@@            Coverage Diff             @@
##            1.0.x       #5      +/-   ##
==========================================
+ Coverage   85.80%   85.86%   +0.06%     
==========================================
  Files           9        9              
  Lines         486      481       -5     
==========================================
- Hits          417      413       -4     
+ Misses         69       68       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Decipher

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5f1200fa-1296-4d71-aced-752fd9d8b598

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: db09ccd1-d4c5-42d1-8633-e5072c9d9bdd

📥 Commits

Reviewing files that changed from the base of the PR and between d2478c0 and 497d155.

📒 Files selected for processing (4)
  • decoupled_settings.services.yml
  • src/PageCache/DenyOnConsumerHeader.php
  • tests/src/Functional/JsonApiSettingsResourceLanguagePrefixTest.php
  • tests/src/Unit/DenyOnConsumerHeaderTest.php
💤 Files with no reviewable changes (1)
  • decoupled_settings.services.yml

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

DenyOnConsumerHeader now denies every request with X-Consumer-ID, independent of path. Service wiring and tests cover path variants and cache separation for identified and anonymous requests.

Changes

Consumer cache policy

Layer / File(s) Summary
Global deny policy and unit coverage
src/PageCache/DenyOnConsumerHeader.php, decoupled_settings.services.yml, tests/src/Unit/DenyOnConsumerHeaderTest.php
The policy no longer uses a JSON:API base path. It denies all requests with X-Consumer-ID. Unit tests cover normalized, prefixed, encoded, aliased, slash-variant, and non-settings paths.
Language-prefixed cache isolation
tests/src/Functional/JsonApiSettingsResourceLanguagePrefixTest.php
The functional test configures French language support and verifies separate consumer-specific and anonymous responses across accepted endpoint spellings.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 497d1

The change prevents header-identified requests from using the internal page cache while preserving correctly varying dynamic caching; no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant DenyOnConsumerHeader
  participant SettingsEndpoint
  participant ServerCache
  Client->>DenyOnConsumerHeader: Request settings path with X-Consumer-ID
  DenyOnConsumerHeader-->>Client: DENY page-cache reuse
  Client->>SettingsEndpoint: Fetch consumer-specific settings
  SettingsEndpoint->>ServerCache: Store response without shared reuse
  Client->>SettingsEndpoint: Fetch without X-Consumer-ID
  SettingsEndpoint->>ServerCache: Read anonymous response
  ServerCache-->>Client: Return anonymous settings
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely states the main change: page-cache denial is based on the consumer header alone.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/1-page-cache-path-prefix

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Decipher Decipher changed the title fix: deny the page cache on every path that ends in the settings endpoint fix: deny the page cache on the header alone Aug 26, 2026
@Decipher

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Decipher

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Decipher

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@Decipher
Decipher merged commit 9dda19d into 1.0.x Aug 26, 2026
11 checks passed
Decipher added a commit that referenced this pull request Aug 26, 2026
@Decipher
Decipher deleted the feature/1-page-cache-path-prefix branch August 26, 2026 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant