fix(levelcode): bound the OAuth calls, and ship HSTS + a Secure session cookie - #415
Open
ndemianc wants to merge 1 commit into
Open
fix(levelcode): bound the OAuth calls, and ship HSTS + a Secure session cookie#415ndemianc wants to merge 1 commit into
ndemianc wants to merge 1 commit into
Conversation
…on cookie 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.
Contributor
There was a problem hiding this comment.
Pull request overview
Bounds Levelcode OAuth provider HTTP calls with explicit Net::HTTP timeouts and hardens production transport/session security by enabling HSTS and Secure session cookies (while explicitly avoiding an http→https redirect to keep the ALB /up health check from causing an outage).
Changes:
- Add connect/read/write timeouts to
Levelcode::ProviderOAuth.perform_http, plus specs pinning timeout behavior and timeout-to-nil handling. - Enable
config.force_sslin production withssl_optionsconfigured for HSTS + secure cookies butredirect: false, with a dedicated spec guarding the health check behavior. - Make the session cookie settings explicit (
securein production,SameSite=Lax,HttpOnly) inconfig/application.rb.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| app/services/levelcode/provider_oauth.rb | Adds bounded Net::HTTP timeouts for all provider calls to prevent long hangs. |
| config/environments/production.rb | Enables force_ssl with HSTS + secure cookies, explicitly disabling redirects. |
| config/application.rb | Makes session cookie secure (prod), SameSite=Lax, and HttpOnly explicit. |
| spec/services/levelcode/provider_oauth_spec.rb | Adds specs asserting timeouts are set and timeouts return nil (not 500). |
| spec/requests/api/levelcode/v1/auth_spec.rb | Updates Net::HTTP double to allow new timeout setters used by perform_http. |
| spec/config/ssl_config_spec.rb | Adds regression specs ensuring /up is not redirected and production config stays conservative. |
Suppressed comments (1)
app/services/levelcode/provider_oauth.rb:292
- The timeout changes will make provider errors (especially timeouts) more common, but the log line doesn’t include enough context to debug quickly (no URI and no exception class). Including both makes production triage much easier without changing behavior.
http.write_timeout = WRITE_TIMEOUT
res = http.request(req)
res.is_a?(Net::HTTPSuccess) ? res.body : nil
rescue StandardError => e
Rails.logger.warn("Levelcode OAuth HTTP error: #{e.message}")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+59
to
+62
| it "keeps the redirect off" do | ||
| expect(production_source).to match(/redirect:\s*false/), | ||
| "ActionDispatch::SSL would redirect the plain-HTTP health check" | ||
| end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.aihas no AAAA record while every other host in the flow does (including our own thin.ly), and that fix is in the load balancer, not here. All three of these are real, verified, and worth fixing on their own.1. No timeouts on any provider call
ProviderOAuth.perform_httpset none, so every call inheritedNet::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.Failing in seconds with an error the user can retry beats succeeding on the rare 30-second call — a spinner with no end is the one outcome a user cannot recover from on their own.
The existing
rescue StandardErroralready coversNet::OpenTimeout/Net::ReadTimeout, so a timeout still becomesnil→ an error page, never a 500. That's now pinned by a spec, because narrowing the rescue would silently turn every timeout into a 500.2. No
Secureon the session cookie, and no HSTS at allBoth verified on the wire before this change:
path=/; httponly; samesite=lax— nosecure— and noStrict-Transport-Securityheader.config.force_sslwasfalse.force_sslis nowtrue, withredirect: false— deliberately. That middleware does three jobs and we want only two:http://thin.ly301s,http://levelcode.ai302s)./upover plain HTTP, and.ebextensions/03_healthcheck.configpinsMatcherHTTPCode: "200". A redirect there is a 301 → every instance unhealthy → site down.assume_ssl = trueshould already make the redirect unreachable, and I verifiedActionDispatch::AssumeSSLis inserted beforeActionDispatch::SSL:But that's not a thing to bet an outage on, so the redirect is disabled explicitly rather than relied upon never to fire.
spec/config/ssl_config_spec.rbcovers both halves — including one spec that reproduces the outage by turning the option back on, so the passing spec can't be passing for an unrelated reason.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
expiresto1.yearonce a week has passed clean — and only then considersubdomains: true, which commits every present and future subdomain to HTTPS at once.3.
SameSiteis now explicit, because OAuth depends on itsame_site: :laxwas already the Rails default; it's stated so nobody "hardens" it to:strict. Google's redirect back to/ai/auth/callbackis a cross-site top-level GET —:strictwithholds the cookie on exactly that request, thestatestashed in the session never comes back, and every sign-in fails withsession_expired.secure:is gated onRails.env.production?. Unconditional would mean the cookie is never sent overhttp://localhost, so development and the specs could not hold a session.Bypasses
14, each verified by reverting the fix and confirming the failure:
redirectturned back on — the outageforce_sslrevertedsecure_cookiesoffassume_ssloff — makes the whole change inertsecureSameSitehardened to:strictOne spec my change broke
auth_spec'sinstance_double(Net::HTTP)permitted onlyuse_ssl=andrequest, so it rejected the new setters. I confirmed it was mine rather than pre-existing (21/21 pass on clean develop) before touching it. The double now allows them; the timeout values stay asserted inprovider_oauth_spec.Writing these specs also caught one of my own: Rack 3 downcases response headers, so an assertion on
headers["Location"]was passing whether or not a redirect happened. The deliberately-failing companion spec is what exposed it.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 doesn't belong in a security patch.1030 examples, 0 failures.