Fix a stored XSS in user-supplied URLs, found by the CI gate nobody had run

The branch is 600 commits ahead of main with no pull request, so I checked which
workflows one would trigger. Six: rspec, rspec-system, audit-selftest,
ruby_lint, brakeman, factory-bot-lint. The design suite covers the first four
and I had been running them all session. I had never run the last two.

Brakeman failed -- LinkToHref, weak confidence, on
organizations/_details.html.erb -- and it was real. `Organization#url` validated
with `URI::DEFAULT_PARSER.make_regexp` and no scheme argument, which accepts any
scheme, so `javascript:alert(document.cookie)` was a valid organization URL. That
field is rendered with `link_to`, so an organization admin sets it and anyone who
views the page and clicks runs script in their own session. Weak confidence
describes the static analysis, not the exposure.

Restricting the scheme is not enough on its own, and that is the more
interesting half. `BroadcastAnnouncement#link` already restricted it to
`http https` and was still bypassable, because `format:` is unanchored and
matches a substring: "javascript:alert(1) http://decoy.example.com" satisfied
it. That field is the "More info" link on every user's dashboard -- the widest
exposure of the three, and it had looked fixed. Found by testing the fix rather
than trusting the pattern that was already there.

Two layers:

  * HttpUrlValidatable -- anchored, http and https only -- on all three fields
    that take a URL from a user: Organization#url, BroadcastAnnouncement#link,
    AccountRequest#organization_website.
  * essentials_external_link and essentials_safe_href at the four render sites.
    Split in two because the dashboard link carries its own classes and
    accessible name, so it needs the href checked rather than the anchor built
    for it.

Both, because the validation guards one write path and a row can arrive by CSV
import, from the console, or from a database restored from before it existed. A
dangerous URL renders as plain text rather than nothing: dropping it hides the
problem from the only people who can fix it, and a bank looking at a nonsense
URL is how it gets corrected.

Checked before changing anything: 0 rows across all three fields held a
non-http(s) value, so nothing was invalidated. Had there been any, the data
migration would have had to come first.

43 new examples, and 21 of 36 watched failing against the old pattern.
Brakeman 1 warning -> 0, exit 0. factory_bot:lint exit 0. Full suite 3,430
examples 0 failures. All 29 audits the selector named: clean, with dead-code at
its documented 147.

The lesson is not about URLs. Four of six gates were covered; the two that were
not are where this was hiding, and it only surfaced because main bumped
brakeman 8.0.5 -> 8.0.6 in the merge and a new version brings new checks.
onboarding.md's post-merge routine has a step 9 now, naming both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: claude

app/helpers/essentials_ui_helper.rb

