* Check a cable connection's session while it's open
Signing out or changing the password ends sessions and then disconnects
their connections. A connection that found its session just before isn't
subscribed to its disconnects yet, so it missed that and stayed open.
Each connection now looks its session up every minute while it's open, and
closes once the session is gone, asking the browser to reconnect so one
holding a current session comes back. That also covers any other way a
connection misses the disconnect, such as a subscription that takes long to
register, and a lookup that fails is simply tried again.
* Check the session on the server's heartbeat rather than a timer of its own
A connection's own timer could outlive it: closing can run before opening finishes, or a channel's teardown can fail before the connection's. The heartbeat only reaches connections that are open, so nothing is left to stop.
* Skip the session check on a rejected connection
A rejected connection stays registered until its socket closes, and has no session to check, so its heartbeat mustn't fail and stop the heartbeat reaching the connections after it.
(cherry picked from commit ce7b5d4ea0)
If the password changes again between this browser's change and its new session, that session is refused, and the browser is sent to sign in instead of an error.
(cherry picked from commit a476cbfa24)
A password change ends every session, but a sign-in or sign-in link checked
against the old password just before the change could still save its session
just after, and that session outlived the change.
The session now checks, as it's saved, that the user's password is still the
one the request loaded. The save's transaction holds SQLite's write lock from
the start, so a change commits either before the check, which fails it, or
after the insert, and ends this session with the others. Sessions start
through the user's association so the check sees the user that was
authenticated, and a refused sign-in is answered like a wrong password.
(cherry picked from commit 19319a1cff)
The profile asked for the current password when the request's password
field was filled in. The check now follows the save itself: the model
checks the challenge whenever the password digest is about to change,
and the profile always hands it the challenge. Editing a name, avatar
or email without a new password still needs none, and scripts that
reset a password, which set no challenge, are unaffected.
Only a string counts as a challenge, and the rate limit counts every
non-empty one, so what's limited is exactly what gets checked.
(cherry picked from commit 4a580302a5)
The lookup under the write lock repeated the one made before
validation, so the query cache could answer it with that earlier miss
even after another connection had registered the endpoint. It runs
uncached now.
Enabling notifications unsubscribed the browser on any refusal, so a
session signed out between authenticating and saving lost the browser's
subscription too, and the next session had nothing to post. Only a
refused endpoint unsubscribes it now.
From #371.
(cherry picked from commit aa93269ceb)
Only room pages posted the subscription again, so a browser that
changed its password and closed the profile page went without
notifications until it next opened a room. A small controller in the
layout does it now, still once per session, and the bell is back to how
it was.
Two tabs posting at once after a password change could both miss the
row and insert two, so the endpoint is looked up again under the same
write lock as the session check.
From #371.
(cherry picked from commit 3786ee5244)
The browser is shared across system tests, and a script added to every
new document stays until it's removed, so later tests would have seen
the granted permission and the fake subscription too.
From #371.
(cherry picked from commit 1af62a5bff)
A push subscription belongs to the user, not a session, so a device
signed out by a password change kept getting notifications. Changing
the password now removes every subscription along with the sessions,
in the same transaction.
A browser still signed in keeps its own subscription in its service
worker, so the notifications controller posts it once per session: the
bell names the session by a digest of its id, never its token, and the
browser remembers the last one it posted for. That post never
unsubscribes the browser when the server refuses it.
A request that authenticated just before the change could put a
subscription back after it. The create action now checks the session
still exists and saves in one immediate transaction, after validating
the endpoint outside it.
From #371.
(cherry picked from commit 6cab924097)
Changing the password now ends every other session, so whoever holds
one could shut the owner out of their own account with a new password.
The profile asks for the current password whenever the password
changes; without it, or with a wrong one, nothing is saved and the page
says why. Other profile edits don't need it, and the admin
reset-password script still works without it.
Attempts are rate limited like signing in, 10 in 3 minutes per address,
so the current password can't be guessed here instead.
From #371. The cached-response test it touches isn't in 1.5.2.
(cherry picked from commit 44a92ff6c6)
A transfer link signed only the user id, so one copied before the
password changed still started a session for up to four hours after.
Bind it to the password salt with generates_token_for, so a new
password ends those links too. Links made before this upgrade stop
working; they last four hours at most.
From #371.
(cherry picked from commit 160ba295b5)
An after_commit condition reads the record's last save, so another save
of the user in the same transaction skipped the disconnect even though
the sessions were gone. The callback that deletes them now registers
the disconnect on the transaction itself, which also leaves a rolled
back change alone. A password changed along with the avatar is covered
too: the attachment's touch doesn't hide the change.
From #371.
(cherry picked from commit e158388f3d)
Keeping the session the password was changed from also kept any copy of
its cookie working, and a copied cookie is one of the ways someone else
gets into an account. Every session ends now, and the profile page
signs the browser that changed the password back in with a new one.
From #371.
(cherry picked from commit b01bb7a0a7)
The sessions end wherever the password changes, but only the profile
page closed the live connections, so a change from a console left a
signed-out device receiving messages until it dropped. Close them after
commit from User, logging rather than failing if the realtime service
can't be reached, since the password is saved by then.
From #371.
(cherry picked from commit c6e0105491)
Changing the password is how someone shuts out whoever else is signed
in to their account, but every session opened with the old password
stayed valid. Now the others end and their live connections close, as
on sign-out. The session the password was changed from stays signed in.
Changing the password outside a request, as from a console, ends every
session.
From #371. 1.5.2 has no record snapshot or response cache, so the test
that a cached session lookup doesn't outlive the password change is
left out; the rest applies as it is.
(cherry picked from commit c78c3fd7ef)
Opengraph::Fetch connects to the IP it checked, not to a fresh lookup of
the host. With http_proxy set, Net::HTTP would instead send the hostname
to the proxy, which looks it up again, so the connection could end up
somewhere other than the checked address. Pass nil as the proxy address,
as web push delivery already does.
(cherry picked from commit a7e9ea53dd)
An asset host's path can contain the asset directory too, as in
https://cdn.example.com/assets, and cutting each URL at the first one would
allow everything the host serves under it. Cut at the last one instead.
(cherry picked from commit 2ba940c683)
Behind a proxy that terminates TLS without X-Forwarded-Proto, Rails sees
http while the page is https, and an http source on a non-default port
doesn't match. A source without a scheme takes the page's own.
(cherry picked from commit 0a494a10cb)
Uploads are served from this origin too, so script-src 'self' would run one
that a cache still holds under a script type from before the upload guard.
Application pages now allow each importmap URL up to its asset directory
(relative root and asset host included), the inline importmap by hash, and
the service worker by worker-src.
(cherry picked from commit 2be2227f40)
With DISABLE_CONTENT_SECURITY_POLICY=true there's no application policy,
and the hash callback started an empty one, so pages sent an empty
Content-Security-Policy header. Skip the callback then.
(cherry picked from commit 2a449401ec)
A request for several byte ranges gets a 206 multipart/byteranges whose
boundary the client needs to split the body. Rewriting it to
octet-stream lost the boundary. No browser runs that type as a script,
so it passes through.
(cherry picked from commit 9c37f41ff6)
script-src 'self', object-src 'none' and base-uri 'self', enforced. The
two inline scripts the importmap helper emits are allowed by their
SHA-256, and the origins its URLs point to (a configured asset host, in
any form Rails accepts) are allowed by name. There is no per-request
nonce, so cached pages stay valid.
DISABLE_CONTENT_SECURITY_POLICY=true leaves the policy off, for an
install whose proxy or customization injects scripts.
(cherry picked from commit 562888251e)
Backport: 1.5.2 has no response cache, so the cached-page test is left out.
Uploaded files come from this origin, so one served with a JavaScript
type could run as one of the app's own scripts. Browsers accept sixteen
spellings of that type, and Disk URLs sign the type they were minted
with, so refusing a list of types at upload can't cover links already
out there. Instead, a middleware rewrites the type of anything Active
Storage reads out that isn't plainly media, a PDF or plain text to
application/octet-stream, whatever the URL says.
(cherry picked from commit cad2d7211d)
Replying swaps each mention in the quoted copy for its text. Assigning
that text to outerHTML parsed it as html; replace the mention with a
text node instead.
(cherry picked from commit 6052b123d5)
auto_link finds URLs and email addresses with regular expressions over
the rendered html, which can't tell an attribute value from text, so it
could put a link inside an attribute. Hand it each text node on its own,
escaped the way the serializer writes text, after the final sanitize, so
links only ever land between tags. Write the result with
preserve_newline so a code block keeps its leading blank lines.
Bump the presentation cache version so nothing rendered the old way is
served again.
Backport of 7ea05e1, 4e29c6e and 9b271d0 from #364. 1.5.2 has no
plain-text fragment cache, so only the message partial's version moves.
The filters can write the same table with or without newlines between
its rows depending on which tests ran first in the process, so the exact
string comparison failed under some seeds. main compares it as markup.
Banning a user deletes their sessions and closes their connections but
keeps their push subscriptions, and Room::MessagePusher chose recipients
by membership alone. A banned user's browser or phone therefore went on
receiving the room name, sender and text of new direct messages,
mentions, and messages in rooms they had set to everything.
Choose subscriptions from active users only. The subscriptions are kept,
so unbanning brings notifications back without the user having to
subscribe again (the client doesn't resubscribe while the browser still
holds a subscription).
Co-authored-by: Marcello Costagliola <176920116+namespaceMarcello@users.noreply.github.com>
* Derive message DOM ids from the server id, not client_message_id
A message's DOM id was derived from the browser-chosen client_message_id
via a Message#to_key override, so dom_id(message) was
"message_<client_message_id>". Turbo's append de-dups by DOM id, so a room
member who posted a message reusing a victim's client_message_id displaced
the victim's message element in every connected member's live view; editing
the attacker's own message then broadcast onto the victim's presentation id.
Drop the to_key override so every message DOM id and broadcast target derives
from the record's primary key. Two distinct records can no longer share a DOM
id regardless of stored client_message_id, so the collision is impossible with
no data migration and no uniqueness constraint. to_param and the fragment
cache key already used the primary key, so message URLs and per-message cache
entries are unchanged.
The composer's optimistic pending message still uses client_message_id as its
placeholder DOM id, which no longer matches the server broadcast's PK-based id.
Reconcile instead by rendering data-client-message-id on the real message and
having the messages controller drop the matching pending placeholder on
connect. Only client-side placeholders (data-pending-message) are removed, so a
message another member posts reusing the same client_message_id can never
displace a real one through the reconciliation path either.
Also point the boost broadcast target at the PK-based dom_id(message, :boosts)
to match the rebuilt container id.
GHSA-3v99-4vxh-xg84
* Bust cached message fragments rendered with client_message_id DOM ids
The message fragment cache keys on the record and the template digest, and
removing the to_key override changes neither. Fragments cached by an earlier
release would keep their message_<client_message_id> ids, so edit, delete and
boost broadcasts, which now target primary-key ids, would miss those messages
in other members' live views until the cache entry expired.
* Locate messages by record id in the client_message_id collision tests
Assert on data-message-id rather than the new primary-key DOM ids, so the tests
describe the behavior instead of the fix and fail on the vulnerable code for the
real reason. Edit the attacker's message to new text and wait for it to arrive,
so the edit path is exercised rather than passing vacuously. Add a request test
that two messages sharing a client_message_id render as distinct elements.
---------
Co-authored-by: Jeremy Daer <jeremy@37signals.com>
Showing an image's preview when its variant had already been made tied the cached message to state
that changes without touching it: a variant made after the message was cached would never show. It also
cost a query per embedded image, since Action Text loads the embeds without their variant records.
Nothing in the app makes those variants, so the partial no longer looks at them.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa
Files are posted as a message's own attachment, and the composer puts none in the rich text: it permits
only mentions and link embeds. The server still keeps any attachment whose signed id resolves, a blob
included, and Action Text's blob partial links to a representation URL that makes the preview on the
first request, without the limits the POST has, and on every request while it fails.
The app's own partial shows an image's preview only if it was already made, and otherwise the file's
name and size, as it does for a file that isn't an image. The cached presentation's version goes up,
since Action Text renders the partial by name and the template digest doesn't see it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa
Jeremy Daer
Jeremy Daer
Rosa Gutierrez
GPT on behalf of DHH
Donal McBreenandJeremy Daer
Marcello CostagliolaandClaude Opus 5.5