From 6095416a65366197b4ffc441d8749d57e779597a Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Sat, 15 Aug 2026 21:23:09 -0400 Subject: [PATCH 1/6] fix(levelcode): bound the OAuth calls, and ship HSTS + a Secure session cookie MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects found while investigating customer reports of sign-in hanging on mobile. None of them is the whole story — the leading suspect for the 5G-specific part is that levelcode.ai has no AAAA record while every other host in the flow does — but all three are real, verified, and worth fixing on their own. 1. NO TIMEOUTS ON ANY PROVIDER CALL `ProviderOAuth.perform_http` set none, so every call inherited Net::HTTP's 60-second defaults. A Google sign-in makes two back to back (token exchange, then the id_token key fetch), so one stalled provider could hold the browser on a blank spinner for ~2 minutes before the callback gave up and redirected to /ai/login?error=oauth_failed. Now 5s connect / 10s read / 10s write — worst case ~30s for a sign-in instead of ~120s. Failing in seconds with an error the user can retry beats succeeding on the rare 30-second call, because a spinner with no end is the one outcome a user cannot recover from. The existing `rescue StandardError` already covers Net::OpenTimeout and Net::ReadTimeout, so a timeout still becomes nil -> an error page, never a 500. That is now pinned. 2. THE SESSION COOKIE WAS NOT `Secure`, AND THERE WAS NO HSTS Both verified on the wire before this change: `path=/; httponly; samesite=lax`, no `secure`, and no Strict-Transport-Security header at all. `config.force_ssl` was false. force_ssl is now true, but with `redirect: false` — deliberately. That middleware does three jobs and we want only two of them: * the ALB already redirects (http://thin.ly 301s, http://levelcode.ai 302s), and * the ALB health check hits /up over PLAIN HTTP while .ebextensions pins MatcherHTTPCode: "200". A redirect there is a 301, every instance goes unhealthy, and the site is down. `assume_ssl = true` should already make the redirect unreachable, and I verified ActionDispatch::AssumeSSL is inserted BEFORE ActionDispatch::SSL — but that is not a thing to bet an outage on, so the redirect is disabled explicitly rather than relied upon never to fire. Both halves are covered by specs, including one that reproduces the outage by turning the option back on. HSTS ships deliberately SHORT: 1 week, no subdomains, no preload. A browser honours it for the full max-age with no way to withdraw it early. Raise it once a week has passed clean. 3. THE SESSION COOKIE IS EXPLICIT ABOUT SameSite NOW `same_site: :lax` was already the Rails default; it is stated so nobody "hardens" it to :strict. Google's redirect back to /ai/auth/callback is a cross-site top-level GET, :strict withholds the cookie on exactly that request, and every sign-in would then fail with `session_expired`. `secure:` is gated on Rails.env.production? — unconditional would mean the cookie is never sent over http://localhost and development could not hold a session. 14 bypasses, each verified by reverting the fix: every timeout individually, the 60s default restored, the rescue narrowed so a timeout escapes as a 500, the redirect turned back on, force_ssl reverted, secure_cookies off, HSTS jumped to a year / all subdomains / preload, assume_ssl off (which makes the whole change inert), the cookie made unconditionally secure, and SameSite hardened to strict. Also fixed a spec my change broke: auth_spec's instance_double(Net::HTTP) permitted only `use_ssl=` and `request`, so it rejected the new setters. The double now allows them; the VALUES stay asserted in provider_oauth_spec. NOT fixed here: the session cookie is still named `_your_app_session` (the Rails template default). Renaming it logs out every signed-in user, which does not belong in a security patch. 1030 examples, 0 failures. --- app/services/levelcode/provider_oauth.rb | 17 +++ config/application.rb | 15 ++- config/environments/production.rb | 32 +++++- spec/config/ssl_config_spec.rb | 106 ++++++++++++++++++ spec/requests/api/levelcode/v1/auth_spec.rb | 6 + .../services/levelcode/provider_oauth_spec.rb | 47 ++++++++ 6 files changed, 216 insertions(+), 7 deletions(-) create mode 100644 spec/config/ssl_config_spec.rb diff --git a/app/services/levelcode/provider_oauth.rb b/app/services/levelcode/provider_oauth.rb index d1410769..4ce2d3df 100644 --- a/app/services/levelcode/provider_oauth.rb +++ b/app/services/levelcode/provider_oauth.rb @@ -23,6 +23,20 @@ module Levelcode module ProviderOAuth module_function + # Every provider call goes through perform_http, and until now NONE of them set a timeout — so each + # one inherited Net::HTTP's 60-second defaults. A Google sign-in makes two of these back to back + # (token exchange, then the id_token verification's key fetch), so one stalled provider could hold + # the browser on a blank spinner for around two minutes before the callback finally gave up and + # redirected to /ai/login?error=oauth_failed. Reported as "sign-in is stuck loading". + # + # These are sized against what the endpoints actually do — a token exchange is a single small POST + # to Google or GitHub, not a slow query. Failing in seconds and showing the user an error they can + # retry beats succeeding on the rare 30-second call, because a spinner with no end is the one + # outcome from which a user cannot recover on their own. + OPEN_TIMEOUT = 5 # seconds to establish the TCP+TLS connection + READ_TIMEOUT = 10 # seconds to wait for the response body + WRITE_TIMEOUT = 10 # seconds to send the request body + GITHUB_PROVIDER = "github" GITHUB_AUTHORIZE_URL = "https://github.com/login/oauth/authorize" @@ -269,6 +283,9 @@ def post_form(url, form, headers: {}) def perform_http(uri, req) http = Net::HTTP.new(uri.host, uri.port) http.use_ssl = (uri.scheme == "https") + http.open_timeout = OPEN_TIMEOUT + http.read_timeout = READ_TIMEOUT + http.write_timeout = WRITE_TIMEOUT res = http.request(req) res.is_a?(Net::HTTPSuccess) ? res.body : nil rescue StandardError => e diff --git a/config/application.rb b/config/application.rb index e495405e..4484a709 100644 --- a/config/application.rb +++ b/config/application.rb @@ -22,7 +22,20 @@ module Backend class Application < Rails::Application # Use cookies for session store config.middleware.use ActionDispatch::Cookies - config.middleware.use ActionDispatch::Session::CookieStore, key: "_your_app_session" + # `secure` at the store as well as via ssl_options in production: defence in depth, and it keeps the + # attribute visible here rather than only as a side effect of force_ssl three files away. Gated on + # the environment because an unconditional `secure: true` means the cookie is never sent over + # http://localhost — development and the specs would silently stop being able to hold a session. + # + # same_site MUST STAY :lax. It is the Rails default, so it is stated explicitly to stop anyone + # "hardening" it to :strict — the Google OAuth callback is a cross-site top-level GET, :strict + # withholds the cookie on exactly that request, and the sign-in then dies with `session_expired` + # because the `state` stashed in the session never comes back. See Levelcode::WebController. + config.middleware.use ActionDispatch::Session::CookieStore, + key: "_your_app_session", + secure: Rails.env.production?, + same_site: :lax, + httponly: true config.api_only = true # Initialize configuration defaults for originally generated Rails version. diff --git a/config/environments/production.rb b/config/environments/production.rb index 347d11dd..ff313523 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -48,12 +48,32 @@ # Can be used together with config.force_ssl for Strict-Transport-Security and secure cookies. config.assume_ssl = true - # Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. - # Disabled for CloudFront - CloudFront handles SSL termination - config.force_ssl = false - - # Skip http-to-https redirect for the default health check endpoint. - # config.ssl_options = { redirect: { exclude: ->(request) { request.path == "/up" } } } + # Strict-Transport-Security + secure cookies, WITHOUT the http->https redirect. + # + # `force_ssl` turns on ActionDispatch::SSL, which does three separate jobs: redirect, HSTS, and + # flagging cookies `secure`. We want the last two and specifically NOT the first: + # + # * The ALB already redirects — verified: http://thin.ly 301s and http://levelcode.ai 302s. + # * The ALB health check hits `/up` over PLAIN HTTP on the instance, and .ebextensions pins + # `MatcherHTTPCode: "200"`. A redirect there is a 301, every instance goes unhealthy, and the + # site is down. `assume_ssl` above should make the redirect unreachable anyway (it marks every + # request as SSL), but "should" is not a thing to bet an outage on, so the redirect is turned + # off explicitly rather than relied upon to never fire. + # + # Until now this was `force_ssl = false`, so the session cookie shipped WITHOUT `Secure` — it went + # in cleartext on any plain-HTTP request — and no HSTS header was sent at all. Both verified on the + # wire before this change. + # + # HSTS starts deliberately SHORT. A browser honours it for the full max-age and there is no way to + # take it back early, so this is a week rather than the Rails default of a year. Once a week has + # passed with no plain-HTTP breakage, raise `expires` to 1.year — and only then consider + # `subdomains: true`, which would commit every present and future subdomain to HTTPS at once. + config.force_ssl = true + config.ssl_options = { + redirect: false, + secure_cookies: true, + hsts: { expires: 1.week, subdomains: false, preload: false } + } # Log to STDOUT by default config.logger = ActiveSupport::Logger.new(STDOUT) diff --git a/spec/config/ssl_config_spec.rb b/spec/config/ssl_config_spec.rb new file mode 100644 index 00000000..08edb833 --- /dev/null +++ b/spec/config/ssl_config_spec.rb @@ -0,0 +1,106 @@ +require "rails_helper" + +# `config.force_ssl = true` switches on ActionDispatch::SSL, which does THREE separate jobs: redirect +# http->https, send Strict-Transport-Security, and flag cookies `Secure`. This app wants the last two +# and specifically not the first, because the ALB health check reaches the instance over plain HTTP. +# +# Getting that wrong is not a subtle bug: `.ebextensions/03_healthcheck.config` pins +# `MatcherHTTPCode: "200"`, so a redirect on /up is a 301, every instance is marked unhealthy, and the +# site goes down. These specs exist because that is a one-word mistake with an outage attached. +RSpec.describe "production SSL configuration" do + PRODUCTION_RB = Rails.root.join("config/environments/production.rb").freeze + HEALTHCHECK_CONFIG = Rails.root.join(".ebextensions/03_healthcheck.config").freeze + + let(:production_source) { File.read(PRODUCTION_RB) } + + # Rack 3 downcases response header names. Looking only for "Location" would make the no-redirect + # assertion pass whether or not a redirect happened — it was written that way first, and the + # deliberately-failing companion spec below is what exposed it. + def location(headers) + headers["location"] || headers["Location"] + end + + describe "the health check cannot be redirected" do + # The mechanism itself, exercised rather than assumed: with `redirect: false`, a plain-HTTP request + # passes straight through instead of being bounced to https. + it "passes a plain-HTTP /up through untouched" do + inner = ->(_env) { [200, { "Content-Type" => "text/plain" }, ["ok"]] } + ssl = ActionDispatch::SSL.new(inner, redirect: false, secure_cookies: true, + hsts: { expires: 1.week, subdomains: false, preload: false }) + + status, headers, _body = ssl.call(Rack::MockRequest.env_for("http://levelcode.ai/up")) + + expect(status).to eq(200), "the ALB matcher pins 200; a redirect here marks every instance unhealthy" + expect(location(headers)).to be_nil + end + + # …and the contrast, so the spec above is not passing for some unrelated reason: the SAME request + # WOULD be redirected if the option were flipped back on. This is the outage, reproduced. + it "WOULD redirect it if `redirect` were ever turned back on" do + inner = ->(_env) { [200, {}, ["ok"]] } + ssl = ActionDispatch::SSL.new(inner, redirect: {}, secure_cookies: true, hsts: { expires: 1.week }) + + status, headers, _body = ssl.call(Rack::MockRequest.env_for("http://levelcode.ai/up")) + + expect(status).to eq(301) + expect(location(headers)).to start_with("https://") + end + + it "still needs to care, because the ALB matcher accepts only a 200" do + # If the health check were ever loosened to accept 3xx, the guards above would be belt-and-braces + # rather than load-bearing — worth knowing, so the two files are read together. + health = File.read(HEALTHCHECK_CONFIG) + expect(health).to include('MatcherHTTPCode: "200"') + expect(health).to include("HealthCheckPath: /up") + end + end + + describe "what production actually declares" do + it "keeps the redirect off" do + expect(production_source).to match(/redirect:\s*false/), + "ActionDispatch::SSL would redirect the plain-HTTP health check" + end + + it "turns on the two things we DO want from force_ssl" do + expect(production_source).to match(/config\.force_ssl\s*=\s*true/) + expect(production_source).to match(/secure_cookies:\s*true/) + expect(production_source).to match(/hsts:\s*\{/) + end + + it "keeps HSTS conservative until it has been proven in production" do + # A browser honours HSTS for the full max-age and there is no way to withdraw it early, so this + # ships short and narrow on purpose. Raising `expires` later is a deliberate act; discovering a + # year-long commitment after the fact is not. + expect(production_source).to match(/expires:\s*1\.week/), + "raise this deliberately once a week has passed with no breakage" + expect(production_source).to match(/subdomains:\s*false/), + "subdomains: true commits every present AND future subdomain to HTTPS at once" + expect(production_source).to match(/preload:\s*false/), + "preload is effectively permanent — it is baked into browser binaries" + end + + it "still assumes SSL behind the load balancer" do + # Without this, request.ssl? is false for every proxied request and ActionDispatch::SSL would + # never apply HSTS or the Secure flag at all — the change would be silently inert. + expect(production_source).to match(/config\.assume_ssl\s*=\s*true/) + end + end + + describe "the session cookie" do + let(:application_source) { File.read(Rails.root.join("config/application.rb")) } + + it "is Secure in production and not in development" do + # An unconditional `secure: true` means the cookie is never sent over http://localhost, so nobody + # can hold a session in development or in the specs. + expect(application_source).to match(/secure:\s*Rails\.env\.production\?/) + end + + it "stays SameSite=Lax, because the OAuth callback depends on it" do + # :strict withholds the cookie on cross-site top-level GETs — which is exactly what Google's + # redirect back to /ai/auth/callback is. The `state` stashed in the session would not come back + # and every sign-in would fail with `session_expired`. + expect(application_source).to match(/same_site:\s*:lax/) + expect(application_source).not_to match(/same_site:\s*:strict/) + end + end +end diff --git a/spec/requests/api/levelcode/v1/auth_spec.rb b/spec/requests/api/levelcode/v1/auth_spec.rb index 1a2dffc0..0be19f1f 100644 --- a/spec/requests/api/levelcode/v1/auth_spec.rb +++ b/spec/requests/api/levelcode/v1/auth_spec.rb @@ -234,6 +234,12 @@ def json http = instance_double(Net::HTTP) allow(Net::HTTP).to receive(:new).and_return(http) allow(http).to receive(:use_ssl=) + # perform_http now bounds every provider call (provider_oauth.rb: OPEN/READ/WRITE_TIMEOUT). + # The double has to permit the setters or it rejects the very calls that stop a stalled + # provider hanging the browser; the timeout VALUES are asserted in provider_oauth_spec.rb. + allow(http).to receive(:open_timeout=) + allow(http).to receive(:read_timeout=) + allow(http).to receive(:write_timeout=) allow(http).to receive(:request) do |req| case req.path when '/login/oauth/access_token' then token_res diff --git a/spec/services/levelcode/provider_oauth_spec.rb b/spec/services/levelcode/provider_oauth_spec.rb index e45a4347..9f1f6c41 100644 --- a/spec/services/levelcode/provider_oauth_spec.rb +++ b/spec/services/levelcode/provider_oauth_spec.rb @@ -116,4 +116,51 @@ def sign_in_with_google expect(Rails.logger).to have_received(:error).with(/google sign-up rejected by validation/i) end end + # Until this was added, perform_http set NO timeouts, so every provider call inherited Net::HTTP's + # 60-second defaults. A Google sign-in makes two of them back to back, so one stalled provider held + # the browser on a blank spinner for ~2 minutes before the callback gave up. Reported by customers as + # "sign-in is stuck loading". + describe "outbound provider HTTP is bounded" do + # perform_http is private and every provider call funnels through it, so pinning it here covers the + # Google token exchange, the id_token key fetch, and both GitHub calls at once. + def perform(http_double) + allow(Net::HTTP).to receive(:new).and_return(http_double) + allow(http_double).to receive(:use_ssl=) + described_class.send(:perform_http, URI.parse("https://oauth2.googleapis.com/token"), double("req")) + end + + it "sets a connect, read AND write timeout on every call" do + http = double("http") + allow(http).to receive(:request).and_return(double("res")) + expect(http).to receive(:open_timeout=).with(described_class::OPEN_TIMEOUT) + expect(http).to receive(:read_timeout=).with(described_class::READ_TIMEOUT) + expect(http).to receive(:write_timeout=).with(described_class::WRITE_TIMEOUT) + perform(http) + end + + it "keeps the timeouts short enough that a stalled provider cannot outlast a user's patience" do + # The failure mode being prevented is a spinner with no end, so the bound that matters is the + # WORST CASE for one sign-in: two sequential calls, each able to burn connect + read. + worst_case = 2 * (described_class::OPEN_TIMEOUT + described_class::READ_TIMEOUT) + expect(worst_case).to be <= 40 + expect(described_class::OPEN_TIMEOUT).to be >= 2 # not so tight that a slow TLS handshake fails + end + + it "turns a timeout into nil rather than an exception, so the callback can show an error" do + # perform_http returning nil is what makes oauth_callback redirect to /ai/login?error=oauth_failed. + # If a timeout escaped instead, the user would get a 500 — still broken, but now unexplained. + http = double("http") + allow(http).to receive(:use_ssl=) + allow(http).to receive(:open_timeout=) + allow(http).to receive(:read_timeout=) + allow(http).to receive(:write_timeout=) + allow(http).to receive(:request).and_raise(Net::ReadTimeout) + allow(Net::HTTP).to receive(:new).and_return(http) + + expect { + expect(described_class.send(:perform_http, URI.parse("https://oauth2.googleapis.com/token"), double("req"))).to be_nil + }.not_to raise_error + end + end + end From d9b13f7a52b81435faeab1b6fb4bf432182c6a9c Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Fri, 2 Oct 2026 23:46:52 -0400 Subject: [PATCH 2/6] test(levelcode): pin the OAuth HTTP contract against a real socket, not a double MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spec-only. The three examples for perform_http set expectations on a double's setters — open_timeout=, read_timeout=, write_timeout= — which pins how the call is spelled rather than what it does, and could not survive the call being written any other way. One of them also derived a "worst case" from the constants themselves. They are replaced by examples that run perform_http against a local TCP server: a 2xx returns its body, any other status returns nil, and a provider that accepts the connection and never answers is given up on — returning nil, inside a clock bound. The clock is the point: without a read timeout the call still returns nil, after Net::HTTP's default minute. Verified against the current, unrefactored code: 12 examples, 0 failures, 0.4s. Then broken three ways: the rescue narrowed so a timeout escapes -> the stall example; the status ignored -> the non-2xx example; the read timeout removed -> the stall example, after exactly 60.02 seconds, which is the reported hang itself. Also clears the rubocop offence CI flagged in this file. --- .../services/levelcode/provider_oauth_spec.rb | 118 +++++++++++------- 1 file changed, 73 insertions(+), 45 deletions(-) diff --git a/spec/services/levelcode/provider_oauth_spec.rb b/spec/services/levelcode/provider_oauth_spec.rb index 9f1f6c41..ebb8c9a1 100644 --- a/spec/services/levelcode/provider_oauth_spec.rb +++ b/spec/services/levelcode/provider_oauth_spec.rb @@ -116,51 +116,79 @@ def sign_in_with_google expect(Rails.logger).to have_received(:error).with(/google sign-up rejected by validation/i) end end - # Until this was added, perform_http set NO timeouts, so every provider call inherited Net::HTTP's - # 60-second defaults. A Google sign-in makes two of them back to back, so one stalled provider held - # the browser on a blank spinner for ~2 minutes before the callback gave up. Reported by customers as - # "sign-in is stuck loading". - describe "outbound provider HTTP is bounded" do - # perform_http is private and every provider call funnels through it, so pinning it here covers the - # Google token exchange, the id_token key fetch, and both GitHub calls at once. - def perform(http_double) - allow(Net::HTTP).to receive(:new).and_return(http_double) - allow(http_double).to receive(:use_ssl=) - described_class.send(:perform_http, URI.parse("https://oauth2.googleapis.com/token"), double("req")) - end - - it "sets a connect, read AND write timeout on every call" do - http = double("http") - allow(http).to receive(:request).and_return(double("res")) - expect(http).to receive(:open_timeout=).with(described_class::OPEN_TIMEOUT) - expect(http).to receive(:read_timeout=).with(described_class::READ_TIMEOUT) - expect(http).to receive(:write_timeout=).with(described_class::WRITE_TIMEOUT) - perform(http) - end - - it "keeps the timeouts short enough that a stalled provider cannot outlast a user's patience" do - # The failure mode being prevented is a spinner with no end, so the bound that matters is the - # WORST CASE for one sign-in: two sequential calls, each able to burn connect + read. - worst_case = 2 * (described_class::OPEN_TIMEOUT + described_class::READ_TIMEOUT) - expect(worst_case).to be <= 40 - expect(described_class::OPEN_TIMEOUT).to be >= 2 # not so tight that a slow TLS handshake fails - end - - it "turns a timeout into nil rather than an exception, so the callback can show an error" do - # perform_http returning nil is what makes oauth_callback redirect to /ai/login?error=oauth_failed. - # If a timeout escaped instead, the user would get a 500 — still broken, but now unexplained. - http = double("http") - allow(http).to receive(:use_ssl=) - allow(http).to receive(:open_timeout=) - allow(http).to receive(:read_timeout=) - allow(http).to receive(:write_timeout=) - allow(http).to receive(:request).and_raise(Net::ReadTimeout) - allow(Net::HTTP).to receive(:new).and_return(http) - - expect { - expect(described_class.send(:perform_http, URI.parse("https://oauth2.googleapis.com/token"), double("req"))).to be_nil - }.not_to raise_error + + # perform_http carries the GitHub token exchange and API reads, and the Google token exchange. + # (Google's signing keys are fetched by the googleauth gem itself, not through here.) These run it + # against a real local socket rather than a double, so they describe what a provider would see and + # hold however the Net::HTTP call happens to be written. + describe "talking to a provider" do + # A local stand-in for a provider. Given a response it answers each request with it; given none + # it accepts the connection and never answers — the failure the timeouts exist for. + def with_provider(response = nil) + server = TCPServer.new("127.0.0.1", 0) + connections = [] + listener = Thread.new do + loop do + socket = server.accept + connections << socket + next unless response + + socket.gets("\r\n\r\n") # the request head — the answered examples send no body + socket.write(response) + socket.close + end + end + yield URI("http://127.0.0.1:#{server.addr[1]}/token"), connections + ensure + listener&.kill + connections&.each { |socket| socket.close unless socket.closed? } + server&.close + end + + def http_response(status, body) + "HTTP/1.1 #{status}\r\nContent-Length: #{body.bytesize}\r\nConnection: close\r\n\r\n#{body}" + end + + def perform(uri, request) + described_class.send(:perform_http, uri, request) + end + + # A stall should cost the suite a fraction of a second, not the ten a real sign-in is allowed. + def with_read_timeout(seconds) + stub_const("#{described_class}::READ_TIMEOUT", seconds) end - end + it "returns the body of a successful response" do + with_provider(http_response("200 OK", '{"access_token":"t"}')) do |uri| + expect(perform(uri, Net::HTTP::Get.new(uri))).to eq('{"access_token":"t"}') + end + end + + it "returns nil for any other status, so the caller can fail the sign-in" do + with_provider(http_response("503 Service Unavailable", "busy")) do |uri| + expect(perform(uri, Net::HTTP::Get.new(uri))).to be_nil + end + end + + # nil rather than an exception is what sends the callback to /ai/login?error=oauth_failed — an + # error the user can retry — instead of a 500. The clock is what proves the timeout was applied: + # without one, this still returns nil, after Net::HTTP's default sixty seconds. + it "gives up on a provider that accepts the connection and never answers" do + with_read_timeout(0.2) + with_provider do |uri| + request = Net::HTTP::Post.new(uri).tap { |post| post.set_form_data(code: "c") } + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + + expect(perform(uri, request)).to be_nil + expect(Process.clock_gettime(Process::CLOCK_MONOTONIC) - started).to be < 5 + end + end + + # Changing one of these is a decision about how long a person waits on a spinner. This makes it + # a visible one. + it "allows a call five seconds to connect and ten to answer" do + expect([ described_class::OPEN_TIMEOUT, described_class::READ_TIMEOUT, described_class::WRITE_TIMEOUT ]) + .to eq([ 5, 10, 10 ]) + end + end end From f27de11643324d54ed6afed4ff17a8afdbad3715 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Fri, 2 Oct 2026 23:47:30 -0400 Subject: [PATCH 3/6] refactor(levelcode): the OAuth call is one Net::HTTP.start with an options hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No behaviour change: the same three timeouts, applied to the same calls. The real-socket examples from the previous commit pass against this untouched — that file's only edits are the body of the helper that shrinks the read timeout, and the example that names the constant. Three constants and three setter calls become one frozen hash handed to Net::HTTP.start, which is how the standard library means to be configured, and whose block form closes the connection when the request is done rather than leaving it to the finaliser. It also removes a cost the original change had to pay elsewhere: auth_spec's double had to grow three `allow(...=)` lines just to tolerate the setters. It now stands in for the session Net::HTTP.start yields and needs to answer `request` and nothing else. One difference on the wire, stated rather than left to be found: an unstarted Net::HTTP#request adds `Connection: close`; a started session does not, and closes the connection itself when the block returns. Each call still uses its own connection. Verified: provider_oauth 12, auth and web request specs 67 — 79 examples, 0 failures. --- app/services/levelcode/provider_oauth.rb | 28 ++++++------------- spec/requests/api/levelcode/v1/auth_spec.rb | 9 +----- .../services/levelcode/provider_oauth_spec.rb | 5 ++-- 3 files changed, 12 insertions(+), 30 deletions(-) diff --git a/app/services/levelcode/provider_oauth.rb b/app/services/levelcode/provider_oauth.rb index 4ce2d3df..08612997 100644 --- a/app/services/levelcode/provider_oauth.rb +++ b/app/services/levelcode/provider_oauth.rb @@ -23,19 +23,12 @@ module Levelcode module ProviderOAuth module_function - # Every provider call goes through perform_http, and until now NONE of them set a timeout — so each - # one inherited Net::HTTP's 60-second defaults. A Google sign-in makes two of these back to back - # (token exchange, then the id_token verification's key fetch), so one stalled provider could hold - # the browser on a blank spinner for around two minutes before the callback finally gave up and - # redirected to /ai/login?error=oauth_failed. Reported as "sign-in is stuck loading". - # - # These are sized against what the endpoints actually do — a token exchange is a single small POST - # to Google or GitHub, not a slow query. Failing in seconds and showing the user an error they can - # retry beats succeeding on the rare 30-second call, because a spinner with no end is the one - # outcome from which a user cannot recover on their own. - OPEN_TIMEOUT = 5 # seconds to establish the TCP+TLS connection - READ_TIMEOUT = 10 # seconds to wait for the response body - WRITE_TIMEOUT = 10 # seconds to send the request body + # How long a provider is given before a sign-in gives up on it. Net::HTTP allows sixty seconds for + # each of these by default, and a stalled provider then holds the browser on a blank spinner — + # the one outcome a user cannot recover from alone. A token exchange is a single small request, + # so seconds are enough: failing fast with an error the user can retry beats succeeding on the + # rare slow call. + HTTP_OPTIONS = { open_timeout: 5, read_timeout: 10, write_timeout: 10 }.freeze GITHUB_PROVIDER = "github" @@ -281,12 +274,9 @@ def post_form(url, form, headers: {}) end def perform_http(uri, req) - http = Net::HTTP.new(uri.host, uri.port) - http.use_ssl = (uri.scheme == "https") - http.open_timeout = OPEN_TIMEOUT - http.read_timeout = READ_TIMEOUT - http.write_timeout = WRITE_TIMEOUT - res = http.request(req) + res = Net::HTTP.start(uri.host, uri.port, use_ssl: uri.scheme == "https", **HTTP_OPTIONS) do |http| + http.request(req) + end res.is_a?(Net::HTTPSuccess) ? res.body : nil rescue StandardError => e Rails.logger.warn("Levelcode OAuth HTTP error: #{e.message}") diff --git a/spec/requests/api/levelcode/v1/auth_spec.rb b/spec/requests/api/levelcode/v1/auth_spec.rb index b7b201fb..a59080a4 100644 --- a/spec/requests/api/levelcode/v1/auth_spec.rb +++ b/spec/requests/api/levelcode/v1/auth_spec.rb @@ -304,14 +304,7 @@ def json emails_res = instance_double(Net::HTTPOK, body: [ { email: 'octo@example.com', primary: true, verified: true } ].to_json) allow(emails_res).to receive(:is_a?).with(Net::HTTPSuccess).and_return(true) http = instance_double(Net::HTTP) - allow(Net::HTTP).to receive(:new).and_return(http) - allow(http).to receive(:use_ssl=) - # perform_http now bounds every provider call (provider_oauth.rb: OPEN/READ/WRITE_TIMEOUT). - # The double has to permit the setters or it rejects the very calls that stop a stalled - # provider hanging the browser; the timeout VALUES are asserted in provider_oauth_spec.rb. - allow(http).to receive(:open_timeout=) - allow(http).to receive(:read_timeout=) - allow(http).to receive(:write_timeout=) + allow(Net::HTTP).to receive(:start) { |*, &session| session.call(http) } allow(http).to receive(:request) do |req| case req.path when '/login/oauth/access_token' then token_res diff --git a/spec/services/levelcode/provider_oauth_spec.rb b/spec/services/levelcode/provider_oauth_spec.rb index ebb8c9a1..5e1b2e0c 100644 --- a/spec/services/levelcode/provider_oauth_spec.rb +++ b/spec/services/levelcode/provider_oauth_spec.rb @@ -155,7 +155,7 @@ def perform(uri, request) # A stall should cost the suite a fraction of a second, not the ten a real sign-in is allowed. def with_read_timeout(seconds) - stub_const("#{described_class}::READ_TIMEOUT", seconds) + stub_const("#{described_class}::HTTP_OPTIONS", described_class::HTTP_OPTIONS.merge(read_timeout: seconds)) end it "returns the body of a successful response" do @@ -187,8 +187,7 @@ def with_read_timeout(seconds) # Changing one of these is a decision about how long a person waits on a spinner. This makes it # a visible one. it "allows a call five seconds to connect and ten to answer" do - expect([ described_class::OPEN_TIMEOUT, described_class::READ_TIMEOUT, described_class::WRITE_TIMEOUT ]) - .to eq([ 5, 10, 10 ]) + expect(described_class::HTTP_OPTIONS).to eq(open_timeout: 5, read_timeout: 10, write_timeout: 10) end end end From 67f120dca80ae5f0d925d3d6950f8de7adfc3e82 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Fri, 2 Oct 2026 23:48:19 -0400 Subject: [PATCH 4/6] fix(levelcode): a timed-out GET was silently retried, doubling the bound it was given MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Net::HTTP's max_retries defaults to 1: an idempotent request that times out is retried once, on a fresh connection, which pays the connect timeout again as well. So the timeouts this PR introduced bounded a POST at 15 seconds — and a GET at 30. Measured against a local server that accepts and never answers, read_timeout 0.5s: GET, default -> Net::ReadTimeout after 1.01s, 2 connections POST, default -> Net::ReadTimeout after 0.50s, 1 connection GET, max_retries: 0 -> Net::ReadTimeout after 0.50s, 1 connection A GitHub sign-in is one POST and two GETs (token, profile, emails), so its worst case was 15 + 30 + 30 = 75 seconds, not the ~30 the PR description gives. With retries off it is three calls of 15. The retry earns nothing here. It exists for a reused keep-alive connection that went stale; every call in this module opens its own. What it does do is double the time a person watches a spinner, on exactly the failure this change is about. The token exchange — the call a sign-in cannot do without — was already single-attempt, being a POST. The example counts connections rather than timing anything, so it cannot flake: one connection, not two. Reverting the option fails it, and the example that pins the numbers. This is a behaviour change, kept in its own commit so it can be judged — or reverted — alone. --- app/services/levelcode/provider_oauth.rb | 6 +++++- spec/services/levelcode/provider_oauth_spec.rb | 15 +++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/app/services/levelcode/provider_oauth.rb b/app/services/levelcode/provider_oauth.rb index 08612997..8dab1af8 100644 --- a/app/services/levelcode/provider_oauth.rb +++ b/app/services/levelcode/provider_oauth.rb @@ -28,7 +28,11 @@ module ProviderOAuth # the one outcome a user cannot recover from alone. A token exchange is a single small request, # so seconds are enough: failing fast with an error the user can retry beats succeeding on the # rare slow call. - HTTP_OPTIONS = { open_timeout: 5, read_timeout: 10, write_timeout: 10 }.freeze + # + # max_retries is part of the bound, not a detail: Net::HTTP silently retries an idempotent request + # once when it times out, so without this every GET here — the GitHub profile and email reads — + # is allowed twice as long as the numbers beside it say. + HTTP_OPTIONS = { open_timeout: 5, read_timeout: 10, write_timeout: 10, max_retries: 0 }.freeze GITHUB_PROVIDER = "github" diff --git a/spec/services/levelcode/provider_oauth_spec.rb b/spec/services/levelcode/provider_oauth_spec.rb index 5e1b2e0c..14872413 100644 --- a/spec/services/levelcode/provider_oauth_spec.rb +++ b/spec/services/levelcode/provider_oauth_spec.rb @@ -184,10 +184,21 @@ def with_read_timeout(seconds) end end + # Net::HTTP retries an idempotent request once after a timeout, on a fresh connection. Left on, + # the GitHub profile and email reads are each allowed double the timeout — which is how a bound + # advertised as fifteen seconds a call quietly becomes thirty. + it "does not retry a GET that timed out" do + with_read_timeout(0.2) + with_provider do |uri, connections| + expect(perform(uri, Net::HTTP::Get.new(uri))).to be_nil + expect(connections.size).to eq(1) + end + end + # Changing one of these is a decision about how long a person waits on a spinner. This makes it # a visible one. - it "allows a call five seconds to connect and ten to answer" do - expect(described_class::HTTP_OPTIONS).to eq(open_timeout: 5, read_timeout: 10, write_timeout: 10) + it "allows a call five seconds to connect and ten to answer, once" do + expect(described_class::HTTP_OPTIONS).to eq(open_timeout: 5, read_timeout: 10, write_timeout: 10, max_retries: 0) end end end From 3410a5a302b05b4ae0f6f2b24980327ca539e7b3 Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Fri, 2 Oct 2026 23:49:42 -0400 Subject: [PATCH 5/6] fix(levelcode): the OAuth error log names the exception and the call that failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From review. The line logged only e.message, which for a timeout is "Net::ReadTimeout with #" at best and "execution expired" at worst — neither says which provider or which of its calls. With the timeouts this PR adds, a timeout is about to become the likeliest way for this line to fire. It now carries the exception class, the HTTP method, and the host and path. Deliberately not the full URI: a query string is where a code or a token would be. The example drives a real timeout and anchors the whole line, so it fails both if the context is dropped and if a query string is ever logged — verified by doing each. --- app/services/levelcode/provider_oauth.rb | 3 ++- spec/services/levelcode/provider_oauth_spec.rb | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/services/levelcode/provider_oauth.rb b/app/services/levelcode/provider_oauth.rb index 8dab1af8..5a31229a 100644 --- a/app/services/levelcode/provider_oauth.rb +++ b/app/services/levelcode/provider_oauth.rb @@ -283,7 +283,8 @@ def perform_http(uri, req) end res.is_a?(Net::HTTPSuccess) ? res.body : nil rescue StandardError => e - Rails.logger.warn("Levelcode OAuth HTTP error: #{e.message}") + # Host and path only — a query string is where a code or a token would be. + Rails.logger.warn("Levelcode OAuth HTTP error: #{e.class}: #{e.message} (#{req.method} #{uri.host}#{uri.path})") nil end diff --git a/spec/services/levelcode/provider_oauth_spec.rb b/spec/services/levelcode/provider_oauth_spec.rb index 14872413..b276a492 100644 --- a/spec/services/levelcode/provider_oauth_spec.rb +++ b/spec/services/levelcode/provider_oauth_spec.rb @@ -195,6 +195,21 @@ def with_read_timeout(seconds) end end + # A timeout is now the likeliest way for this line to fire, and its message alone names neither + # the provider nor the call. + it "logs what failed and where: the exception, the method, the host and path — never the query" do + allow(Rails.logger).to receive(:warn) + with_read_timeout(0.2) + with_provider do |uri| + uri.query = "code=secret" + perform(uri, Net::HTTP::Get.new(uri)) + end + + expect(Rails.logger).to have_received(:warn).with( + a_string_matching(%r{\ALevelcode OAuth HTTP error: Net::ReadTimeout: .* \(GET 127\.0\.0\.1/token\)\z}) + ) + end + # Changing one of these is a decision about how long a person waits on a spinner. This makes it # a visible one. it "allows a call five seconds to connect and ten to answer, once" do From 104c37d5ac5c6433138133de6fb86c34da1ff85a Mon Sep 17 00:00:00 2001 From: Sergii Demianchuk Date: Fri, 2 Oct 2026 23:54:01 -0400 Subject: [PATCH 6/6] refactor(config): the SSL options are data, and the spec runs the real middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From review: the spec asserted that production.rb's SOURCE matched /redirect:\s*false/, so a comment containing that text satisfied it. Rather than strip comments and keep matching text, the options become a value — Backend::Application::SSL_OPTIONS — that production.rb assigns and the spec hands to ActionDispatch::SSL. The examples now describe what the middleware does: /up over plain HTTP gets its 200 and no redirect; the same request with the redirect left on is a 301; the HSTS header is exactly "max-age=604800". Doing that surfaced something the text-matching spec could not: `secure_cookies: true` did nothing. ActionDispatch::SSL only flags cookies Secure on requests it would otherwise have redirected (ssl.rb:71,84 in Rails 7.2.3), and `redirect: false` makes that no request at all. The old spec passed because it checked that the words were in the file. So: - the inert key is removed. Shipped and proposed options were run side by side over six requests and are identical in status, cookie, HSTS and location; - the comments that said the middleware flags cookies are corrected. The session cookie IS Secure in production, but only because the store sets it — that flag was described as defence in depth and is in fact the whole mechanism; - an example pins that the middleware leaves cookies alone, since the design rests on it. Whether to have the middleware flag cookies after all — `redirect: { exclude: … }` for /up, the form Rails documents — is a behaviour decision and is not made here. Two facts are decided only at a production boot and cannot be observed from the test environment: the three switches in production.rb, and the store's `secure:`. Those are still read from source, but as whole lines of code, which a comment cannot satisfy. The health-check config is parsed as the YAML it is rather than searched as a string, and Rack::MockResponse replaces a hand-rolled helper for header case. Verified: - 9 examples; rubocop clean (this file carried 8 of the 9 offences CI flagged). - Eleven mutations, each caught: the redirect left on; force_ssl reverted; assume_ssl removed; HSTS to a year, to all subdomains, to preload; the cookie unconditionally Secure; SameSite :strict; the store's flag removed; production handed other options; and the reviewer's case — the setting turned off with the old text left in a comment. - production.rb evaluated for real in a clean process (RAILS_ENV=production): assume_ssl and force_ssl true, ssl_options the same object as the constant, AssumeSSL ahead of SSL, the session store resolved to secure: true — and through that middleware pair a plain-HTTP /up returns 200 with no redirect. --- config/application.rb | 23 ++++- config/environments/production.rb | 29 +------ spec/config/ssl_config_spec.rb | 139 ++++++++++++++---------------- 3 files changed, 86 insertions(+), 105 deletions(-) diff --git a/config/application.rb b/config/application.rb index 4484a709..5dfdadfe 100644 --- a/config/application.rb +++ b/config/application.rb @@ -20,12 +20,27 @@ module Backend class Application < Rails::Application + # What ActionDispatch::SSL is given in production, where config/environments/production.rb turns + # it on. Held here as a value so the specs can run the real middleware with the real options. + # + # redirect: false — the ALB already redirects http to https, and its health check reaches /up over + # PLAIN HTTP and accepts only a 200 (.ebextensions/03_healthcheck.config). A redirect there marks + # every instance unhealthy and takes the site down. + # + # hsts — deliberately short. A browser honours it for the full max-age and it cannot be withdrawn + # early. Raise `expires` to 1.year once a week has passed clean, and only then consider + # `subdomains: true`, which commits every present and future subdomain to HTTPS at once. + SSL_OPTIONS = { + redirect: false, + hsts: { expires: 1.week, subdomains: false, preload: false } + }.freeze + # Use cookies for session store config.middleware.use ActionDispatch::Cookies - # `secure` at the store as well as via ssl_options in production: defence in depth, and it keeps the - # attribute visible here rather than only as a side effect of force_ssl three files away. Gated on - # the environment because an unconditional `secure: true` means the cookie is never sent over - # http://localhost — development and the specs would silently stop being able to hold a session. + # `secure` is set HERE, on the store, because nothing else will set it: with its redirect off, + # ActionDispatch::SSL does not flag cookies Secure — Rails skips that for any request it would not + # have redirected. Production only: unconditionally, the cookie would never be sent over + # http://localhost, and neither development nor the specs could hold a session. # # same_site MUST STAY :lax. It is the Rails default, so it is stated explicitly to stop anyone # "hardening" it to :strict — the Google OAuth callback is a cross-site top-level GET, :strict diff --git a/config/environments/production.rb b/config/environments/production.rb index ff313523..cd0436ec 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -48,32 +48,11 @@ # Can be used together with config.force_ssl for Strict-Transport-Security and secure cookies. config.assume_ssl = true - # Strict-Transport-Security + secure cookies, WITHOUT the http->https redirect. - # - # `force_ssl` turns on ActionDispatch::SSL, which does three separate jobs: redirect, HSTS, and - # flagging cookies `secure`. We want the last two and specifically NOT the first: - # - # * The ALB already redirects — verified: http://thin.ly 301s and http://levelcode.ai 302s. - # * The ALB health check hits `/up` over PLAIN HTTP on the instance, and .ebextensions pins - # `MatcherHTTPCode: "200"`. A redirect there is a 301, every instance goes unhealthy, and the - # site is down. `assume_ssl` above should make the redirect unreachable anyway (it marks every - # request as SSL), but "should" is not a thing to bet an outage on, so the redirect is turned - # off explicitly rather than relied upon to never fire. - # - # Until now this was `force_ssl = false`, so the session cookie shipped WITHOUT `Secure` — it went - # in cleartext on any plain-HTTP request — and no HSTS header was sent at all. Both verified on the - # wire before this change. - # - # HSTS starts deliberately SHORT. A browser honours it for the full max-age and there is no way to - # take it back early, so this is a week rather than the Rails default of a year. Once a week has - # passed with no plain-HTTP breakage, raise `expires` to 1.year — and only then consider - # `subdomains: true`, which would commit every present and future subdomain to HTTPS at once. + # Strict-Transport-Security, and deliberately NOT the http->https redirect: the ALB health check + # reaches /up over plain HTTP and accepts only a 200. The options, and the reason for each, are + # SSL_OPTIONS in config/application.rb — kept there so the specs can run the real middleware. config.force_ssl = true - config.ssl_options = { - redirect: false, - secure_cookies: true, - hsts: { expires: 1.week, subdomains: false, preload: false } - } + config.ssl_options = Backend::Application::SSL_OPTIONS # Log to STDOUT by default config.logger = ActiveSupport::Logger.new(STDOUT) diff --git a/spec/config/ssl_config_spec.rb b/spec/config/ssl_config_spec.rb index 08edb833..6730256a 100644 --- a/spec/config/ssl_config_spec.rb +++ b/spec/config/ssl_config_spec.rb @@ -1,106 +1,93 @@ require "rails_helper" -# `config.force_ssl = true` switches on ActionDispatch::SSL, which does THREE separate jobs: redirect -# http->https, send Strict-Transport-Security, and flag cookies `Secure`. This app wants the last two -# and specifically not the first, because the ALB health check reaches the instance over plain HTTP. +# Production sits behind an ALB that terminates TLS and talks to the instance over plain HTTP. So +# ActionDispatch::SSL is on for one job — Strict-Transport-Security — and must never do its other +# one, the http->https redirect: the ALB health check accepts nothing but a 200 from /up, and a +# redirect there marks every instance unhealthy. # -# Getting that wrong is not a subtle bug: `.ebextensions/03_healthcheck.config` pins -# `MatcherHTTPCode: "200"`, so a redirect on /up is a 301, every instance is marked unhealthy, and the -# site goes down. These specs exist because that is a one-word mistake with an outage attached. -RSpec.describe "production SSL configuration" do - PRODUCTION_RB = Rails.root.join("config/environments/production.rb").freeze - HEALTHCHECK_CONFIG = Rails.root.join(".ebextensions/03_healthcheck.config").freeze - - let(:production_source) { File.read(PRODUCTION_RB) } - - # Rack 3 downcases response header names. Looking only for "Location" would make the no-redirect - # assertion pass whether or not a redirect happened — it was written that way first, and the - # deliberately-failing companion spec below is what exposed it. - def location(headers) - headers["location"] || headers["Location"] +# These run the real middleware with the options production is given, so they describe what +# production does rather than what its config file looks like. +RSpec.describe "transport security in production" do + let(:app) { ->(_env) { [ 200, { "set-cookie" => "id=1; path=/" }, [ "ok" ] ] } } + let(:options) { Backend::Application::SSL_OPTIONS } + + def get(url, **overrides) + Rack::MockRequest.new(ActionDispatch::SSL.new(app, **options, **overrides)).get(url) end - describe "the health check cannot be redirected" do - # The mechanism itself, exercised rather than assumed: with `redirect: false`, a plain-HTTP request - # passes straight through instead of being bounced to https. - it "passes a plain-HTTP /up through untouched" do - inner = ->(_env) { [200, { "Content-Type" => "text/plain" }, ["ok"]] } - ssl = ActionDispatch::SSL.new(inner, redirect: false, secure_cookies: true, - hsts: { expires: 1.week, subdomains: false, preload: false }) + describe "the load balancer's health check" do + it "gets its 200 from /up over plain HTTP, never a redirect" do + response = get("http://levelcode.ai/up") - status, headers, _body = ssl.call(Rack::MockRequest.env_for("http://levelcode.ai/up")) - - expect(status).to eq(200), "the ALB matcher pins 200; a redirect here marks every instance unhealthy" - expect(location(headers)).to be_nil + expect(response.status).to eq(200) + expect(response.location).to be_nil end - # …and the contrast, so the spec above is not passing for some unrelated reason: the SAME request - # WOULD be redirected if the option were flipped back on. This is the outage, reproduced. - it "WOULD redirect it if `redirect` were ever turned back on" do - inner = ->(_env) { [200, {}, ["ok"]] } - ssl = ActionDispatch::SSL.new(inner, redirect: {}, secure_cookies: true, hsts: { expires: 1.week }) + # The same request with the redirect left on. This is the outage — and the reason the example + # above cannot be passing by accident. + it "would be redirected if the redirect were ever left on" do + response = get("http://levelcode.ai/up", redirect: {}) + + expect(response.status).to eq(301) + expect(response.location).to eq("https://levelcode.ai/up") + end - status, headers, _body = ssl.call(Rack::MockRequest.env_for("http://levelcode.ai/up")) + it "still accepts nothing but a 200 from /up" do + health_check = YAML.load_file(Rails.root.join(".ebextensions/03_healthcheck.config")) + .dig("option_settings", "aws:elasticbeanstalk:environment:process:default") - expect(status).to eq(301) - expect(location(headers)).to start_with("https://") + expect(health_check).to include("HealthCheckPath" => "/up", "MatcherHTTPCode" => "200") end + end - it "still needs to care, because the ALB matcher accepts only a 200" do - # If the health check were ever loosened to accept 3xx, the guards above would be belt-and-braces - # rather than load-bearing — worth knowing, so the two files are read together. - health = File.read(HEALTHCHECK_CONFIG) - expect(health).to include('MatcherHTTPCode: "200"') - expect(health).to include("HealthCheckPath: /up") + describe "Strict-Transport-Security" do + # One assertion on the whole header: a longer max-age, includeSubDomains and preload would each + # change it, and each is a commitment a browser holds us to with no way to withdraw it early. + it "is sent over HTTPS for one week, for this host only" do + expect(get("https://levelcode.ai/").headers["strict-transport-security"]).to eq("max-age=604800") end end - describe "what production actually declares" do - it "keeps the redirect off" do - expect(production_source).to match(/redirect:\s*false/), - "ActionDispatch::SSL would redirect the plain-HTTP health check" + describe "the session cookie" do + let(:session_options) do + Rails.application.middleware.find { |m| m.klass == ActionDispatch::Session::CookieStore }.args.first end - it "turns on the two things we DO want from force_ssl" do - expect(production_source).to match(/config\.force_ssl\s*=\s*true/) - expect(production_source).to match(/secure_cookies:\s*true/) - expect(production_source).to match(/hsts:\s*\{/) + # With its redirect off, ActionDispatch::SSL leaves cookies alone: Rails only flags them Secure + # on requests it would otherwise have redirected. So the store has to carry the flag itself. + it "is not made Secure by the middleware, which is why the store sets its own flag" do + expect(get("https://levelcode.ai/").headers["set-cookie"]).to eq("id=1; path=/") end - it "keeps HSTS conservative until it has been proven in production" do - # A browser honours HSTS for the full max-age and there is no way to withdraw it early, so this - # ships short and narrow on purpose. Raising `expires` later is a deliberate act; discovering a - # year-long commitment after the fact is not. - expect(production_source).to match(/expires:\s*1\.week/), - "raise this deliberately once a week has passed with no breakage" - expect(production_source).to match(/subdomains:\s*false/), - "subdomains: true commits every present AND future subdomain to HTTPS at once" - expect(production_source).to match(/preload:\s*false/), - "preload is effectively permanent — it is baked into browser binaries" + # :strict withholds the cookie on a cross-site top-level GET, which is exactly what a provider's + # redirect back to /ai/auth/callback is. The `state` stashed in the session would not come back, + # and every sign-in would fail with `session_expired`. + it "is HttpOnly, and SameSite=Lax rather than Strict" do + expect(session_options).to include(httponly: true, same_site: :lax) end - it "still assumes SSL behind the load balancer" do - # Without this, request.ssl? is false for every proxied request and ActionDispatch::SSL would - # never apply HSTS or the Secure flag at all — the change would be silently inert. - expect(production_source).to match(/config\.assume_ssl\s*=\s*true/) + it "is not Secure outside production, or it would never be sent over http://localhost" do + expect(session_options).to include(secure: false) end end - describe "the session cookie" do - let(:application_source) { File.read(Rails.root.join("config/application.rb")) } + # Two things are decided only when Rails boots in production, so nothing above can observe them. + # They are read as whole lines of code: a comment that mentions a setting is not a match. + describe "what only a production boot evaluates" do + def lines_of(path) + Rails.root.join(path).readlines.map(&:strip) + end - it "is Secure in production and not in development" do - # An unconditional `secure: true` means the cookie is never sent over http://localhost, so nobody - # can hold a session in development or in the specs. - expect(application_source).to match(/secure:\s*Rails\.env\.production\?/) + it "turns the middleware on, behind the proxy, with these options" do + expect(lines_of("config/environments/production.rb")).to include( + "config.assume_ssl = true", + "config.force_ssl = true", + "config.ssl_options = Backend::Application::SSL_OPTIONS" + ) end - it "stays SameSite=Lax, because the OAuth callback depends on it" do - # :strict withholds the cookie on cross-site top-level GETs — which is exactly what Google's - # redirect back to /ai/auth/callback is. The `state` stashed in the session would not come back - # and every sign-in would fail with `session_expired`. - expect(application_source).to match(/same_site:\s*:lax/) - expect(application_source).not_to match(/same_site:\s*:strict/) + it "marks the session cookie Secure" do + expect(lines_of("config/application.rb")).to include("secure: Rails.env.production?,") end end end