From e265ad91060712df80a8ff9dcce83f4dc7a2583e Mon Sep 17 00:00:00 2001 From: AI Dev Date: Mon, 21 Sep 2026 09:14:48 +0000 Subject: [PATCH 1/3] Cache the anonymous page, so a bot costs a file read instead of a render (#233) On 2026-09-20 the front page took 62,588 requests and every one was cache=- : not one served by nginx, every one a Rails render, for 1,787 distinct clients. Assets were at 89% hit, snapshots and the Open Wall were cached, and the largest class of traffic on the site was the only large one that was not. 25,525 of those arrived from openipc.kz, openipc.ru and openipc.eu, which proxy without caching. So caching at the origin is what stops a mirror visitor costing a render, without touching three configs this repository does not own. Rails had been asking for this for days. Since #155 a GET on / answers `public, max-age=300, stale-while-revalidate=3600` and a catalogue page `max-age=3600, stale-while-revalidate=86400`, and nothing was listening. The change is a proxy_cache on `location /`, the way the three locations above it already have one. ## What this location needs that the others do not It is the one that also serves /admin and Devise, so two of the guards that are right for a gallery page are wrong here. No `proxy_hide_header Set-Cookie`. Every other cached location strips it, because those pages never set one; this location serves the sign-in form, and stripping the header there means nobody can sign in. Not *storing* such a response is what is wanted, and nginx does that by itself. No `proxy_cache_valid`. Rails is the only thing that knows about the flash, the signed-in admin and the CSRF token, so it is the only thing that can decide what may be cached; proxy_cache_valid applies to responses that say nothing, which here are exactly the ones that must not be stored. ## Two things that would have gone unnoticed `add_header` in a location replaces every inherited one, so adding X-Cache-Status would have removed the vhost's HSTS from every page on the site -- invisibly, because Rails sends a stronger one of its own. Repeated in the location, and asserted. The key keeps the query string. Only 444 of 63,032 front-page requests carry one, so there is nothing to gain by dropping it, and dropping it is what produced the bug openipc-microcache.conf documents at length. ## Verified on production / and /ru HIT; /donate, /privacy and /get-started MISS then HIT. Two HSTS headers, as before. Accept-Language still selects the language, so Vary is being honoured -- / with Accept-Language: ru answers lang="ru", which is why that header is load-bearing rather than decoration. /admin/sign_in is MISS, max-age=0 private, and still sets its session cookie. 733 tests. Six guards, each confirmed by reverting it -- after the first round of reverts turned out to be editing the assets location instead of this one, and the first version of the tests matched the comments explaining why a directive is absent rather than the directives themselves. --- deploy/nginx/sites-available/org.openipc | 55 +++++++++++ test/deploy/page_cache_test.rb | 119 +++++++++++++++++++++++ 2 files changed, 174 insertions(+) create mode 100644 test/deploy/page_cache_test.rb diff --git a/deploy/nginx/sites-available/org.openipc b/deploy/nginx/sites-available/org.openipc index 41b3edb..3aad423 100644 --- a/deploy/nginx/sites-available/org.openipc +++ b/deploy/nginx/sites-available/org.openipc @@ -448,6 +448,61 @@ server { proxy_connect_timeout 120; proxy_send_timeout 120; proxy_read_timeout 180; + + # The pages themselves (#233). + # + # On 2026-09-20 the front page took 62,588 requests and every one was + # cache=- : not one served by nginx, every one a Rails render, for + # 1,787 distinct clients. Assets hit 89%, the snapshots and the Open + # Wall are cached, and the largest class on the site was the one class + # that was not. 25,525 of those arrived from openipc.kz, openipc.ru and + # openipc.eu, which proxy without caching -- so caching here is what + # stops a mirror visitor costing a render, without touching three + # configs this repository does not own. + # + # Rails has been asking for this for days. Since #155 a GET on / comes + # back `public, max-age=300, stale-while-revalidate=3600`, and a + # catalogue page `max-age=3600, stale-while-revalidate=86400`, and + # nothing was listening. + proxy_cache openipc_micro; + + # The whole URI, query included. Only 444 of 63,032 front-page requests + # carry one, so there is nothing to gain by dropping it -- and dropping + # it is what produced the bug the microcache conf documents at length, + # where a key that ignored ?locale= served Russian to English readers. + proxy_cache_key $scheme$host$request_uri; + + # No proxy_cache_valid, deliberately. Rails decides what may be cached + # and for how long, and it is the only thing that can: it knows about + # the flash, the signed-in admin and the CSRF token. A page it declares + # `max-age=0, private` -- /admin, anything with a flash, any non-200 -- + # nginx will not store, and a proxy_cache_valid here would be a second + # opinion that silently overrides the first on exactly those pages. + # + # For the same reason there is no `proxy_hide_header Set-Cookie`, which + # every other cached location has. Those serve pages that never need a + # cookie; this one serves /admin and Devise, which must be able to set + # a session. nginx already refuses to store a response carrying + # Set-Cookie, which is the right answer rather than one to suppress. + proxy_cache_lock on; + proxy_cache_lock_age 30s; + proxy_cache_lock_timeout 30s; + proxy_cache_use_stale updating error timeout; + + # An admin reaches Rails rather than an anonymous copy, and an admin's + # render is never stored for anyone else. Same pair as the locations + # above; see ApplicationController#refuse_shared_caching_for_admins. + proxy_cache_bypass $cookie__openipc_session; + proxy_no_cache $upstream_http_x_admin_view; + proxy_hide_header X-Admin-View; + + # `add_header` in a location REPLACES every inherited one, so the + # server-level HSTS has to be repeated here or adding X-Cache-Status + # would quietly remove it. Rails sends a stronger HSTS of its own on + # every page, so the loss would not have shown up in a browser -- which + # is exactly why it is worth being explicit about. + add_header Strict-Transport-Security max-age=15768000; + add_header X-Cache-Status $upstream_cache_status; } } diff --git a/test/deploy/page_cache_test.rb b/test/deploy/page_cache_test.rb new file mode 100644 index 0000000..98b4af4 --- /dev/null +++ b/test/deploy/page_cache_test.rb @@ -0,0 +1,119 @@ +# frozen_string_literal: true + +require 'test_helper' + +# The pages themselves are cached by nginx now (#233). +# +# On 2026-09-20 the front page took 62,588 requests and every one was `cache=-` +# -- every one a Rails render, for 1,787 distinct clients. Assets were at 89% +# hit, snapshots and the Open Wall were cached, and the largest class on the +# site was the only large one that was not. +# +# What makes the catch-all different from every other cached location, and what +# these hold: it is the one that also serves /admin and Devise. The guards that +# are right for a gallery page are wrong here, and the ways to get it wrong are +# all one plausible line. +class PageCacheTest < ActiveSupport::TestCase + VHOST = Rails.root.join('deploy/nginx/sites-available/org.openipc').read.freeze + + # The proxying catch-all in the TLS vhost. There are two `location /` blocks + # -- the port-80 one only redirects to https -- and the first in the file is + # the wrong one, which is what the earliest version of this helper picked up. + def catch_all + blocks = VHOST.scan(/\n location \/ \{\n.*?\n \}\n/m) + proxying = blocks.select { |b| b.include?('proxy_pass') } + + assert_equal 1, proxying.length, + "expected exactly one proxying `location /`, found #{proxying.length} " \ + "among #{blocks.length} blocks" + proxying.first + end + + # Directives only. The block explains at length why it does *not* set + # proxy_cache_valid and does *not* hide Set-Cookie, and a naive `include?` + # matches that prose and reports the opposite of the truth -- which is what + # the first version of these tests did. + def directives + catch_all.lines.reject { |l| l.strip.start_with?('#') }.join + end + + test 'the page cache exists at all' do + assert_includes directives, 'proxy_cache openipc_micro', + 'the largest class of traffic on the site is uncached again' + end + + # A response carrying Set-Cookie must not be stored, and nginx already + # refuses to store one. Every other cached location strips the header + # instead, because those pages never need a cookie -- but this location + # serves /admin and Devise, and stripping it there means nobody can sign in. + test 'the catch-all does not strip Set-Cookie the way the others do' do + assert_not_includes directives, 'proxy_hide_header Set-Cookie', <<~MESSAGE.chomp + `location /` hides Set-Cookie. That is correct for the gallery locations, + whose pages never set one, and it breaks signing in here: Devise's + session cookie is set on a response this location serves. + + Not storing such a response is what is wanted, and nginx does that by + itself. Hiding the header instead throws away the cookie and keeps + serving the page. + MESSAGE + end + + # Rails is the only thing that knows about the flash, the signed-in admin and + # the CSRF token, so it is the only thing that can decide what may be cached. + # A proxy_cache_valid here is a second opinion that overrides the first on + # exactly the pages where the first one matters. + test 'nginx does not offer its own opinion on how long a page lives' do + offered = directives.lines.grep(/proxy_cache_valid/).map(&:strip) + + assert_empty offered, <<~MESSAGE.chomp + `location /` sets a lifetime of its own: + + #{offered.join("\n ")} + + This location serves /admin, Devise and every page carrying a flash. + ApplicationController declares those `max-age=0, private` and nginx + honours it; proxy_cache_valid applies to responses that say nothing, + which here are the ones that must not be stored. + MESSAGE + end + + # `add_header` in a location replaces every inherited one. The server block + # sets HSTS, so adding X-Cache-Status without repeating it removes HSTS from + # every page on the site -- invisibly, because Rails sends a stronger one of + # its own and a browser would still be protected. + test 'adding a cache header does not drop the security header above it' do + server_level = VHOST.lines.grep(/add_header Strict-Transport-Security/).map(&:strip).uniq + + refute_empty server_level, 'the vhost stopped setting HSTS; this test now proves nothing' + assert_includes directives, server_level.first, <<~MESSAGE.chomp + `location /` uses add_header and does not repeat: + + #{server_level.first} + + add_header at location level replaces all inherited add_header + directives, so this location would serve every page on the site without + the vhost's HSTS. + MESSAGE + end + + test 'an admin still reaches Rails, and is never stored for anyone else' do + assert_includes directives, 'proxy_cache_bypass $cookie__openipc_session' + assert_includes directives, 'proxy_no_cache $upstream_http_x_admin_view' + end + + # Vary covers headers, not the query string. The front page renders per + # Accept-Language -- `/` with Accept-Language: ru answers lang="ru" -- so + # Vary is load-bearing here and nginx honours it. The query has to be in the + # key by hand, and dropping it is what produced the bug the microcache + # configuration documents. + test 'the key keeps the query string' do + key = directives[/proxy_cache_key (.*);/, 1] + + assert key, '`location /` caches without a key of its own' + assert_includes key, '$request_uri', <<~MESSAGE.chomp + The key is `#{key}`, which drops the query string. ?locale= still + selects a language on routes with no prefixed form, so a key that + ignores it stores one language under a key that claims none. + MESSAGE + end +end From 0fb620db77ad890e39eb0f8bc1cd147648ee5b5b Mon Sep 17 00:00:00 2001 From: AI Dev Date: Mon, 21 Sep 2026 09:22:30 +0000 Subject: [PATCH 2/3] Address review: the HSTS guard was satisfied by the line it was guarding The test grepped the whole vhost for `add_header Strict-Transport-Security`, and since this change the catch-all contains one. So removing the server-level directive left the assertion looking at the location's own copy, passed, and would have let every other location -- assets, fonts, the snapshot pages, the Open Wall -- lose the header it inherits, silently. Confirmed before fixing: with the server-level line deleted, the old test was still green. It now reads directives that belong to a `server` block rather than to any location inside it, tracked by brace depth rather than by indentation so it does not quietly stop working when someone reformats the file. Both directions fail as they should: deleting the inherited declaration, and dropping the location's repeat of it. 733 tests, no rubocop offences. --- test/deploy/page_cache_test.rb | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/test/deploy/page_cache_test.rb b/test/deploy/page_cache_test.rb index 98b4af4..b52a5a0 100644 --- a/test/deploy/page_cache_test.rb +++ b/test/deploy/page_cache_test.rb @@ -37,6 +37,25 @@ def directives catch_all.lines.reject { |l| l.strip.start_with?('#') }.join end + # Directives that belong to a `server` block itself, not to any location + # inside it -- which is what "inherited" means and what a location's own + # add_header replaces. Tracked by brace depth rather than indentation, so it + # does not quietly stop working the day someone reformats the file. + def server_level_directives + depth = 0 + in_location = nil + + VHOST.lines.filter_map do |line| + stripped = line.strip + opening = stripped.end_with?('{') + in_location = depth if opening && stripped.start_with?('location', 'if (') + keep = depth == 1 && in_location.nil? && !opening && !stripped.start_with?('#') + depth += line.count('{') - line.count('}') + in_location = nil if in_location && depth <= in_location + keep ? line : nil + end + end + test 'the page cache exists at all' do assert_includes directives, 'proxy_cache openipc_micro', 'the largest class of traffic on the site is uncached again' @@ -82,9 +101,17 @@ def directives # every page on the site -- invisibly, because Rails sends a stronger one of # its own and a browser would still be protected. test 'adding a cache header does not drop the security header above it' do - server_level = VHOST.lines.grep(/add_header Strict-Transport-Security/).map(&:strip).uniq + server_level = server_level_directives.grep(/add_header Strict-Transport-Security/) + .map(&:strip).uniq - refute_empty server_level, 'the vhost stopped setting HSTS; this test now proves nothing' + refute_empty server_level, <<~MESSAGE.chomp + No `server` block declares HSTS any more. + + This test used to grep the whole file, which the catch-all's own copy of + the directive satisfied -- so removing the inherited one left every + other location without HSTS and this test still green. Whatever removed + it needs to be looked at, not this assertion. + MESSAGE assert_includes directives, server_level.first, <<~MESSAGE.chomp `location /` uses add_header and does not repeat: From e83efed84d6d85261ed1fb981b5808fbf4962c61 Mon Sep 17 00:00:00 2001 From: AI Dev Date: Mon, 21 Sep 2026 09:24:26 +0000 Subject: [PATCH 3/3] Split the vhost parser up so rubocop is satisfied Same behaviour, three small methods instead of one that tripped four Metrics cops at once. Both revert directions still fail. --- test/deploy/page_cache_test.rb | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/test/deploy/page_cache_test.rb b/test/deploy/page_cache_test.rb index b52a5a0..e009177 100644 --- a/test/deploy/page_cache_test.rb +++ b/test/deploy/page_cache_test.rb @@ -20,7 +20,7 @@ class PageCacheTest < ActiveSupport::TestCase # -- the port-80 one only redirects to https -- and the first in the file is # the wrong one, which is what the earliest version of this helper picked up. def catch_all - blocks = VHOST.scan(/\n location \/ \{\n.*?\n \}\n/m) + blocks = VHOST.scan(%r{\n location / \{\n.*?\n \}\n}m) proxying = blocks.select { |b| b.include?('proxy_pass') } assert_equal 1, proxying.length, @@ -43,19 +43,30 @@ def directives # does not quietly stop working the day someone reformats the file. def server_level_directives depth = 0 - in_location = nil + location_depth = nil VHOST.lines.filter_map do |line| - stripped = line.strip - opening = stripped.end_with?('{') - in_location = depth if opening && stripped.start_with?('location', 'if (') - keep = depth == 1 && in_location.nil? && !opening && !stripped.start_with?('#') - depth += line.count('{') - line.count('}') - in_location = nil if in_location && depth <= in_location + keep = depth == 1 && location_depth.nil? && directive?(line) + depth, location_depth = advance(line, depth, location_depth) keep ? line : nil end end + def directive?(line) + stripped = line.strip + !stripped.empty? && !stripped.start_with?('#') && !stripped.end_with?('{') + end + + def advance(line, depth, location_depth) + stripped = line.strip + opens_location = stripped.end_with?('{') && stripped.start_with?('location', 'if (') + location_depth = depth if location_depth.nil? && opens_location + + depth += line.count('{') - line.count('}') + location_depth = nil if location_depth && depth <= location_depth + [depth, location_depth] + end + test 'the page cache exists at all' do assert_includes directives, 'proxy_cache openipc_micro', 'the largest class of traffic on the site is uncached again'