Render link previews only from web URLs - #276

Require a host before accepting web URLs

When Campfire is served over HTTPS, a hand-written attachment can use https:/rooms/1 or https:rooms/1. Ruby parses either value as URI::HTTPS, so this method preserves it, but browsers resolve it against the current origin as https://<campfire-host>/rooms/1. This bypasses the intended local-path restriction; when supplied as url, merely viewing the message performs an authenticated same-origin GET and can change the viewer's last_room cookie through the RoomsController#show callback. Require a nonempty host/authority before retaining the URL.

Useful? React with 👍 / 👎.

🤖 Good catch, fixed in c3ae67a. web_url now requires a host as well as an http(s) scheme, so https:/rooms/1, https:rooms/1 and https:// are all dropped. Test cases added for each. It also keeps the rule aligned with the unfurl path, which resolves the host through the private-network guard and so can never produce a hostless URL either.

Reject absolute same-origin preview images

When a member hand-writes url as https://<campfire-host>/rooms/<shared-room-id>, this predicate accepts it because both the scheme and host are present, and the partial emits it as an <img>. Viewing the message then performs an authenticated same-origin GET; RoomsController runs remember_last_room_visited, which overwrites the viewer's last_room cookie. Fresh evidence beyond the earlier malformed-URL case is that the equivalent absolute same-origin URL still passes the newly added check, so reject the current origin at render time or cryptographically bind preview attributes to output from the unfurl endpoint.

Useful? React with 👍 / 👎.

🤖 Agreed, fixed in eceec28. A preview's link and image now have to name a host that isn't the one serving the request (Current.request_host), so an absolute same-origin URL is dropped along with the hostless forms. Tests cover the attachable and the room render.

Worth saying what this does and doesn't reach: it stops the message body naming our own host, which is what makes the browser attach the reader's session. It doesn't turn the preview image into a first-party one, so a preview still loads a third-party image, which is what unfurling a link means. Serving preview images from our own origin would close that, and it's a product change rather than part of this fix.

Canonicalize the hostname before comparing origins

A hand-written image URL such as https://%77ww.example.com/rooms/1 bypasses this comparison when Campfire is hosted at www.example.com: Ruby leaves the escaped byte in URI#host, while browsers canonicalize the URL to https://www.example.com/rooms/1 before fetching it. The resulting <img> therefore still performs an authenticated same-origin GET and triggers callbacks such as RoomsController#remember_last_room_visited. Fresh evidence beyond the absolute same-host form addressed earlier is the browser/Ruby disagreement for percent-encoded hostnames; compare a browser-canonicalized hostname or reject encoded authority components.

Useful? React with 👍 / 👎.

🤖 Fixed in ef4b88d. The host comparison now rejects any host carrying a percent-escape, and ignores case and a trailing dot, so https://%77ww.example.com/rooms/1 no longer reads as a different host than the one we answer on. An escaped host is also something the unfurl could never produce: it resolves the host through the private-network guard, which would fail on it.

Prevent external image redirects back to Campfire

When url names an attacker-controlled external endpoint that responds with a redirect to this Campfire, this predicate accepts the initial host, but the browser follows the <img> redirect and attaches the viewer's cookies to the same-origin destination. That again permits an automatic authenticated GET such as /rooms/1, despite the stated protection. Fresh evidence beyond the direct same-host URL addressed earlier is that the final redirect target is never checked; remote images need to be fetched through a controlled proxy or otherwise prevented from redirecting readers back to the application.

Useful? React with 👍 / 👎.

🤖 Agreed that the redirect case is real, and deliberately not addressed here.

A URL check can't reach it: the redirect target only exists at fetch time, in the reader's browser. The same is true of a preview image an unfurl produced, since the page being previewed picks the image URL and that server can redirect wherever it likes, so this isn't something the change regresses.

What closes it is serving preview images from our own origin: fetch the image ourselves, follow redirects under the private-network guard we already have, check what came back, and re-serve it. That's a product change with its own bandwidth and caching decisions, so I've written it up on the card for the maintainer to decide rather than folding it into this fix.

