fix(levelcode): bound the OAuth calls, and ship HSTS + a Secure session cookie

app/services/levelcode/provider_oauth.rb

@@ -23,6 +23,17 @@ module Levelcode
   module ProviderOAuth
     module_function
 
+    # 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.
+    #
+    # 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"
 
     GITHUB_AUTHORIZE_URL = "https://github.com/login/oauth/authorize"
@@ -267,12 +278,13 @@ 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")
-      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}")
+      # 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
 

config/application.rb

@@ -20,9 +20,37 @@
 
 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
-    config.middleware.use ActionDispatch::Session::CookieStore, key: "_your_app_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
+    # 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.

config/environments/production.rb

@@ -48,12 +48,11 @@
   # 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, 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 = Backend::Application::SSL_OPTIONS
 
   # Log to STDOUT by default
   config.logger = ActiveSupport::Logger.new(STDOUT)

spec/config/ssl_config_spec.rb

@@ -0,0 +1,93 @@
+require "rails_helper"
+
+# 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.
+#
+# 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 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")
+
+      expect(response.status).to eq(200)
+      expect(response.location).to be_nil
+    end
+
+    # 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
+
+    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(health_check).to include("HealthCheckPath" => "/up", "MatcherHTTPCode" => "200")
+    end
+  end
+
+  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 "the session cookie" do
+    let(:session_options) do
+      Rails.application.middleware.find { |m| m.klass == ActionDispatch::Session::CookieStore }.args.first
+    end
+
+    # 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
+
+    # :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 "is not Secure outside production, or it would never be sent over http://localhost" do
+      expect(session_options).to include(secure: false)
+    end
+  end
+
+  # 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 "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 "marks the session cookie Secure" do
+      expect(lines_of("config/application.rb")).to include("secure: Rails.env.production?,")
+    end
+  end
+end

spec/requests/api/levelcode/v1/auth_spec.rb

@@ -304,8 +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=)
+        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

spec/services/levelcode/provider_oauth_spec.rb

@@ -116,4 +116,104 @@ def sign_in_with_google
       expect(Rails.logger).to have_received(:error).with(/google sign-up rejected by validation/i)
     end
   end
+
+  # 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}::HTTP_OPTIONS", described_class::HTTP_OPTIONS.merge(read_timeout: seconds))
+    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
+
+    # 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
+
+    # 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
+      expect(described_class::HTTP_OPTIONS).to eq(open_timeout: 5, read_timeout: 10, write_timeout: 10, max_retries: 0)
+    end
+  end
 end