Move the session-skip from a before filter to real Rack middleware
The before filter added in the previous commit never ran for static assets: Sinatra serves public_folder files (every JS/CSS/font/vendor file) through its own built-in shortcut, bypassing route and filter dispatch entirely — confirmed live, curl still got a fresh set-cookie on /js/map.js after that commit deployed, and the user hit the exact same logout on refresh again. A HAR from that retest shows the same clobbering pattern as before, just via a different response each time. SkipSessionWrite is a plain Rack middleware instead, positioned (via `use`, after `set :sessions`) inside Rack::Session::Cookie's own wrapping — it sees the same env static dispatch does, so every response is covered, not just Sinatra-routed ones.
This commit is contained in:
1 parent
bd46b72144
commit
b7618edc25
1 file changed
+35
-22
@@ -15,6 +15,40 @@ require_relative "lib/maptool/plugins"
|
|||||||
require_relative "lib/maptool/pmtiles_builder"
|
require_relative "lib/maptool/pmtiles_builder"
|
||||||
|
|
||||||
module Maptool
|
module Maptool
|
||||||
|
# Only /login, /auth/callback and /logout ever write to session
|
||||||
|
# (oauth_state, user, session.clear) — everything else 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, a 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.
|
||||||
|
#
|
||||||
|
# A plain Rack middleware, not a Sinatra `before` filter: static assets
|
||||||
|
# (public_folder — every JS/CSS/font/vendor file) are served by
|
||||||
|
# Sinatra's own built-in static-file shortcut, which bypasses route and
|
||||||
|
# filter dispatch entirely but is still wrapped by this, same as every
|
||||||
|
# other response — each one was independently capable of winning the
|
||||||
|
# clobbering race a `before` filter alone left uncovered.
|
||||||
|
class SkipSessionWrite
|
||||||
|
WRITE_PATHS = %w[/login /auth/callback /logout].freeze
|
||||||
|
|
||||||
|
def initialize(app)
|
||||||
|
@app = app
|
||||||
|
end
|
||||||
|
|
||||||
|
def call(env)
|
||||||
|
env["rack.session.options"][:skip] = true unless WRITE_PATHS.include?(env["PATH_INFO"])
|
||||||
|
@app.call(env)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
class App < Sinatra::Base
|
class App < Sinatra::Base
|
||||||
set :public_folder, File.join(__dir__, "public")
|
set :public_folder, File.join(__dir__, "public")
|
||||||
set :views, File.join(__dir__, "views")
|
set :views, File.join(__dir__, "views")
|
||||||
@@ -33,6 +67,7 @@ module Maptool
|
|||||||
# don't survive a restart there, which is fine outside production.
|
# don't survive a restart there, which is fine outside production.
|
||||||
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) }
|
||||||
|
use SkipSessionWrite
|
||||||
|
|
||||||
# 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
|
||||||
@@ -150,28 +185,6 @@ 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