diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 952f6f3a2..32352c0a0 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -8,13 +8,17 @@ Only emit the CSRF token when it can actually be used. Emitting it calls form_authenticity_token, which writes session[:_csrf_token] and forces a _BadgeApp_session cookie -- defeating CDN caching for anonymous visitors. - Anonymous read-only pages need no token: form_with embeds its own hidden - authenticity_token (so login/signup/reset forms still work), and the only - JavaScript consumer of this meta tag (jQuery UJS "method:" links such as - logout and user-delete) appears only on logged-in pages. See - docs/cdn-cache-not-logged-in.md. + Most anonymous read-only pages need no token: form_with embeds its own + hidden authenticity_token (so login/signup/reset forms still work). + The exception is an anonymous page with a rails-ujs "method:" link, which + reads the token from this meta tag -- e.g. the login page's "Log in with + GitHub" button (a link_to ... method: 'post'). Such a page must opt in + explicitly with `content_for :needs_csrf_meta, 'true'` (the non-block form; + a block returning bare `true` outputs nothing, so content_for? stays + false), keeping the safe, cacheable default (no token) for other pages. See + docs/cdn-cache-not-logged-in.md (Change 1 and Section 9.1). -%> - <% if logged_in? %><%= csrf_meta_tags %><% end %> + <% if logged_in? || content_for?(:needs_csrf_meta) %><%= csrf_meta_tags %><% end %> <%= yield :special_head_values %> <%# The following are the same for a given locale, cache for speed -%> <% cache_frozen locale do # do not cache csrf_meta_tags -%> diff --git a/app/views/sessions/new.html.erb b/app/views/sessions/new.html.erb index cd023b139..0b4346a87 100644 --- a/app/views/sessions/new.html.erb +++ b/app/views/sessions/new.html.erb @@ -1,3 +1,13 @@ +<%# The "Log in with GitHub" button below is a rails-ujs link_to ... method: + 'post'; rails-ujs reads the CSRF token from the + tag, which the layout omits for anonymous users (CDN caching, Change 1). + Opt back in so the token is present and the POST to /auth/github passes + CSRF. This page is never CDN-cached, so emitting the tag here is harmless. + See docs/cdn-cache-not-logged-in.md Section 9.1. %> +<%# Use the non-block form: a content_for block captures the block's *output*, + and a block returning a bare `true` outputs nothing, so content_for? would + stay false. A non-empty string value makes content_for? true. %> +<% content_for :needs_csrf_meta, 'true' %> <% content_for :special_head_values do %> <%# same-origin (must NOT be no-referrer): Cross-origin requests (e.g. the OmniAuth redirect to github.com) carry no diff --git a/docs/cdn-cache-not-logged-in.md b/docs/cdn-cache-not-logged-in.md index 66f979d63..d6ac1c4a9 100644 --- a/docs/cdn-cache-not-logged-in.md +++ b/docs/cdn-cache-not-logged-in.md @@ -354,18 +354,50 @@ with a guarded version: Only emit the CSRF token when it can actually be used. Emitting it calls form_authenticity_token, which writes session[:_csrf_token] and forces a _BadgeApp_session cookie -- defeating CDN caching for anonymous visitors. - Anonymous read-only pages need no token: form_with embeds its own hidden - authenticity_token (so login/signup/reset forms still work), and the only - JavaScript consumer of this meta tag (jQuery UJS "method:" links such as - logout and user-delete) appears only on logged-in pages. See - docs/cdn-cache-not-logged-in.md. + Most anonymous read-only pages need no token: form_with embeds its own + hidden authenticity_token (so login/signup/reset forms still work). + The exception is an anonymous page with a rails-ujs "method:" link, which + reads the token from this meta tag -- e.g. the login page's "Log in with + GitHub" button (a link_to ... method: 'post'). Such a page must opt in + explicitly with `content_for :needs_csrf_meta, 'true'` (the non-block form; + a block returning bare `true` outputs nothing, so content_for? stays + false), keeping the safe, cacheable default (no token) for other pages. See + docs/cdn-cache-not-logged-in.md (Change 1 and Section 9.1). -%> - <% if logged_in? %><%= csrf_meta_tags %><% end %> + <% if logged_in? || content_for?(:needs_csrf_meta) %><%= csrf_meta_tags %><% end %> ``` `logged_in?` is the existing `SessionsHelper` predicate (`@session_user_id.present?`) and is available in views. +> **The login page is a real anonymous UJS consumer — do not skip the opt-in.** +> An earlier draft of this plan claimed UJS `method:` links "appear only on +> logged-in pages." That is **wrong**: `app/views/sessions/new.html.erb` +> renders the "Log in with GitHub" button as `link_to … method: 'post'`, and +> rails-ujs reads the token from this meta tag. Without the opt-in, the +> anonymous login page has no meta tag, the generated POST to `/auth/github` +> carries no token, and OmniAuth rejects it +> (`ActionController::InvalidAuthenticityToken` → `/auth/failure` → 404). The +> login page therefore sets `content_for :needs_csrf_meta, 'true'` (see the +> per-page opt-in note below). The header's "Login"/"Sign up" entries are +> ordinary GET `link_to`s, so they do **not** trigger this and every other +> anonymous page stays cookie-free and cacheable. A regression test +> (`test/integration/cdn_caching_test.rb`, "anonymous login page emits the CSRF +> meta tag for GitHub OAuth") locks this in **with forgery protection enabled** +> — without that flag the bug is invisible in the test environment (Section 6). + +**Per-page opt-in — use the non-block `content_for`.** On a page that needs the +token, set the flag at the top of its template: + +```erb +<% content_for :needs_csrf_meta, 'true' %> +``` + +Use the **non-block** form with a non-empty string value. The block form +`content_for(:needs_csrf_meta) { true }` does **not** work: `content_for` +captures the block's rendered *output*, and a block returning a bare `true` +outputs nothing, so `content_for?` stays false and the tag is never emitted. + ### Change 2: Cache anonymous `projects#show` HTML In [`app/controllers/projects_controller.rb`](../app/controllers/projects_controller.rb), @@ -464,6 +496,14 @@ cookies. #### Option A: Fastly Web UI Request Setting (recommended) +This is the configuration actually used in production. Because it lives in the +Fastly service (not in this repository), it is **not** under the project's +normal version control — so the exact steps are recorded here. It is a +load-bearing security control (Section 9.4): without it, a logged-in or +remember-me user can be served a cached anonymous page. + +The setting in summary: + * **Name:** `Bypass cache for personalized requests` * **Action:** `Pass` * **Condition** — Apply if: @@ -472,6 +512,55 @@ cookies. req.http.Cookie ~ "(_BadgeApp_session|remember_token|user_id)=" && req.url.path !~ "\.(css|js|png|gif|jpg|jpeg|svg|json|csv|txt|ico|woff2?|map)$" && req.url.path !~ "/(badge|baseline)$" ``` +**Step-by-step in the Fastly web UI:** + +1. **Identify the service.** The Fastly service is named by the `FASTLY_SERVICE_ID` + environment variable on the corresponding Heroku app + (`heroku config -a | grep FASTLY_SERVICE_ID`). Match that ID in the + dashboard. Do staging first, then production. +2. **Open and clone the active version.** At + [manage.fastly.com](https://manage.fastly.com), open the service, then click + **Edit configuration → Clone version N to edit**. Fastly will not let you + edit the live version directly; cloning creates an editable draft, and + nothing goes live until you Activate (step 6). +3. **Create the request setting.** In the left sidebar click **Settings** (the + gear). In the **Request Settings** section click **Create a request + setting**, then set **Name** = `Bypass cache for personalized requests` and + **Action** = **Pass**. Leave the other fields at their defaults. +4. **Attach the condition.** In the same form, under **Conditions** / **Request + condition**, click **Create a new condition**. Set **Name** = + `Personalized page request` and paste the **Apply if** expression above + verbatim (it is valid VCL; if the UI rejects it, the usual cause is a + smart-quote introduced by copy/paste — make sure the quotes are plain `"`). + Save the condition, then save the request setting. +5. **(Optional) Cookie-stripping for hit rate.** The `unset req.http.Cookie` + optimization in Option B (which lets guests carrying only unrelated cookies, + e.g. analytics, still share one cached object) is **not** expressible as a + simple Request Setting. It is *not required for correctness* — Option A above + is sufficient and safe on its own. If you want the hit-rate improvement, add + it via a small custom VCL snippet (Option B) instead. +6. **Activate.** Click **Activate** on the draft version to make it live (a few + seconds to propagate). + +> **If your UI has no "Pass" action on Request Settings:** the equivalent is a +> **Cache Setting** with **Action: Pass** plus a **Cache condition** using the +> same expression. + +**Companion Rails kill switch.** The Fastly rule and the Rails kill switch +(`BADGEAPP_CACHE_SHOW_PROJECT`, Change 2) are independent. Caching HTML only +actually happens when *both* are enabled: the Fastly bypass rule is active *and* +`BADGEAPP_CACHE_SHOW_PROJECT` is not `false`. If show pages return +`private, no-store` with no `Surrogate-Control` even after the Rails deploy, +check `heroku config -a | grep BADGEAPP_CACHE_SHOW_PROJECT` — set it to +`true` (or unset it; the default is on) and restart. Conversely, setting it to +`false` is the instant rollback for HTML caching, leaving this Fastly rule +harmlessly in place. + +**Verify after activating** with `script/verify_cdn_caching.sh -v ` +(Section 8): a request carrying `_BadgeApp_session` or `remember_token` must +return a non-`HIT` `X-Cache` even when an anonymous cached object exists, while +an anonymous request gets `MISS` then `HIT`. + #### Option B: Custom VCL snippet (placement: `recv`) ```vcl @@ -755,6 +844,13 @@ The rendered show page is the canonical section URL (`/en/projects/:id/:section`); `/en/projects/:id` only redirects to it, so the tests below target the section URL directly. +[`script/verify_cdn_caching.sh`](../script/verify_cdn_caching.sh) automates all +of the checks in this section (run it as +`script/verify_cdn_caching.sh -v https://staging.bestpractices.dev 1`); it +exits non-zero on any failure, so it also serves as the periodic synthetic +monitor recommended in Section 9.4. The raw `curl` recipes below remain useful +for ad-hoc inspection. + ### Anonymous request is cached ```bash @@ -817,18 +913,28 @@ which our change omits for anonymous users. cookie" test iterates over a list of anonymous pages; adding UJS/AJAX to any of them flips the meta tag back on and trips the test. Keep that list current as anonymous pages are added. -* **Do better (opt-in escape hatch).** If a specific anonymous page ever - legitimately needs the token, it should opt in explicitly rather than - forcing it globally. For example, gate the layout on login *or* an - explicit request: +* **Opt-in escape hatch (implemented).** A specific anonymous page that + legitimately needs the token opts in explicitly rather than forcing it + globally. The layout is gated on login *or* an explicit request: ```erb <% if logged_in? || content_for?(:needs_csrf_meta) %><%= csrf_meta_tags %><% end %> ``` - and have that one page set `content_for(:needs_csrf_meta) { true }`. This - keeps the default (no token, cacheable) safe while making the exception - loud and local. + and the page sets the flag at the top of its template: + + ```erb + <% content_for :needs_csrf_meta, 'true' %> + ``` + + This is **already in use** by the login page + (`app/views/sessions/new.html.erb`), whose "Log in with GitHub" button is a + rails-ujs `link_to … method: 'post'` that needs the token (see the warning + under Change 1). Use the **non-block** form with a non-empty string: + `content_for(:needs_csrf_meta) { true }` does **not** work because + `content_for` captures the block's *output*, and a block returning a bare + `true` outputs nothing, leaving `content_for?` false. The default (no token, + cacheable) stays safe for every page that does not opt in. ### 9.2 A persistent flash reaches an anonymous user diff --git a/script/verify_cdn_caching.sh b/script/verify_cdn_caching.sh new file mode 100755 index 000000000..c34b4255f --- /dev/null +++ b/script/verify_cdn_caching.sh @@ -0,0 +1,216 @@ +#!/usr/bin/env bash + +# Copyright the Linux Foundation and the +# OpenSSF Best Practices badge contributors +# SPDX-License-Identifier: MIT + +# Verify CDN (Fastly) caching of anonymous "project show" pages, per +# docs/cdn-cache-not-logged-in.md Section 8. Runs items 1-4 of the staging +# verification plan: +# +# 1. An anonymous request is cacheable and served from cache (HIT), and +# sets no _BadgeApp_session cookie. +# 2. A request carrying _BadgeApp_session bypasses the cache (never HIT), +# even when a cached anonymous object exists. +# 3. A request carrying the remember-me cookie bypasses the cache likewise. +# 4. JSON still caches regardless of cookies (no regression). +# +# Items 2 and 3 exercise the Fastly bypass rule (Change 3), which lives in +# the Fastly config, NOT in this repo. If that rule is not yet deployed to +# the target, those checks will fail -- that is a real signal, not a bug in +# this script. Note: until cacheable HTML actually ships, items 2 and 3 pass +# trivially (nothing is cached, so nothing can be a HIT); they only become +# meaningful once item 1 passes. +# +# This is also suitable as a periodic synthetic monitor (Section 9.4): a +# session-cookie or remember-me request returning HIT is a security +# regression in the bypass rule. +# +# Usage: script/verify_cdn_caching.sh [-v|--verbose] [BASE_URL] [PROJECT_ID] +# -v, --verbose Print the full response header block for every request +# (useful for inspecting the existing JSON/badge VCL while +# hunting for the rule to copy, and confirming the CDN +# strips Surrogate-Control before the browser). +# BASE_URL default: https://staging.bestpractices.dev +# PROJECT_ID default: 1 +# +# Exit status is 0 only if every check passes. + +set -u + +usage() { + cat <<'EOF' +Usage: script/verify_cdn_caching.sh [-v|--verbose] [BASE_URL] [PROJECT_ID] + -v, --verbose Print the full response header block for every request. + BASE_URL default: https://staging.bestpractices.dev + PROJECT_ID default: 1 +EOF +} + +VERBOSE='' +positional=() +for arg in "$@"; do + case "$arg" in + -v|--verbose) VERBOSE=1 ;; + -h|--help) usage; exit 0 ;; + -*) printf 'Unknown option: %s\n' "$arg" >&2; usage >&2; exit 2 ;; + *) positional+=("$arg") ;; + esac +done + +BASE="${positional[0]:-https://staging.bestpractices.dev}" +PROJECT_ID="${positional[1]:-1}" +BASE="${BASE%/}" # strip any trailing slash + +SHOW_URL="$BASE/en/projects/$PROJECT_ID/passing" +JSON_URL="$BASE/projects/$PROJECT_ID.json" + +# Colorize only when stdout is a terminal. +if [ -t 1 ]; then + RED=$(printf '\033[31m'); GREEN=$(printf '\033[32m') + YELLOW=$(printf '\033[33m'); CYAN=$(printf '\033[36m') + BOLD=$(printf '\033[1m'); RESET=$(printf '\033[0m') +else + RED=''; GREEN=''; YELLOW=''; CYAN=''; BOLD=''; RESET='' +fi + +passes=0 +failures=0 + +# Fetch a URL with Fastly debugging on, discarding the body and printing +# only the response headers (no "< " prefix, unlike curl -v). Extra args +# (e.g. -H "Cookie: ...") are passed through. +fetch_headers() { + curl -sS -o /dev/null -D - -H 'Fastly-Debug: 1' "$@" +} + +# In --verbose mode, print a labeled, indented dump of a header block (with +# CR characters stripped so it reads cleanly). No-op otherwise. +# Usage: vlog "label" "$headers" +vlog() { + [ -n "$VERBOSE" ] || return 0 + printf '%s --- %s ---%s\n' "$CYAN" "$1" "$RESET" + printf '%s\n' "$2" | tr -d '\r' | sed '/^$/d; s/^/ /' +} + +# Fetch + (optionally) dump in one step, returning the headers on stdout. +# The dump goes to stderr so it never pollutes the captured value. +# Usage: headers=$(fetch_and_log "label" [curl args...] URL) +fetch_and_log() { + local label="$1"; shift + local headers + headers=$(fetch_headers "$@") + vlog "$label" "$headers" >&2 + printf '%s' "$headers" +} + +# Extract the last value of a (case-insensitive) header from header text. +# Usage: header_value "X-Cache" "$headers" +header_value() { + printf '%s\n' "$2" | grep -i "^$1:" | tail -1 | + sed 's/^[^:]*:[[:space:]]*//' | tr -d '\r' +} + +# Does the response indicate a Fastly cache HIT? Fastly may chain nodes +# (e.g. "X-Cache: MISS, HIT"), so we look for HIT anywhere in the value. +is_hit() { + printf '%s\n' "$1" | grep -iq '^X-Cache:.*HIT' +} + +report() { # status_bool description detail + if [ "$1" = 'true' ]; then + passes=$((passes + 1)) + printf ' %sPASS%s %s\n' "$GREEN" "$RESET" "$2" + else + failures=$((failures + 1)) + printf ' %sFAIL%s %s\n' "$RED" "$RESET" "$2" + fi + [ -n "${3:-}" ] && printf ' %s%s%s\n' "$YELLOW" "$3" "$RESET" + return 0 +} + +printf '%sVerifying CDN caching%s\n' "$BOLD" "$RESET" +printf ' Base URL: %s\n' "$BASE" +printf ' Show page: %s\n' "$SHOW_URL" +printf ' JSON: %s\n' "$JSON_URL" +[ -n "$VERBOSE" ] && printf ' Verbose: on\n' +printf '\n' + +# --- Item 1: anonymous request is cacheable and served from cache ---------- +printf '%s1. Anonymous show page is cacheable (MISS then HIT), no cookie%s\n' \ + "$BOLD" "$RESET" +h1=$(fetch_and_log 'anon request 1 (warm)' "$SHOW_URL") +h2=$(fetch_and_log 'anon request 2 (expect HIT)' "$SHOW_URL") +xc1=$(header_value 'X-Cache' "$h1") +xc2=$(header_value 'X-Cache' "$h2") +surrogate=$(header_value 'Surrogate-Control' "$h1") +setcookie=$(printf '%s\n' "$h1" "$h2" | grep -i '^Set-Cookie:.*_BadgeApp_session') + +if [ -n "$surrogate" ]; then + report true "advertises Surrogate-Control to the CDN" "Surrogate-Control: $surrogate" +else + report false "advertises Surrogate-Control to the CDN" \ + 'missing Surrogate-Control: Rails is not marking this page cacheable' +fi + +if [ -z "$setcookie" ]; then + report true 'sets no _BadgeApp_session cookie' +else + report false 'sets no _BadgeApp_session cookie' "$setcookie" +fi + +if is_hit "$h2"; then + report true 'second request is a Fastly HIT' "X-Cache: $xc1 -> $xc2" +else + report false 'second request is a Fastly HIT' \ + "X-Cache: $xc1 -> $xc2 (Fastly bypass rule may be passing all HTML, or caching not yet enabled)" +fi + +# --- Item 2: session cookie bypasses the cache ----------------------------- +printf '\n%s2. _BadgeApp_session request bypasses the cache (never HIT)%s\n' \ + "$BOLD" "$RESET" +fetch_and_log 'anon warm 1' "$SHOW_URL" >/dev/null # ensure a cached object exists +fetch_and_log 'anon warm 2' "$SHOW_URL" >/dev/null +hs=$(fetch_and_log 'with _BadgeApp_session cookie' \ + -H 'Cookie: _BadgeApp_session=anything' "$SHOW_URL") +xcs=$(header_value 'X-Cache' "$hs") +if is_hit "$hs"; then + report false 'session-cookie request is not served from cache' \ + "X-Cache: $xcs (SECURITY: a personalized request got a cached anonymous page -- check the Fastly bypass rule)" +else + report true 'session-cookie request is not served from cache' "X-Cache: $xcs" +fi + +# --- Item 3: remember-me cookie bypasses the cache ------------------------- +printf '\n%s3. remember_token request bypasses the cache (never HIT)%s\n' \ + "$BOLD" "$RESET" +hr=$(fetch_and_log 'with remember_token cookie' \ + -H 'Cookie: remember_token=anything' "$SHOW_URL") +xcr=$(header_value 'X-Cache' "$hr") +if is_hit "$hr"; then + report false 'remember-me request is not served from cache' \ + "X-Cache: $xcr (SECURITY: check the Fastly bypass rule)" +else + report true 'remember-me request is not served from cache' "X-Cache: $xcr" +fi + +# --- Item 4: JSON still caches regardless of cookies (no regression) ------- +printf '\n%s4. JSON still caches despite a session cookie (no regression)%s\n' \ + "$BOLD" "$RESET" +fetch_and_log 'json warm (with cookie)' \ + -H 'Cookie: _BadgeApp_session=anything' "$JSON_URL" >/dev/null +hj=$(fetch_and_log 'json with cookie (expect HIT)' \ + -H 'Cookie: _BadgeApp_session=anything' "$JSON_URL") +xcj=$(header_value 'X-Cache' "$hj") +if is_hit "$hj"; then + report true 'JSON is served from cache even with a cookie' "X-Cache: $xcj" +else + report false 'JSON is served from cache even with a cookie' \ + "X-Cache: $xcj (JSON is meant to cache regardless of cookies)" +fi + +# --- Summary --------------------------------------------------------------- +printf '\n%sSummary:%s %s%d passed%s, %s%d failed%s\n' \ + "$BOLD" "$RESET" "$GREEN" "$passes" "$RESET" "$RED" "$failures" "$RESET" + +[ "$failures" -eq 0 ] diff --git a/test/integration/cdn_caching_test.rb b/test/integration/cdn_caching_test.rb index b4d73ee41..68ec4ed46 100644 --- a/test/integration/cdn_caching_test.rb +++ b/test/integration/cdn_caching_test.rb @@ -5,6 +5,7 @@ require 'test_helper' +# rubocop:disable Metrics/ClassLength class CdnCachingTest < ActionDispatch::IntegrationTest setup do @project = projects(:one) @@ -102,6 +103,21 @@ class CdnCachingTest < ActionDispatch::IntegrationTest end end + # The login page initiates GitHub OAuth with a rails-ujs "method: post" + # link, which reads the CSRF token from the meta tag. Change 1 removes that + # tag for anonymous users, so the login page must opt back in via + # content_for(:needs_csrf_meta). Without the tag the POST to /auth/github + # fails CSRF (ActionController::InvalidAuthenticityToken) and login 404s. + # This must be tested with forgery protection ON: it is off by default in + # the test env, so the bug is invisible otherwise (see Section 6 caveat). + test 'anonymous login page emits the CSRF meta tag for GitHub OAuth' do + with_forgery_protection do + get login_path(locale: :en) + assert_response :success + assert_select 'meta[name="csrf-token"]', count: 1 + end + end + # An obsolete-section 301 from inside show must NOT be cached: the # "return if performed?" guard keeps cache_on_cdn from attaching to it. test 'obsolete-section redirect is not CDN-cacheable' do @@ -150,3 +166,4 @@ def with_forgery_protection ActionController::Base.allow_forgery_protection = original end end +# rubocop:enable Metrics/ClassLength