@@ -306,6 +306,33 @@ def essentials_action_button(label, path, method:, variant: :primary, size: :md,
     end
   end
 
+  # A user-supplied URL, rendered as a link only if it is safe to click.
+  #
+  # `HttpUrlValidatable` stops a `javascript:` or `data:` URL being *saved*, and this stops one
+  # that was saved before that validation existed being *rendered* as a live link. Belt and
+  # braces on purpose: the validation guards one write path, and a row can arrive by import, by
+  # console, or from a database restored from before the fix.
+  #
+  # Falls back to plain text rather than dropping the value: the reader should still see what the
+  # field contains, and a bank looking at a nonsense URL is how it gets corrected.
+  # The URL if it is safe to put in an `href`, otherwise nil. Call sites that build their own
+  # link -- the dashboard's "More info" carries its own classes and accessible name -- guard the
+  # href with this and keep their markup.
+  def essentials_safe_href(url)
+    url if url.present? && url.match?(HttpUrlValidatable::HTTP_URL)
+  end
+
+  def essentials_external_link(url, **html_attrs)
+    return if url.blank?
+
+    href = essentials_safe_href(url)
+    # Plain text rather than nothing: the reader should still see what the field holds, and a bank
+    # looking at a nonsense URL is how it gets corrected.
+    return tag.span(url, class: "text-slate-600") if href.nil?
+
+    link_to href, href, class: "link-brand", rel: "nofollow noopener", **html_attrs
+  end
+
   # --- Status pills ---------------------------------------------------------
   #
   # Never colour alone: every tone pairs its colour with a word, and callers may add an

app/models/account_request.rb

@@ -16,12 +16,16 @@
 #  ndbn_member_id       :bigint
 #
 class AccountRequest < ApplicationRecord
+  include HttpUrlValidatable
+
   has_paper_trail
   validates :name, presence: true
   validates :email, presence: true, uniqueness: true
   validates :request_details, presence: true, length: { minimum: 50 }
   validates :email, format: { with: URI::MailTo::EMAIL_REGEXP }
-  validates :organization_website, format: { with: URI::DEFAULT_PARSER.make_regexp, message: "should look like 'https://www.example.com'" }, allow_blank: true
+  # Rendered as text rather than a link today, so this is consistency rather than a fix -- but a
+  # field that looks like a URL tends to become one.
+  validates_http_url :organization_website
 
   validate :email_not_already_used_by_organization
   validate :email_not_already_used_by_user

app/models/broadcast_announcement.rb

@@ -12,10 +12,14 @@
 #  user_id         :bigint           not null
 #
 class BroadcastAnnouncement < ApplicationRecord
+  include HttpUrlValidatable
+
   has_paper_trail
   belongs_to :user
   belongs_to :organization, optional: true
-  validates :link, format: URI::DEFAULT_PARSER.make_regexp(%w[http https]), allow_blank: true
+  # Scheme was already restricted here; the pattern was not anchored, so a valid URL appended to
+  # a `javascript:` one satisfied it. Rendered as "More info" on every user's dashboard.
+  validates_http_url :link
   validates :message, presence: true
 
   def expired?

app/models/concerns/http_url_validatable.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+# A URL a user supplies and the app later renders as a link.
+#
+# **Two separate faults, both proven before this was written.**
+#
+# `URI::DEFAULT_PARSER.make_regexp` with no arguments accepts *any* scheme, so
+# `javascript:alert(document.cookie)` was a valid `Organization#url` -- and `organizations/show`
+# renders that field with `link_to`, which makes it a stored XSS: an organization admin sets it,
+# and anyone who views the page and clicks runs script in their own session.
+#
+# `format:` is not anchored. Even with the scheme restricted, as `BroadcastAnnouncement#link`
+# already had it, the regexp matches a *substring* -- so `"javascript:alert(1) http://decoy.com"`
+# satisfied it, and that field is rendered as the "More info" link on every user's dashboard.
+#
+# Brakeman found the first of these (LinkToHref, weak confidence) once its version was bumped by a
+# merge from main. The second was found by testing the fix.
+module HttpUrlValidatable
+  extend ActiveSupport::Concern
+
+  # Anchored, and http(s) only. Everything else -- `javascript:`, `data:`, `file:` -- is rejected.
+  HTTP_URL = /\A#{URI::DEFAULT_PARSER.make_regexp(%w[http https])}\z/
+
+  class_methods do
+    def validates_http_url(*attributes, message: "should look like 'https://www.example.com'")
+      validates(*attributes, format: {with: HTTP_URL, message: message}, allow_blank: true)
+    end
+  end
+end

app/models/organization.rb

@@ -42,6 +42,8 @@
 #
 
 class Organization < ApplicationRecord
+  include HttpUrlValidatable
+
   has_paper_trail
   resourcify
 
@@ -51,7 +53,9 @@ class Organization < ApplicationRecord
   self.ignored_columns += ["short_name"]
 
   validates :name, presence: true
-  validates :url, format: { with: URI::DEFAULT_PARSER.make_regexp, message: "it should look like 'http://www.example.com'" }, allow_blank: true
+  # Rendered with `link_to` on the organization page, so the scheme has to be restricted --
+  # see HttpUrlValidatable.
+  validates_http_url :url, message: "it should look like 'http://www.example.com'"
   validates :email, format: { with: URI::MailTo::EMAIL_REGEXP }, allow_blank: true
   validate :correct_logo_mime_type
   validate :some_request_type_enabled

app/views/admin/broadcast_announcements/_broadcast_announcement.html.erb

@@ -1,6 +1,6 @@
 <tr id="<%= dom_id broadcast_announcement %>">
   <td><%= broadcast_announcement.message %></td>
-  <td><%= link_to  broadcast_announcement.link, broadcast_announcement.link %></td>
+  <td><%= essentials_external_link broadcast_announcement.link %></td>
   <td><%= broadcast_announcement.user&.name || "N/A" %></td>
   <td><%= broadcast_announcement.expiry %>
     <% if broadcast_announcement.expired? %>

app/views/broadcast_announcements/_broadcast_announcement.html.erb

@@ -1,6 +1,6 @@
 <tr id="<%= dom_id broadcast_announcement %>">
   <td><%= broadcast_announcement.message %></td>
-  <td><%= link_to  broadcast_announcement.link, broadcast_announcement.link %></td>
+  <td><%= essentials_external_link broadcast_announcement.link %></td>
   <td><%= broadcast_announcement.user&.name || "N/A" %></td>
   <td class="date text-left"><%= broadcast_announcement.expiry %>
     <% if broadcast_announcement.expired? %>

app/views/dashboard/_announcements.html.erb

@@ -13,12 +13,15 @@
             <%= announcement.created_at.strftime(announcement.created_at.year == Date.current.year ? "%B %-d" : "%B %-d %Y") %>
           </p>
           <p class="mt-0.5 text-sm text-slate-700"><%= announcement.message %></p>
-          <% if announcement.link.present? %>
+          <%# The href is guarded rather than the whole link: this one carries its own classes
+              and accessible name. An announcement link is the widest exposure of the three --
+              every user's dashboard renders it. %>
+          <% if (announcement_href = essentials_safe_href(announcement.link)) %>
             <%# The visible words stay short because the card is a compact grid cell, and the
                 accessible name carries the context WCAG 2.4.4 wants -- "More info" on its own
                 says nothing in a screen reader's list of links. The visible text is a prefix of
                 the accessible name, which is what WCAG 2.5.3 Label in Name requires. %>
-            <%= link_to "More info", announcement.link,
+            <%= link_to "More info", announcement_href,
                   aria: {label: "More info about the announcement of #{announcement.created_at.strftime("%B %-d")}"},
                   class: "mt-2 inline-flex items-center gap-1 text-sm font-medium link-brand focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-brand-600" %>
           <% end %>

app/views/organizations/_details.html.erb

@@ -26,9 +26,9 @@
     <%= essentials_detail "Name", @organization.name %>
     <%= essentials_detail "NDBN membership ID", @organization.ndbn_member&.full_name %>
     <%= essentials_detail "URL" do %>
-      <% if @organization.url.present? %>
-        <%= link_to @organization.url, @organization.url, class: "link-brand" %>
-      <% end %>
+      <%# Guarded: a `javascript:` URL stored before HttpUrlValidatable existed would otherwise
+          render as a live link here. This is the site Brakeman's LinkToHref found. %>
+      <%= essentials_external_link @organization.url %>
     <% end %>
     <%= essentials_detail "Email" do %>
       <% if @organization.email.present? %>

design.md

@@ -471,6 +471,30 @@ Target is **WCAG 2.2 AA**. These are the rules this app has actually had to enfo
   link that is its own block — a table cell, a list item, a card row — takes no *permanent*
   underline, because there is no adjacent body text to be confused with and a whole underlined
   column is noise; the hover and focus cue covers it.
+<a id="user-supplied-links"></a>
+- **A URL a user typed is never put straight into an `href`.** `essentials_external_link` for a
+  link that is nothing but the URL, `essentials_safe_href` where the call site builds its own link
+  and needs to keep its classes and accessible name. Both accept `http` and `https` and nothing
+  else.
+
+  This is not theoretical. `Organization#url` validated with `URI::DEFAULT_PARSER.make_regexp` and
+  no scheme argument, which accepts **any** scheme, so `javascript:alert(document.cookie)` was a
+  valid organization URL — and the organization page rendered that field with `link_to`. An
+  organization admin sets it; anyone who views the page and clicks runs script in their own
+  session. Brakeman's `LinkToHref` found it, at weak confidence, once a merge from `main` bumped
+  its version.
+
+  **And restricting the scheme is not enough on its own.** `format:` is not anchored, so the
+  pattern matches a *substring*: `BroadcastAnnouncement#link` already restricted the scheme to
+  `http https` and still accepted `"javascript:alert(1) http://decoy.example.com"` — a field
+  rendered as the "More info" link on every user's dashboard. `HttpUrlValidatable::HTTP_URL` is
+  anchored.
+
+  Two layers, deliberately. The validation guards one write path; the helper guards the render, for
+  a row that arrived by import, by console, or from a database restored from before the validation
+  existed. A dangerous URL renders as **plain text rather than nothing**, because the reader should
+  still see what the field holds — a bank looking at a nonsense URL is how it gets corrected.
+
 <a id="inert-on-arrival"></a>
 - **A control that leads nowhere is disabled only when pressing it would cost something.** Several
   can be pressed before they can do anything: Today on a calendar that opens on today, "Reset