From b6525587897c4c38d4bfbc11a9862b8fa1ba9190 Mon Sep 17 00:00:00 2001 From: "David A. Wheeler" Date: Fri, 26 Jun 2026 11:24:28 -0400 Subject: [PATCH 1/3] Clarify safety in CDN misc-pages plan (Section 10) Document-only changes to the planned-future-work section for caching miscellaneous static pages: - 10.2: clarify the table names canonical locale-prefixed URLs (e.g. /en), and that the locale-less form is never the cached object (it 302-redirects uncached via redir_missing_locale). - 10.3: add a caveat that cache_static_page_on_cdn is a before_action that commits cache headers before the body, so qualifying actions must never gain an internal redirect; otherwise move to an in-action "return if performed?" guard as projects#show does. - 10.5: add a "locale-less static paths redirect uncached" test asserting each bare path 302s with private, no-store and no Surrogate-Control, locking in that the Accept-Language-dependent redirect is never cached. Co-Authored-By: Claude Opus 4.8 Signed-off-by: David A. Wheeler --- docs/cdn-cache-not-logged-in.md | 45 ++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/docs/cdn-cache-not-logged-in.md b/docs/cdn-cache-not-logged-in.md index 87b326c3f..c494d88e9 100644 --- a/docs/cdn-cache-not-logged-in.md +++ b/docs/cdn-cache-not-logged-in.md @@ -1092,9 +1092,17 @@ page depended on per-request or live data, the *existing* fragment cache would already serve stale content. So the wrapper is simultaneously the selection rule and the evidence the page is static-after-boot. The initial set: +The "Page" column names each page by its canonical, **locale-prefixed** URL +(e.g. `/en`, `/en/criteria`) — that is the object the CDN caches. The +locale-less form (bare `/`, `/cookies`, `/criteria`, …) is **never** the cached +object: it 302-redirects to the locale-prefixed URL via `redir_missing_locale` +(an early parent `before_action` that calls `disable_cache` and halts the chain +before `cache_static_page_on_cdn` can run — see Section 4.1 and the +"locale-less static paths redirect uncached" test in Section 10.5). + | Page | Controller#action | Fragment cache key today | | --- | --- | --- | -| Home (`/`) | `StaticPagesController#home` | `cache_frozen locale` | +| Home (`/en`) | `StaticPagesController#home` | `cache_frozen locale` | | Cookies policy | `StaticPagesController#cookies` | `cache_frozen locale` | | Criteria discussion | `StaticPagesController#criteria_discussion` | `cache_frozen locale` | | Criteria stats | `StaticPagesController#criteria_stats` | `cache_frozen locale` | @@ -1169,6 +1177,20 @@ flash in a `before_action` is sufficient. As with the show page, Change 1 guarantees the anonymous response carries no CSRF meta tag, so nothing writes `_BadgeApp_session`. +> **Caveat — these actions must never gain an *internal* redirect.** Unlike +> `projects#show` (which guards an internal obsolete-section 301 with +> `return if performed?`, Section 5, Change 2), `cache_static_page_on_cdn` is a +> `before_action` that commits the `Surrogate-Control` / `Cache-Control` +> headers *before* the action body runs. The qualifying actions are safe today +> because none of them redirects internally (`home`, `cookies`, +> `criteria_discussion`, `criteria_stats` have empty bodies; +> `CriteriaController#index`/`#show` only call `set_params` / +> `set_criteria_level`). If a future change makes one of these actions +> redirect (other than the locale redirect, which is handled earlier in the +> parent `before_action` chain), it would wrongly inherit cache headers — at +> that point move the guard in-action with `return if performed?`, as `show` +> does. + ### 10.4 Invalidation: boot-time purge + delayed re-purge (decided) These pages change only when a new version is deployed (new code or @@ -1240,6 +1262,27 @@ the qualifying paths (`/en`, `/en/cookies`, `/en/criteria_discussion`, (`password_resets#create`), asserting `private, no-store` on the next page. * **Kill switch** — with `CACHE_MISC_PAGES` stubbed false, the response is `private, no-store`. +* **Locale-less paths redirect uncached** — the browser-dependent locale + redirect (`redir_missing_locale`) must never be cached, so requesting each + page *without* a locale prefix must 302 to its locale-prefixed form with + `private, no-store` and **no** `Surrogate-Control`. This is the precise + invariant that keeps a browser's `Accept-Language`-chosen target from being + served to everyone (Section 4.1): + + ```ruby + # The locale redirect varies by Accept-Language and must never be cached; + # only the locale-prefixed page it lands on is cacheable. + test 'locale-less static paths redirect uncached' do + ['/', '/cookies', '/criteria_discussion', '/criteria_stats', + '/criteria'].each do |path| + get path + assert_response :found # 302, not 301 + assert_equal 'private, no-store', + response.headers['Cache-Control'], path + assert_nil response.headers['Surrogate-Control'], path + end + end + ``` The reused `PurgeCdnProjectJob` already has coverage for purging an arbitrary key; add a job assertion only if it is renamed. From 17f000fa2ba88a7718316d224d851de4b11561f4 Mon Sep 17 00:00:00 2001 From: "David A. Wheeler" Date: Fri, 26 Jun 2026 12:26:38 -0400 Subject: [PATCH 2/3] Cache "unchanging" anonymous pages on the CDN Implement the planned CDN caching of anonymous, static-after-boot pages (home, cookies, criteria discussion/stats, criteria index/show) using the same machinery as the project-show caching: conditional CSRF meta tag, cache_on_cdn / omit_session_cookie, and the existing Fastly cookie-bypass rule. No new Fastly config is required. - ApplicationController: add CACHE_UNCHANGING_PAGES kill switch (BADGEAPP_CACHE_UNCHANGING), the shared UNCHANGING_SURROGATE_KEY ('unchanging'), and the cache_unchanging_page_on_cdn guard (caches only anonymous, flash-free HTML). - StaticPagesController / CriteriaController: attach the guard as a before_action on the qualifying actions. "cookies" intentionally has no action method (it would shadow ActionController::Cookies#cookies), so the Rails/LexicallyScopedActionFilter cop is narrowly disabled there. - config/puma.rb: after_booted hook purges the shared key on boot, plus a delayed re-purge to beat the rolling-deploy race (reuses PurgeCdnProjectJob). Uses after_booted (Puma 7 deprecated on_booted). - Tests: extend CdnCachingTest with cacheability (shared surrogate key, across en+fr locales), the byte-identical invariant, logged-in bypass, carried-over-flash bypass, locale-less-redirect-uncached, and a guard that the runtime-changing Atom feed is never cached. - Name the concept "unchanging" (rendered output does not change until the next deploy) rather than "miscellaneous"; update docs Section 10 to match. Co-Authored-By: Claude Opus 4.8 Signed-off-by: David A. Wheeler --- app/controllers/application_controller.rb | 37 ++++++ app/controllers/criteria_controller.rb | 8 ++ app/controllers/static_pages_controller.rb | 17 +++ config/puma.rb | 22 ++++ docs/cdn-cache-not-logged-in.md | 66 ++++++----- test/integration/cdn_caching_test.rb | 128 ++++++++++++++++++++- 6 files changed, 246 insertions(+), 32 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 92b89925b..6e2674e29 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -175,6 +175,20 @@ def user_for_paper_trail BADGE_CACHE_SURROGATE_CONTROL = "max-age=#{BADGE_CACHE_MAX_AGE}, stale-if-error=#{BADGE_CACHE_STALE_AGE}".freeze + # Kill switch: set BADGEAPP_CACHE_UNCHANGING=false to instantly stop caching + # the "unchanging" pages (home, cookies, criteria discussion/stats, criteria + # index/show) -- pages whose rendered output does not change while the + # application runs (only on deploy) -- falling back to private, no-store + # without a redeploy. Mirrors CACHE_SHOW_PROJECT. + # See docs/cdn-cache-not-logged-in.md Section 10. + CACHE_UNCHANGING_PAGES = ENV['BADGEAPP_CACHE_UNCHANGING'] != 'false' + + # One shared surrogate key for every "unchanging" page, so a single purge + # refreshes all of them. Deliberately NOT per-page: these pages all change + # together (only on deploy), and one key avoids a purge_all that would + # needlessly evict the valuable project-show, JSON, and badge caches. + UNCHANGING_SURROGATE_KEY = 'unchanging' + # Fewer pages are cacheable than you might initially expect. # Most of the pages on this site vary depending on whether or not # you're logged in (because the header varies), so we can't cache most @@ -244,6 +258,29 @@ def set_surrogate_key_header(*surrogate_keys) end # rubocop:enable Naming/AccessorMethodName + # Cache an "unchanging" page on the CDN for anonymous users -- a page whose + # rendered output does not change while the application runs (only on deploy). + # Mirrors the projects#show HTML guard (cache only when the response carries + # no per-user state). cache_on_cdn also calls omit_session_cookie, so no + # Set-Cookie is emitted. + # + # Used as a before_action on qualifying actions; it runs after the inherited + # set_default_cache_control before_action, so it correctly overrides the + # private, no-store default for anonymous, flash-free HTML. The qualifying + # actions issue no internal redirect, so committing cache headers in a + # before_action (before the body runs) is safe; the browser-dependent locale + # redirect is handled earlier by redir_missing_locale, which halts the chain + # before this runs. See docs/cdn-cache-not-logged-in.md Section 10. + # @return [void] + def cache_unchanging_page_on_cdn + return unless CACHE_UNCHANGING_PAGES + return unless request.format.symbol == :html + return if logged_in? || !flash.empty? + + set_surrogate_key_header UNCHANGING_SURROGATE_KEY + cache_on_cdn + end + # Completely disables caching for sensitive pages. # Uses **no-store** to prevent any caching of the response. # @return [void] diff --git a/app/controllers/criteria_controller.rb b/app/controllers/criteria_controller.rb index 3a9e2a3d1..3f7d913f5 100644 --- a/app/controllers/criteria_controller.rb +++ b/app/controllers/criteria_controller.rb @@ -7,6 +7,14 @@ # Controller for criteria functionality. # class CriteriaController < ApplicationController + # Cache these "unchanging" pages on the CDN for anonymous users (output does + # not change until the next deploy). index/show only read URL-derived params + # (no mutable DB state) and issue no internal redirect. Query-string variants + # (?details=, etc.) are distinct cache objects under Fastly's URL-based key + # and all share UNCHANGING_SURROGATE_KEY. + # See docs/cdn-cache-not-logged-in.md Section 10. + before_action :cache_unchanging_page_on_cdn, only: %i[index show] + # Displays list of resources. # @return [void] def index diff --git a/app/controllers/static_pages_controller.rb b/app/controllers/static_pages_controller.rb index 9b046bc8d..3acb1345c 100644 --- a/app/controllers/static_pages_controller.rb +++ b/app/controllers/static_pages_controller.rb @@ -23,6 +23,23 @@ class StaticPagesController < ApplicationController # because flashes are stored in the session. before_action :omit_session_cookie, only: %i[robots google_verifier] + # Cache "unchanging" pages on the CDN for anonymous users -- pages whose + # output does not change until the next deploy. These actions render + # translation-driven content with no mutable DB state and issue no internal + # redirect, so the before_action (which commits cache headers before the + # body) is safe. The locale redirect for a locale-less URL is handled earlier + # by redir_missing_locale, which halts the chain first. + # See docs/cdn-cache-not-logged-in.md Section 10. + # + # rubocop:disable Rails/LexicallyScopedActionFilter + # "cookies" intentionally has no action method: defining one would shadow + # ActionController::Cookies#cookies (the cookie jar), breaking auth. It + # renders via implicit render from cookies.html.erb; the filter still matches + # by action name, so the cop's "define it on the class" rule cannot apply. + before_action :cache_unchanging_page_on_cdn, + only: %i[home cookies criteria_discussion criteria_stats] + # rubocop:enable Rails/LexicallyScopedActionFilter + def home; end def criteria_stats; end diff --git a/config/puma.rb b/config/puma.rb index 23a0cc024..ce931507c 100644 --- a/config/puma.rb +++ b/config/puma.rb @@ -53,6 +53,28 @@ # ActiveRecord::Base.establish_connection if defined?(ActiveRecord) # end +# After the web server boots a new release, refresh the shared CDN cache of +# "unchanging" pages (home, cookies, criteria pages) so a deploy's +# content/translation changes become visible. after_booted fires once, only in +# the server process -- not in `rails console`, rake tasks, or tests -- so no +# guard is needed. (Puma 7 deprecated the older on_booted name.) +# See docs/cdn-cache-not-logged-in.md Section 10. +after_booted do + if ApplicationController::CACHE_UNCHANGING_PAGES + key = ApplicationController::UNCHANGING_SURROGATE_KEY + # Immediate purge. purge_by_key catches its own errors and returns false + # (never raises), and is a no-op without Fastly credentials, so a Fastly + # hiccup (or a non-production boot) cannot break startup. + FastlyRails.purge_by_key(key) + # Delayed re-purge closes the rolling-deploy race: an old, still-draining + # process can repopulate the cache just after the immediate purge. This is + # the same recovery path PurgeCdnProjectJob provides for project edits. + PurgeCdnProjectJob + .set(wait: ApplicationController::BADGE_PURGE_DELAY.seconds) + .perform_later(key) + end +end + # Allow puma to be restarted by `rails restart` command. plugin :tmp_restart diff --git a/docs/cdn-cache-not-logged-in.md b/docs/cdn-cache-not-logged-in.md index c494d88e9..11f972d66 100644 --- a/docs/cdn-cache-not-logged-in.md +++ b/docs/cdn-cache-not-logged-in.md @@ -1056,7 +1056,7 @@ cookie-bypass rule, with the project surrogate key for purging). --- -## 10. Planned future work: CDN cache of static pages +## 10. Planned future work: CDN cache of unchanging pages > **Status: not part of this branch.** Everything in this section is recorded > here so the work is *ready to start* later; it will be implemented on a @@ -1090,15 +1090,16 @@ A page qualifies when **its entire body is wrapped in a single it reads no mutable database state.** That wrapper is not incidental: if such a page depended on per-request or live data, the *existing* fragment cache would already serve stale content. So the wrapper is simultaneously the selection -rule and the evidence the page is static-after-boot. The initial set: +rule and the evidence the page is unchanging (static after boot; it does not +change until the next deploy). The initial set: The "Page" column names each page by its canonical, **locale-prefixed** URL (e.g. `/en`, `/en/criteria`) — that is the object the CDN caches. The locale-less form (bare `/`, `/cookies`, `/criteria`, …) is **never** the cached object: it 302-redirects to the locale-prefixed URL via `redir_missing_locale` (an early parent `before_action` that calls `disable_cache` and halts the chain -before `cache_static_page_on_cdn` can run — see Section 4.1 and the -"locale-less static paths redirect uncached" test in Section 10.5). +before `cache_unchanging_page_on_cdn` can run — see Section 4.1 and the +"locale-less unchanging paths redirect uncached" test in Section 10.5). | Page | Controller#action | Fragment cache key today | | --- | --- | --- | @@ -1135,26 +1136,27 @@ alongside the existing cache constants, plus a kill switch mirroring `CACHE_SHOW_PROJECT`: ```ruby -# Kill switch: set BADGEAPP_CACHE_MISC_PAGES=false to instantly stop caching +# Kill switch: set BADGEAPP_CACHE_UNCHANGING=false to instantly stop caching # these pages (falls back to private, no-store) without a redeploy. -CACHE_MISC_PAGES = ENV['BADGEAPP_CACHE_MISC_PAGES'] != 'false' +CACHE_UNCHANGING_PAGES = ENV['BADGEAPP_CACHE_UNCHANGING'] != 'false' -# One shared surrogate key for every "static after startup" page, so a single -# purge refreshes all of them. Deliberately NOT per-page: these pages all -# change together (only on deploy), and one key avoids a purge_all that would +# One shared surrogate key for every "unchanging" page, so a single purge +# refreshes all of them. Deliberately NOT per-page: these pages all change +# together (only on deploy), and one key avoids a purge_all that would # needlessly evict the valuable project-show, JSON, and badge caches. -MISC_SURROGATE_KEY = 'miscellaneous' +UNCHANGING_SURROGATE_KEY = 'unchanging' -# Cache a "static after startup" page on the CDN for anonymous users. +# Cache an "unchanging" page on the CDN for anonymous users -- a page whose +# rendered output does not change until the next deploy. # Mirrors the projects#show HTML guard (Section 5, Change 2): cache only when # the response carries no per-user state. cache_on_cdn also calls # omit_session_cookie, so no Set-Cookie is emitted. -def cache_static_page_on_cdn - return unless CACHE_MISC_PAGES +def cache_unchanging_page_on_cdn + return unless CACHE_UNCHANGING_PAGES return unless request.format.symbol == :html return if logged_in? || !flash.empty? - set_surrogate_key_header MISC_SURROGATE_KEY + set_surrogate_key_header UNCHANGING_SURROGATE_KEY cache_on_cdn end ``` @@ -1165,11 +1167,11 @@ overrides the `private, no-store` default for anonymous, flash-free HTML: ```ruby # app/controllers/static_pages_controller.rb -before_action :cache_static_page_on_cdn, +before_action :cache_unchanging_page_on_cdn, only: %i[home cookies criteria_discussion criteria_stats] # app/controllers/criteria_controller.rb -before_action :cache_static_page_on_cdn, only: %i[index show] +before_action :cache_unchanging_page_on_cdn, only: %i[index show] ``` Because these actions set no flash on their render path, checking the incoming @@ -1179,7 +1181,7 @@ guarantees the anonymous response carries no CSRF meta tag, so nothing writes > **Caveat — these actions must never gain an *internal* redirect.** Unlike > `projects#show` (which guards an internal obsolete-section 301 with -> `return if performed?`, Section 5, Change 2), `cache_static_page_on_cdn` is a +> `return if performed?`, Section 5, Change 2), `cache_unchanging_page_on_cdn` is a > `before_action` that commits the `Surrogate-Control` / `Cache-Control` > headers *before* the action body runs. The qualifying actions are safe today > because none of them redirects internally (`home`, `cookies`, @@ -1212,19 +1214,20 @@ low-volume pages and merely tightens the staleness bound. ([`app/jobs/purge_cdn_project_job.rb`](../app/jobs/purge_cdn_project_job.rb)) already purges an **arbitrary** key with `retry_on` backoff, so **reuse it** directly (optionally rename it `PurgeCdnKeyJob`, since it is no longer -project-specific). Trigger from Puma's `on_booted` hook in +project-specific). Trigger from Puma's `after_booted` hook in [`config/puma.rb`](../config/puma.rb), which fires once, only in the server process — not in `rails console`, rake tasks, or tests, avoiding spurious -purges: +purges. (Puma 7 deprecated the older `on_booted` name in favor of +`after_booted`.) ```ruby # config/puma.rb # After the web server boots a new release, refresh the shared cache of -# "static after startup" pages so a deploy's content/translation changes +# "unchanging" pages so a deploy's content/translation changes # become visible. -on_booted do - if ApplicationController::CACHE_MISC_PAGES - key = ApplicationController::MISC_SURROGATE_KEY +after_booted do + if ApplicationController::CACHE_UNCHANGING_PAGES + key = ApplicationController::UNCHANGING_SURROGATE_KEY # Immediate purge. purge_by_key catches its own errors and returns # false (never raises), so a Fastly hiccup cannot break boot. FastlyRails.purge_by_key(key) @@ -1240,27 +1243,28 @@ end (If a Puma hook proves awkward, the fallback is an `after_initialize` block *guarded to the server process* so it does not fire for console/rake/test; -the `on_booted` hook is preferred precisely because it needs no such guard.) +the `after_booted` hook is preferred precisely because it needs no such guard.) In development and test, `FastlyRails.purge_by_key` is a no-op when Fastly credentials are absent, so this is inert outside production. ### 10.5 Tests Reuse the `CdnCachingTest` harness (Section 6), including -`with_forgery_protection`. Add the static paths to the existing +`with_forgery_protection`. Add the unchanging paths to the existing "anonymous read-only GETs set no session cookie" list, and add, looping over the qualifying paths (`/en`, `/en/cookies`, `/en/criteria_discussion`, -`/en/criteria_stats`, `/en/criteria`, `/en/criteria/0`): +`/en/criteria_stats`, `/en/criteria`, `/en/criteria/0`) across more than one +locale: * **Cacheable when anonymous** — `Surrogate-Control` present, `Cache-Control: - no-store`, `Surrogate-Key` equals `miscellaneous`, and no `_BadgeApp_session` + no-store`, `Surrogate-Key` equals `unchanging`, and no `_BadgeApp_session` cookie. * **Byte-identical across two requests** — the same core-invariant test as the show page (Section 6), so any future anonymous-only variance trips it. * **Logged-in bypass** — `private, no-store` (no `Surrogate-Control`). * **Carried-over flash bypass** — same persistent-flash technique as Section 6 (`password_resets#create`), asserting `private, no-store` on the next page. -* **Kill switch** — with `CACHE_MISC_PAGES` stubbed false, the response is +* **Kill switch** — with `CACHE_UNCHANGING_PAGES` stubbed false, the response is `private, no-store`. * **Locale-less paths redirect uncached** — the browser-dependent locale redirect (`redir_missing_locale`) must never be cached, so requesting each @@ -1272,7 +1276,7 @@ the qualifying paths (`/en`, `/en/cookies`, `/en/criteria_discussion`, ```ruby # The locale redirect varies by Accept-Language and must never be cached; # only the locale-prefixed page it lands on is cacheable. - test 'locale-less static paths redirect uncached' do + test 'locale-less unchanging paths redirect uncached' do ['/', '/cookies', '/criteria_discussion', '/criteria_stats', '/criteria'].each do |path| get path @@ -1301,10 +1305,10 @@ This work **must land after** the Section 1–9 changes are in production, because it relies on Change 1 (no anonymous CSRF meta tag ⇒ no gratuitous session cookie) and Change 3 (the cookie-bypass rule). Suggested order on the future branch: add the guard + constants + `before_action`s and tests; add the -`on_booted` purge; deploy; verify with the Section 8 `curl` recipe against the +`after_booted` purge; deploy; verify with the Section 8 `curl` recipe against the new paths (e.g. `/en` and `/en/criteria`), confirming first-`MISS`-then-`HIT`, no `Set-Cookie`, and that a session-cookie request returns `private, no-store`. -If anything misbehaves, set `BADGEAPP_CACHE_MISC_PAGES=false` to disable +If anything misbehaves, set `BADGEAPP_CACHE_UNCHANGING=false` to disable instantly. --- diff --git a/test/integration/cdn_caching_test.rb b/test/integration/cdn_caching_test.rb index d616b966d..dbe4c8e5b 100644 --- a/test/integration/cdn_caching_test.rb +++ b/test/integration/cdn_caching_test.rb @@ -27,7 +27,12 @@ class CdnCachingTest < ActionDispatch::IntegrationTest '/en', '/en/projects', "/en/projects/#{@project.id}/passing", - '/en/feed' + '/en/feed', + '/en/cookies', + '/en/criteria_discussion', + '/en/criteria_stats', + '/en/criteria', + '/en/criteria/0' ].each do |path| get path assert_response :success @@ -181,6 +186,127 @@ class CdnCachingTest < ActionDispatch::IntegrationTest end end + # ---------------------------------------------------------------------- + # "Unchanging" pages (Section 10): home, cookies, criteria discussion/stats, + # and the criteria index/show pages. Their rendered output does not change + # until the next deploy. They reuse the exact machinery of the project-show + # caching above. + # ---------------------------------------------------------------------- + + # These pages are translation-driven, so exercise a non-English locale too. + # Each locale is a distinct cache object (the locale is in the URL, hence in + # Fastly's cache key), so caching one must never leak into another. + UNCHANGING_LOCALES = %w[en fr].freeze + + # The locale-prefixed pages the CDN actually caches, for a given locale. + # "//criteria/0" is the criteria show page; "//criteria" is + # the index. + def unchanging_paths(locale) + %W[ + /#{locale} /#{locale}/cookies /#{locale}/criteria_discussion + /#{locale}/criteria_stats /#{locale}/criteria /#{locale}/criteria/0 + ] + end + + # Each unchanging page is CDN-cacheable when anonymous: it advertises + # Surrogate-Control, the shared "unchanging" Surrogate-Key (so one purge + # refreshes all of them), no-store Cache-Control, and sets no session cookie. + test 'anonymous unchanging pages are CDN-cacheable' do + with_forgery_protection do + UNCHANGING_LOCALES.each do |locale| + unchanging_paths(locale).each do |path| + get path + assert_response :success + assert response.headers['Surrogate-Control'].present?, + "#{path} should send Surrogate-Control for the CDN" + assert_equal 'no-store', response.headers['Cache-Control'], path + assert_equal ApplicationController::UNCHANGING_SURROGATE_KEY, + response.headers['Surrogate-Key'], path + assert_nil cookies['_BadgeApp_session'], path + end + end + end + end + + # CORE CORRECTNESS INVARIANT (Section 9.4): the CDN serves one cached object + # to every anonymous visitor, so two anonymous responses must be byte- + # identical. These pages are translation-driven with no mutable DB state, so + # an anonymous request has nothing left to vary. If a future change adds + # anonymous-only variance, this fails. + test 'anonymous unchanging pages are identical across requests' do + with_forgery_protection do + UNCHANGING_LOCALES.each do |locale| + unchanging_paths(locale).each do |path| + get path + assert_response :success + first_body = response.body + get path + assert_response :success + assert_equal first_body, response.body, + "#{path} must be byte-identical so the CDN can share " \ + 'one cached object among all guests' + end + end + end + end + + # Logged-in users must bypass the cache: a private, non-cacheable response + # with no Surrogate-Control. (Locale-independent, so en is sufficient.) + test 'logged-in unchanging pages are not CDN-cacheable' do + log_in_as(users(:test_user)) # POSTs login; do this before enabling CSRF + with_forgery_protection do + unchanging_paths('en').each do |path| + get path + assert_response :success + assert_equal 'private, no-store', response.headers['Cache-Control'], + path + assert_nil response.headers['Surrogate-Control'], path + end + end + end + + # An unchanging page that renders a carried-over flash is per-user content + # and must not be CDN-cached. (No forgery protection here: test env disables + # it, and this exercises the flash guard, not CSRF.) Uses the same persistent + # flash source as the show-page flash test (password_resets#create). + test 'unchanging page rendering a carried-over flash is not cacheable' do + post '/en/password_resets', + params: { password_reset: { email: 'nobody@example.org' } } + assert response.redirect? + get '/en/cookies' + assert_response :success + assert_equal 'private, no-store', response.headers['Cache-Control'], + 'a page rendering a carried-over flash must not be cached' + end + + # A locale-less request to any of these pages must 302-redirect (uncached) to + # its locale-prefixed form: the redirect target varies by Accept-Language, so + # it must never be cached (Section 4.1). Only the page it lands on is + # cacheable. + test 'locale-less unchanging paths redirect uncached' do + [ + '/', '/cookies', '/criteria_discussion', '/criteria_stats', + '/criteria' + ].each do |path| + get path + assert_response :found # 302, not 301 + assert_equal 'private, no-store', response.headers['Cache-Control'], path + assert_nil response.headers['Surrogate-Control'], path + end + end + + # The Atom feed renders Project.recently_updated -- mutable data that changes + # while the application runs -- so it must NEVER be CDN-cached, unlike the + # static-after-boot pages above. It calls no cache_on_cdn, so it keeps the + # set_default_cache_control default. This guards against a future change that + # mistakenly adds it to the cacheable set. + test 'the feed is not CDN-cacheable' do + get '/en/feed' + assert_response :success + assert_equal 'private, no-store', response.headers['Cache-Control'] + assert_nil response.headers['Surrogate-Control'] + end + def with_forgery_protection original = ActionController::Base.allow_forgery_protection ActionController::Base.allow_forgery_protection = true From 645ebfa62ae50f71859689f32a9c429fbfa239b8 Mon Sep 17 00:00:00 2001 From: "David A. Wheeler" Date: Fri, 26 Jun 2026 12:41:14 -0400 Subject: [PATCH 3/3] Reduce system-test flakiness Reduce system-test flakiness by implementing two driver-layer changes in test/application_system_test_case.rb: - Pass Chrome's --disable-dev-shm-usage so the renderer uses /tmp instead of the small (often 64MB) /dev/shm. Under load /dev/shm fills, the renderer crashes, and the dead browser session cascades into spurious errors in later tests. This is the standard fix for that cascade. - Raise Capybara.default_max_wait_time from 5s to 10s. The polling helpers (ensure_choice, wait_for_jquery, wait_for_url) derive their Timeout budgets from this; under contention 5s was too tight and they timed out together. 10s costs nothing on the happy path. Co-Authored-By: Claude Opus 4.8 Signed-off-by: David A. Wheeler --- test/application_system_test_case.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/application_system_test_case.rb b/test/application_system_test_case.rb index a877fa107..da49d72ec 100644 --- a/test/application_system_test_case.rb +++ b/test/application_system_test_case.rb @@ -40,7 +40,12 @@ Capybara.javascript_driver = driver.present? ? driver : :headless_chrome Capybara.default_driver = driver.present? ? driver : :headless_chrome -Capybara.default_max_wait_time = 5 +# Headroom for the polling/wait helpers (ensure_choice, wait_for_jquery, +# wait_for_url) whose Timeout budgets derive from this. Under load (a full +# system-test batch contending for CPU and the browser), 5s was too tight and +# they would all time out together; 10s costs nothing on the happy path since +# waits return as soon as their condition is met. +Capybara.default_max_wait_time = 10 Capybara.server_port = 31_337 # By default newer versions of Capybara have the annoying habit of @@ -64,5 +69,10 @@ class ApplicationSystemTestCase < ActionDispatch::SystemTestCase driver = ENV['DRIVER'].try(:to_sym) driven_by :selenium, using: driver || :headless_chrome, screen_size: [1400, 1400] do |option| option.add_argument('no-sandbox') + # Use /tmp instead of /dev/shm for Chrome's shared memory. /dev/shm is + # often capped at 64MB in containers/CI; when it fills under load the + # renderer process crashes, killing the browser session and cascading into + # spurious errors in later tests. This flag is the standard fix. + option.add_argument('disable-dev-shm-usage') end end