Compare against a host available outside requests

When a bot webhook returns text/html, Bot::WebhookJob passes it to Webhook#receive_text_reply_to, which immediately broadcasts and renders the message from a background job where Current.request is unset. Here Current.request_host.to_s therefore becomes "", so an attachment targeting the Campfire host is treated as external and its authenticated same-origin image request is sent to connected room members; production fragment caching in messages/_message.html.erb can also preserve that unsafe rendering for later page loads. Use a stable configured application host (or reject local URLs before request-independent rendering) rather than relying solely on request-local state.

Useful? React with 👍 / 👎.

🤖 Half of this landed, and thank you for it: the cached presentation was a real problem. messages/_message.html.erb caches each message's rendered presentation, and its key can't see the link preview partial, because ActionText renders that one by name rather than through a render call the digestor can follow. A message already in the cache would have kept its old preview after this shipped. Bumped the version in 32e3e5a, which is what that template's own comment asks for.

On the host comparison outside a request, I'm leaving it as it is. Every message a member writes is rendered inside a request: the controllers call broadcast_create, which renders inline rather than through broadcast_append_later_to. The one render with no request is the bot webhook reply you named, and that body is the bot's own HTTP response, wired up by an administrator to a service the operator runs. SECURITY.md puts that at operator level rather than treating it as a boundary.

Giving the comparison a configured host instead would mean introducing one: Campfire derives every URL it builds from the request it's answering (SetCurrentRequest#default_url_options), and there's no canonical host setting to read. That's worth doing on its own terms, not inside this fix.

Normalize IP address hosts before comparing

When Campfire is served from an IP literal, alternate spellings bypass this comparison: for example, Ruby preserves 2130706433 while browsers canonicalize http://2130706433/rooms/1 to http://127.0.0.1/rooms/1; expanded IPv6 addresses have the same problem. The preview image is therefore fetched from Campfire with the viewer's cookies despite the host check. Fresh evidence beyond the previously addressed percent-escaped hostname case is this browser normalization of numeric hosts; normalize IP literals equivalently or reject non-canonical numeric hosts.

Useful? React with 👍 / 👎.

🤖 Fixed in 9e19658, by refusing the shape rather than matching the spellings. A preview's host now has to look like a domain name: a dot, a letter, no escapes. That drops every address spelling in one rule, 127.0.0.1, 2130706433, 0177.0.0.1, [::1] and localhost alike, without this code trying to canonicalize a URL the way a browser does.

Saying plainly what that covers: it is not browser canonicalization parity, and I don't think this comparison should grow toward it, since that is a race the browser wins by definition. It stops a message body naming an address, which is not something a link preview has any business doing, and an unfurled preview never does. The structural answer to the whole class is serving preview images from our own origin, which is on the card for the maintainer to decide.

Reject dotted hexadecimal IPv4 host spellings

When Campfire is served from an IPv4 literal, a URL such as http://0x7f.0.0.1/rooms/1 passes this predicate because its Ruby host contains both dots and the letter x, while browsers canonicalize it to http://127.0.0.1/rooms/1 before fetching. The preview can therefore still issue an authenticated same-origin GET. Fresh evidence beyond the previously tested address spellings is that dotted hexadecimal components satisfy the newly added domain-name heuristic; explicitly reject numeric components with 0x prefixes or compare browser-canonicalized addresses.

Useful? React with 👍 / 👎.

🤖 Fixed in 436ea06. The shape check now reads the host's last label: a domain name ends in a word, never a number and never the hexadecimal spelling of one, so 0x7f.0.0.1 and 1.2.3.0xff go with the rest of the address spellings. A punycode domain still passes, and there's a test for one.

That's where I'm leaving the comparison. It now refuses anything that isn't plainly a domain name, which is the property worth having, and I'd rather it stayed that than grow into an imitation of a browser's URL canonicalizer, which is a race this code can't win. If another spelling turns up, the answer I'd argue for is the structural one on the card: fetch the preview image ourselves and serve it from our own origin, so the reader's browser never resolves a URL a message picked.