Transfer the C completed-response cache lesson into Rails, keeping authentication, room checks and cookies per request. A persistent read-only SQLite observer detects local and foreign commits and rejects racing admission. Whole-page misses render fresh to avoid stale nested fragments; message ETags reflect token-neutral presentation.
* 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>
A video's preview and a picture's thumbnail are made inside the request that posts the message, and
nothing bounded how long either could take.
- The video preview filter also selects any frame from 5 seconds on. Rails' filter takes the second
frame it selects, which a video with a single keyframe and no scene change only gives at its end, so
ffmpeg decoded all of it.
- TimeLimitedVideoPreviewer gives ffmpeg 10 seconds of wall-clock time, kills it past that, and reports
a failed preview, so the message is posted without one.
- Pictures and videos above 250 megapixels, or whose size couldn't be read, get no preview: decoding
costs in proportion to the pixels, however small the file.
- The view shows a preview only if it was made when the message was posted. Its URL used to make it on
view, so a preview that failed or was skipped would be attempted again on every view. The cached
presentation's version goes up, so cached messages pick this up.
- A video's poster is made, when the message is posted, at the size the view shows it. The full-size WebP
made until now wasn't shown anywhere, and encoding it costs in proportion to the frame's pixels.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa
Deleting the memberships with delete_all skips Membership's after_destroy_commit, which resets a member's
connections when one membership is revoked. Until the job ran, a member who had the room open kept its
streams, and a message that still landed in the room (a request already past the membership check, a bot's
reply) reached them. The request now reads the members' ids in the transaction that deletes their
memberships, and the job resets their connections before it destroys the messages. It costs a Redis round
trip per member, about 1.5 s for 10,000, so it's done in the job rather than in the request.
User#grant_membership_to_open_rooms read the open rooms and inserted in a separate statement, so a user
created while a room was being closed could read it as open and be granted it after the close. It's now a
single insert ... select, which SQLite runs under the write lock, skipping duplicates as insert_all did.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa
The deadline in Opengraph::Fetch started after the lookup of the pasted
host, and an unfurl looks up more hosts outside any fetch: the canonical
URL's, and the image's, once to check its content type and again to
validate it. Each of those waits as long as the resolver takes to give
up. Move the deadline up to UnfurlLinksController, around everything the
link leads to; Opengraph::Fetch keeps its per-operation timeouts and
makes no retries.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa
Room#destroy destroyed every message inside the room's own
transaction, which holds SQLite's write lock until the last one: on a
room with many messages, every other write in the app waited and
failed. The request now takes the room away from its members and
leaves the rest to Room::DestroyJob, which destroys the messages one
at a time, each in its own short transaction, and then the room.
An open room is closed in the request, so that someone who joins the
account before the job ends isn't given it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa
Rails main compiles HTML templates through Herb under the 8.2 framework
defaults, which Campfire loads. Herb rejects `case` and its first `when`
in a single ERB tag, so the three pwa/ partials failed to compile, and
`herb:check` rejects ERB output in attribute names, which the account
settings' switch used for `checked`. Give `case` its own tag and build
the switch with tag.input; both render the same under Erubi.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 244e77241b)
Pages of messages load them with_presentation, which preloads each
message's boosts with their boosters and avatars, but the boosts partial
asked for message.boosts.ordered: a new query for every message, and the
preloaded boosts went unused. Sort the preloaded boosts in Ruby. The
boosts frame, whose message comes without them, still queries them in
order with their boosters.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9
Every boost had its own fragment, nested inside the message fragment that
already holds it. With a cold cache, a read and a write per boost plus a
read_multi and a write_multi per message: a room page of 40 messages with
120 boosts takes ~410 ms instead of ~290 ms. Redis runs without persistence
(config/redis.conf), so every page is cold after a restart.
A boost touches its message, so adding or removing one rewrites the message
fragment anyway. The inner fragments only paid off when a message with many
boosts was drawn again, and there they hid a lookup per booster. The boosters
are now loaded with the boosts, one query per message.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9
Administrators see banned users in the account settings, but only the
first page counted them: the next pages listed active users alone. A
banned member before the page boundary shifted the offset, so one active
member was never listed. Both pages now share User.visible_to.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9
Since administrators were grouped apart from members (b52c318), the account
settings page loads every user to split them and renders all of them. The
lazy next page still starts at the 501st user, so on an account with more
than 500 people, scrolling down lists those users a second time.
Load administrators on their own and page only members, both on the settings
page and on the pages that follow it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0142qgjggdJ2KDdGk7RF9Xm9
Directs merge Room.directs and order by recency. Shared rooms reuse
with_ordered_room and without_direct_rooms, with the STI filter coming
from Room.without_directs so the join alias stays rooms.
Co-authored-by: Thomas Klemm <github@tklemm.eu>
Load visible direct memberships ordered by room recency and other
rooms ordered by name, instead of hydrating every membership and
splitting them in Ruby.
Fixes#307.
Co-authored-by: Thomas Klemm <github@tklemm.eu>
The thumbnail or video preview is generated while the message is posted.
When ffmpeg or libvips can't decode the file (a truncated upload, an .mp4
holding only audio), the error escaped after the message had been saved:
the request failed with a 500, the message was never broadcast, and the
sender's upload stayed at 100% while the rest of the files in that drop
were never sent. Posting the message without a preview keeps the file and
lets everyone see it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JVFo3Lt9T8M5NR7KxVvsZ2
Now that the unread fanout runs in a job, it can run after a member has
already opened the room and moved on to another one. Their sidebar would
then mark the room unread for a message they have seen.
The job now notifies only members who still have the room unread or are
in it now. Room#receive marks members who aren't in the room unread, and
opening the room clears it, so a member who caught up in the meantime is
skipped. When the
job runs right away this is the same set of people as before, except
members who have hidden the room, whose sidebar doesn't list it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JVFo3Lt9T8M5NR7KxVvsZ2
install-edge.svg lives in app/assets/images/external/, alongside the
other install icons, but the Edge branch of pwa/_install_instructions
asked for it at the top level. Propshaft raises MissingAssetError, so a
browser the useragent gem reports as Edge got a 500 on the profile page
and anywhere else the partial renders.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
link_to passed `action: "soft-keyboard#open"` as a plain option, so it
rendered as an `action` attribute, which Stimulus never reads. The link
in messages/_actions.html.erb already passes it under `data:`; this one
now does too, so tapping "Add a boost" on a touch device opens the
keyboard for the boost form as the menu's link does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since the unread rooms stream was scoped per user, posting a message
publishes the room id once to each member of the room. Each publish is a
round trip to Redis, made one after another inside the request, so the time
to post a message grows with the size of the room and in a big room is far
more than the rest of the request.
The fanout now runs in Message::BroadcastUnreadRoomJob, so the poster no
longer waits for it. Who gets the notice and what it says are unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NuTtwJKhb77Dv7EQqv2C3C
The search box keeps only word characters, but FTS5 still reads AND, OR,
NOT and NEAR in the query as operators, so searching for "AND" or for
"salt AND" raised "fts5: syntax error" and answered with a 500. Quoting
each word makes FTS5 look for it as text, which is how it already treats
the same words in lower case.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NuTtwJKhb77Dv7EQqv2C3C
* Preload only uncached messages and reduce rendering overhead
* Keep benchmark summaries without raw JSON results
* Use Ruby benchmark drivers and keep generated results out of the repo
Link previews fetch a page's OpenGraph title and description, and
Opengraph::Metadata strips tags from both before the values reach the
browser. When a field consists entirely of a markup tag, stripping
leaves it blank, the metadata fails its presence validation, and the
unfurl endpoint returns no content, so no preview is produced.
Add regression tests at the model and controller layers that pin this:
a title or description made only of a markup tag is stripped to blank
and rejected, and the endpoint answers 204. The existing sanitize tests
only cover fields that keep non-blank text after stripping, so this
blank-and-rejected path was previously untested.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186eyzivcTn6wqjEE4Wnxdt
Pasting a link builds the preview by interpolating the unfurled metadata
into an HTML string. The image URL went into src="..." unescaped, so a
page whose og:image carries a double quote closes the attribute early and
everything after it becomes attributes on the preview's img element.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186eyzivcTn6wqjEE4Wnxdt
A preview belongs to the page it previews, so both URLs point somewhere
else. An absolute URL on our own host passed the scheme and host checks,
and every reader's browser fetched it with their session attached, which
turns a message into a GET request made on the reader's behalf.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186eyzivcTn6wqjEE4Wnxdt
A link preview's link and image come from attributes on the message body,
which the composer fills in from the unfurl the server performed. A body
written by hand can put anything in those attributes, and the preview
partial rendered them as they were.
Keep the link and the image only when they parse as absolute http or https
URLs, so nothing in a message body can aim either one at another scheme or
at a path on this Campfire, and render the title and the description as
text.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0186eyzivcTn6wqjEE4Wnxdt
Action Cable authorizes a Connection once at the WebSocket handshake and
never re-checks it. Destroying the session record refuses future handshakes
and HTTP requests bearing the cookie, but a socket opened before sign out
keeps its handshake-time current_user and keeps authorizing new
subscriptions and delivering frames as the signed-out user.
Reset the user's remote connections when the session is terminated. Clients
tear down and reconnect: the signed-out device carries a destroyed session
and cleared cookie and is rejected at the fresh handshake, while the user's
other devices with still-valid sessions reconnect and stay live. This reuses
the existing reset_remote_connections primitive already used on membership
removal, for the same reason.
Run the disconnect last and best-effort, after the session record and cookie
are already gone, so sign out completes even when the realtime service is
unreachable.
Active Storage mounts its direct-upload write endpoints -- POST
/rails/active_storage/direct_uploads and the disk-service PUT at
/rails/active_storage/disk/:token -- on framework controllers that inherit
from ActiveStorage::BaseController, so they never pass through
ApplicationController's require_authentication. Anyone who can read the
public login page can lift a CSRF token and Rails session cookie, POST to
the metadata endpoint, and receive a signed disk PUT URL without holding a
Campfire session_token.
That is enough to allocate ActiveStorage::Blob rows and persist bytes to
disk anonymously. The blobs stay unattached (no message can be created
without an account) and nothing purges them, so an unauthenticated caller
can grow storage without bound. Because the recommended self-host layout
co-locates uploaded files and the SQLite database on one /rails/storage
volume, that growth eventually makes database writes fail -- blocking login
and messaging until an administrator frees space and purges the blobs.
Campfire uploads attachments through MessagesController as a normal
multipart POST and does not use direct uploads at all, so these endpoints
have no legitimate anonymous caller. Require a valid Campfire session
before the metadata endpoint allocates a blob or the disk endpoint accepts
an upload; both return 401 to anonymous callers. Serving (disk#show,
representations, blob redirects) is unchanged.
* Guard push-subscription endpoints against SSRF
Web push delivery POSTed to the endpoint URL a user supplied when
registering a subscription, with no scheme, host, or private-network
check -- unlike the OpenGraph unfurl path, which already routes through
the shared SSRF address policy (surfguard). Any authenticated user could
register a subscription whose endpoint pointed at an internal address and
have the server fetch it on every chat message: blind SSRF for internal
recon and reachability probing, plus a thread-pool DoS on the delivery
pool.
Validate the endpoint when the subscription is saved: it must be HTTPS,
its host must belong to a known browser push service (allowlist), and it
must resolve to a public IP. On every delivery, re-resolve the host and
pin the connection to that public IP so a later DNS rebind can't redirect
the request to an internal address. If no public IP resolves at delivery
time -- a rebind, or a subscription that predates this validation --
delivery is skipped rather than falling back to re-resolving the raw
host.
Adds model, controller, and delivery-pinning tests, plus a DNS stub
helper for deterministic resolution in tests.
* Route push endpoint guarding through RestrictedHTTP::PrivateNetworkGuard
The endpoint SSRF check already delegated its private-network classification
to surfguard, but called Surfguard.resolve_public_ips directly rather than
through RestrictedHTTP::PrivateNetworkGuard -- the hostname-in, address-out
shim #241 established as the app's single guarded-outbound entry point and
that Opengraph::Fetch resolves through. Route push resolution through the same
guard so once-campfire keeps one place that resolves and classifies outbound
addresses. Behavior is unchanged: the guard raises Violation when a permitted
host resolves only to blocked addresses (previously an empty list -> nil) and
propagates Surfguard::Unresolvable for a host that resolves to nothing; both
map to a nil endpoint IP, which fails validation and skips delivery. The
allowlist, HTTPS/443 constraints, and per-delivery IP pinning are unchanged.
* Address push-SSRF review: defer DNS off enqueue path, disable proxy on pinned path, revalidate on re-registration
- Resolve the guarded endpoint IP lazily inside WebPush::Notification#deliver
(on the bounded delivery worker) instead of eagerly when the notification is
built on the serial enqueue path, so a slow resolver can't stall the push job
before any delivery starts. resolved_endpoint_ip only reads the already-loaded
endpoint attribute, so it is safe off the AR connection.
- Pin the delivery socket with an explicit nil proxy address so http_proxy/
https_proxy can't route the request through a proxy that re-resolves the host
and defeats the ipaddr pin.
- Revalidate an existing subscription on re-registration so a row predating
endpoint validation gets the same 422 as a fresh create instead of being kept
alive by touch.
- Regression tests: resolution deferred to delivery, pin survives proxy env,
legacy invalid row rejected with 422.
* Bound the web-push delivery queue (max_queue, not the ignored queue_size)
Concurrent::ThreadPoolExecutor takes :max_queue; :queue_size was silently
ignored, leaving the delivery backlog unbounded (max_queue: 0). A flood of
valid push subscriptions could accumulate queued deliveries without limit --
more acute now that each delivery task also resolves DNS. Using max_queue: 10000
activates the intended cap; overflow raises RejectedExecutionError under the
default :abort policy, which deliver_later already rescues (push is best-effort,
retried on the next message).
* Trim push-SSRF guard comments to match house style
Apply the review suggestions on the push-subscription SSRF guard: replace
the verbose rationale comments with terse one-liners (or drop them where
the code speaks for itself). No behavior change -- the Surfguard-shim
routing, per-delivery public-IP pin, and bounded delivery queue are
untouched.
Bots can only create. A lifecycle notification — an alert that fires and then
resolves, a deploy that starts and finishes, a backup that runs — therefore has
to post a second message, and the room becomes an append-only log of states
rather than a view of the current one.
Adds PATCH and DELETE inside the existing bot_key scope, routed to
Messages::ByBotsController. The body is read the way create reads it, so
updating a message is the same request shape as posting one.
No new authorization: both actions already run through ensure_can_administer,
and can_administer? grants access only to a record the user created, so a bot
key reaches that bot's own messages and no others. set_room narrows it again by
looking the room up through the bot's own memberships. A leaked bot key gains
what it could already do by posting: write to rooms that bot belongs to.
update answers head :ok rather than the redirect, which meant extracting the
update and its broadcast into update_message — calling super and then head
would double render, since the parent redirects inside the action. destroy
needs no split, because the parent renders implicitly like create does.
GPT on behalf of DHH
Donal McBreen
Marcello Costagliola
Cursor Agent
Sam Ruby
Stanko Krtalić
Thomas Klemm
David Heinemeier Hansson
Rosa Gutierrez
Jeremy Daer
Ronald Lokers