Skip re-writing the session cookie on every response, not just the three that change it
Rack::Session::Cookie re-serializes and re-sets the cookie on every response that even reads session — Rack::Protection's CSRF check does, on nearly every request — reflecting whatever cookie that particular request happened to arrive with, whether or not anything actually changed. A HAR capture caught the real failure mode: seconds after a successful login, offline_data.js's _checkUpdate (an unrelated background poll fired the instant the page loads) was still carrying the browser's pre-login cookie — a client-side timing race across the OAuth redirect chain, not something this server controls — and its own re-issued Set-Cookie landed after the login's, silently overwriting session[:user] with nothing logged anywhere to explain it. Only /login, /auth/callback and /logout ever actually write to session. A before filter now skips the write everywhere else, removing every other response from that race — a stale request can still read a stale cookie, but it can no longer clobber a fresher one. Also drops the earlier except: [:session_hijacking] from the previous commit: rack-protection 4.2.1 only enables SessionHijacking when explicitly opted into via :use, so Sinatra's default setup never turned it on in the first place — the exclusion was a no-op, not the fix.
This commit is contained in:
1 parent
6126b5055b
commit
bd46b72144
1 file changed
+22
-15
@@ -34,21 +34,6 @@ module Maptool
|
|||||||
set :sessions, httponly: true, secure: production?, same_site: :lax,
|
set :sessions, httponly: true, secure: production?, same_site: :lax,
|
||||||
secret: ENV.fetch("SESSION_SECRET") { SecureRandom.hex(32) }
|
secret: ENV.fetch("SESSION_SECRET") { SecureRandom.hex(32) }
|
||||||
|
|
||||||
# Sinatra enables Rack::Protection::SessionHijacking by default once
|
|
||||||
# sessions are on: it fingerprints a session to the User-Agent header
|
|
||||||
# seen on the request that first wrote to it, and *silently* empties
|
|
||||||
# the session (no exception, nothing logged) on any later request
|
|
||||||
# where that header doesn't match — which is exactly what a login
|
|
||||||
# behind production's reverse proxy hit: the OAuth callback set
|
|
||||||
# session[:user] fine, but the very next ordinary page load (or even a
|
|
||||||
# plain refresh) came back logged out with nothing in the logs to
|
|
||||||
# explain it. Its own docs already admit every header it can check is
|
|
||||||
# spoofable anyway ("will not prevent determined hijacking attempts")
|
|
||||||
# — losing it is a minor tradeoff for a login that actually stays
|
|
||||||
# logged in. Every other Rack::Protection check (XSS header, path
|
|
||||||
# traversal, JSON CSRF, frame options, ...) stays on.
|
|
||||||
set :protection, except: [:session_hijacking]
|
|
||||||
|
|
||||||
# Cache-busts /js and /css requests: browsers (and a caching proxy in
|
# Cache-busts /js and /css requests: browsers (and a caching proxy in
|
||||||
# front of production, see docs/deployment.adoc) hang onto static
|
# front of production, see docs/deployment.adoc) hang onto static
|
||||||
# assets for hours, so without this a deploy that changes app.js/css
|
# assets for hours, so without this a deploy that changes app.js/css
|
||||||
@@ -165,6 +150,28 @@ module Maptool
|
|||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
# Only these three routes ever write to session (oauth_state, user,
|
||||||
|
# session.clear below) — every other route only ever reads it, if it
|
||||||
|
# touches session at all. That still matters: Rack::Session::Cookie
|
||||||
|
# re-serializes and re-sets the cookie on *every* response that so
|
||||||
|
# much as read session (Rack::Protection's own CSRF check does, on
|
||||||
|
# nearly every request) — reflecting whatever cookie that particular
|
||||||
|
# request happened to arrive with, whether or not anything changed.
|
||||||
|
# A HAR capture caught the actual failure mode this caused: seconds
|
||||||
|
# after a successful login, offline_data.js's _checkUpdate — an
|
||||||
|
# unrelated background poll fired the instant the page loads — was
|
||||||
|
# still carrying the browser's pre-login cookie (a client-side timing
|
||||||
|
# race across the OAuth redirect chain, not something this server
|
||||||
|
# controls), and its own re-issued Set-Cookie landed *after* the
|
||||||
|
# login's, silently overwriting session[:user] back out with nothing
|
||||||
|
# logged anywhere to explain it. Skipping the write everywhere except
|
||||||
|
# the routes that actually need it removes every other response from
|
||||||
|
# that race — a stale request can still be served from a stale
|
||||||
|
# cookie, but it can no longer clobber a fresher one.
|
||||||
|
before do
|
||||||
|
session.options[:skip] = true unless %w[/login /auth/callback /logout].include?(request.path_info)
|
||||||
|
end
|
||||||
|
|
||||||
get "/login" do
|
get "/login" do
|
||||||
session[:oauth_state] = SecureRandom.hex(16)
|
session[:oauth_state] = SecureRandom.hex(16)
|
||||||
redirect OAUTH_CLIENT.auth_code.authorize_url(
|
redirect OAUTH_CLIENT.auth_code.authorize_url(
|
||||||
|
|||||||
Reference in new issue
Block a user