Build and verify stage 0, and reclassify the builder

Stage 0 assembles off origin/main and stands up: the HttpUrlValidatable concern,
three models, safe_http_url, two specs. 41 examples, rubocop clean, brakeman 0
warnings, and javascript:alert(1) rejected where http:// is accepted.

Building it corrected the plan a third time. The two views that render
BroadcastAnnouncement#link are the widest exposure and are vulnerable on main --
they were the reason for a render guard at all -- and they cannot be extracted,
because the migration rewrote them entirely into essentials_status_pill and
essentials_row_icon_*. Taking the files would drag the migration into a security
PR. The check for that caught it: 8 design-system references in a stage that
must not have any.

So stage 0 stops every new bad row, and there are 0 today. Main's two render
sites wait for the announcements area. Guarding them sooner is a two-line hand
patch, not an extract, and is deliberately outside the script.

Also recorded in the plan: verifying a stage costs a bundle install each way,
because main's lockfile wants sprockets, terser and execjs that this branch
removed.

build-stage.rb is no longer declared as an audit. It reports nothing about the
app, so it belongs in which-audits' NOT_AN_AUDIT list with a reason -- giving it
an AUDIT-READS line to satisfy the ratchet was the wrong way to quiet the check,
and it made the selector tell people to run a branch builder.

page-audit 0 defects; seam-check 0; audit-selftest 13/6/0; state.rb --check up
to date. Nothing pushed anywhere but design.

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

Author: claude

bin/design/build-stage.rb

@@ -1,8 +1,6 @@
 #!/usr/bin/env ruby
 # frozen_string_literal: true
 
-# AUDIT-READS: AUDITS
-#
 # Assembles one stage of docs/review-plan.md as a local branch off `main`.
 #
 # **Why this exists rather than a set of pushed branches.** Branches that are not pushed do not
@@ -26,6 +24,7 @@
 
 ROOT = File.expand_path("../..", __dir__)
 def git(*args) = `git -C #{ROOT} #{args.map { |a| Shellwords.escape(a) }.join(" ")} 2>&1`
+
 def git!(*args)
   out = git(*args)
   abort "git #{args.join(" ")} failed:\n#{out}" unless $?.success?

bin/design/which-audits.rb

@@ -61,7 +61,8 @@
   "state.rb" => "regenerates the change log's Current state table; gated by its own --check",
   "status.rb" => "a report, not a check: which controllers render on a design system layout",
   "audit.js" => "a one-off page inspector for a human, documented as an exception in README.md",
-  "which-audits.rb" => "this script"
+  "which-audits.rb" => "this script",
+  "build-stage.rb" => "assembles a review-plan stage as a branch; it reports nothing about the app"
 }.freeze
 
 def matches?(patterns, path)

docs/changelog.md

@@ -360,6 +360,7 @@ zero across these commits; several were pre-existing bugs the old markup had bee
 | `fe03e1254` | **The migration was finished and the check could not tell: 347 of 384 was a measurement error, and the six views it named were all migrated.** Asked what remained, I read the files `status.rb` reported — **all six were done, and the check was wrong six times out of six**. Three were bare `f.input`, which is the *finished* state rather than an unmigrated one, because `config.default_wrapper = :essentials` makes the wrapper supply the div, the label, the control classes and the error — each of those three files carries a comment saying the hand-pasted classes were *removed* for that reason. The others carried a data-table cell class (`class="percent"`), a Stimulus controller (`data-controller="highchart"`), and a helper call whose argument was a variable rather than the symbol literal the pattern wanted. **Widening it was the wrong instinct, and I did it three times before noticing.** The page wrapper, `link-brand`, `EssentialsUiHelper` and the cell conventions took 347 → 360; resolving "renders a migrated partial", which the check's own comment already claimed it did, took it to 364; simple_form builder calls took it to 346 of 349 — and the last three files would have needed a fourth pass. The comment in the file records that the pattern had **already** been widened once, for 51 pages, for the same reason. **That is a check that cannot be finished**: the design system gains vocabulary with every component, so a positive-marker test is permanently one behind, and every miss reports completed work as outstanding. **So the measure is inverted.** The legacy vocabulary is closed — ADR 0011 deleted both frameworks and they cannot gain words — so the question is now *does this view still use the old system?*, which is what "is the migration done" means, and it reads **0 of 350**. The positive count survives as a labelled indicator, because a four-line table row can be migrated and have nothing to mark. Files that cannot carry a marker for the system they *are* — the 34 components in `shared/essentials/` plus gem and framework partials — are named separately rather than counted as failures, because a silent exclusion reads like an oversight. **The first draft of the inverted check was wrong too**, and the controls caught it: it flagged `admin/dashboard`, `partners/_show_header` and `reports/index`, all three of which carry `card-surface` — ours — because `\bcard\b` matches the "card" inside it. `float-right` came out as well, being a Tailwind utility as much as a Bootstrap one. **Thirteen controls**, one per legacy group, plus those two which must stay silent; without them the inversion would have traded six false negatives for three false positives. `state.rb` reads the new line and says `n/a` rather than 0 when the parse fails, proven against clean, 4-legacy and crashed output. Both status tables corrected — the change log's generated one and `migration-map.md`'s hand-maintained copy, whose own paragraph had guessed "the 37 are not a backlog" correctly and never checked. |
 | `b4e217f4b` | **A stored XSS, found by running 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, of which the design suite covers four. **I had never run `brakeman` or `factory-bot-lint`.** 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 — and that field is rendered with `link_to`. An organization admin sets it; anyone who views the page and clicks runs script in their own session. Weak confidence describes the static analysis, not the exposure. **The more interesting half is that restricting the scheme is not enough.** `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 already there. **`HttpUrlValidatable`** is anchored and `http https` only, on all three fields that take a URL from a user (`Organization#url`, `BroadcastAnnouncement#link`, `AccountRequest#organization_website`), and **`essentials_external_link` / `essentials_safe_href`** guard the four render sites. Two layers deliberately: the validation guards one write path, and a row can arrive by CSV import, from the console, or from a backup predating it. The helper is split because the dashboard link carries its own classes and accessible name and needs the href checked rather than the anchor built for it. A dangerous URL renders as **plain text rather than nothing** — dropping it hides the problem from the only people who can fix it. **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 examples, and **21 of 36 watched failing** against the old pattern. Brakeman 1 warning → **0, exit 0**; `factory_bot:lint` exit 0. 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. The post-merge routine has a step 9 now, naming both. |
 | `b666920d4` | **A staged plan for landing 602 commits, and executing it corrected it twice.** `CONTRIBUTING.md` asks for PRs "limited to one particular issue"; this branch is **1,051 files, +89,606/−24,093**, and one pull request of that size gets approved on trust or left to rot. `docs/review-plan.md` splits it into stages that each leave `main` working: foundation, audit suite, fifteen per-area PRs, retire the old stack, docs. **The order is forced by one fact** — `main` has 20 Sass files and Propshaft compiles no Sass, so the pipeline switch cannot precede the view migration. The branch's own history proves the order works: Tailwind entered at commit **2 of 602** alongside Sprockets, Sass left at **22**, Propshaft arrived at **114**. Composition measured rather than guessed: **41% of added lines are documentation**, and **221 of 602 commits are "Fill in the change log hash"** bookkeeping, leaving 376 substantive — so stages are reviewed as diffs, not commit sequences. **Then I began, and stage 1 disproved itself.** It was to be the ADRs alone — reasoning first, arguable before any code. Built against `main` it failed a link check: **ADR 0010 links to `design.md`**, which arrives with the foundation, and dropping 0010 broke a second link because **0010 and 0011 reference each other**. The ADRs and the spec are one unit; they are stage 1 now, and ADR 0012 travels with the Propshaft change it describes. **The second correction is worse and quieter**: the obvious way to assemble a stage is `git checkout design -- <paths>`, and the tip's `Gemfile` has Propshaft and **no** Sass, Bootstrap or Sprockets — stage 4's removals baked in. Taking it would land stage 4 inside stage 1 and unstyle every unconverted screen. The state each stage needs is the one the branch was in when that stage finished, and it exists: `cda053539` carries `bootstrap`, `sass-rails`, `sprockets` **and** `tailwindcss-rails` together with all twenty `.scss` files. Both findings are in the plan, and the per-stage checklist gained two steps ahead of the tests: every relative `.md` link must resolve against that stage's own files, and no later stage's removals may be present — check `Gemfile` and `app/assets/`. **Stage 0 is not the migration at all**: the stored XSS is live on `main` today, same validations and the same `link_to` in two views, so it should go first and alone — but there is no `SECURITY.md` and no disclosure channel, and a public PR announcing it to an app used by 200+ non-profits is a decision for the maintainers, not for me. |
+| `TBD` | **Stage 0 built and verified against `main`, and building it corrected the plan a third time.** `ruby bin/design/build-stage.rb 0` assembles it off `origin/main`: the `HttpUrlValidatable` concern, three models, `safe_http_url`, two specs — **41 examples, rubocop clean, brakeman 0 warnings**, and `javascript:alert(1)` rejected where `http://` is accepted. **The render guard moved out of the design system first**, because the two views that render `BroadcastAnnouncement#link` are the widest exposure and are vulnerable on `main`, and having the fix depend on `EssentialsUiHelper` meant they could only be repaired as part of a UI migration — the wrong dependency for a security fix. `safe_http_url` is in `ApplicationHelper` now and `essentials_safe_href` delegates to it. **Then the correction**: those two views still cannot go in stage 0, because the migration rewrote them entirely into `essentials_status_pill` and `essentials_row_icon_*`, so taking the files drags the migration into a security PR. The check caught it — **8 design-system references in a stage that must not have any**. Stage 0 therefore stops every new bad row (there are **0** today) and leaves `main`'s two render sites until the announcements area converts; guarding them sooner is a two-line hand patch, not an extract, and is deliberately outside the script. **`bin/design/build-stage.rb` is the durable form of this work**: unpushed branches do not survive a tree that has been rolled back eight times, so the stages are reproducible from a committed script instead. Each stage names the commit its files come from, because the tip has every later stage's *removals* baked in — its `Gemfile` carries Propshaft and no Sass, Bootstrap or Sprockets. A marker check refuses a source commit carrying a later stage's changes, **proven by pointing stage 1 at the tip: four complaints, exit 2**. Two bugs of my own found by running it: it resolved `HEAD` *after* switching branches, so the files a stage adds did not exist to check out; and a commit message with backticks was mangled by the shell. Also recorded: verifying a stage costs a `bundle install` each way, because `main`'s lockfile wants `sprockets`, `terser` and `execjs` that this branch removed. Nothing pushed anywhere but `design`. |
 
 
 

docs/review-plan.md

@@ -52,8 +52,27 @@ exactly what makes area-by-area PRs possible.
 `main` render `link_to broadcast_announcement.link, broadcast_announcement.link`. **The
 vulnerability is live on `main` today**, independent of anything on this branch.
 
-The model half of the fix applies to `main` unchanged. The render guard depends on
-`EssentialsUiHelper` and travels with stage 2.
+**Built and verified against `main` on 2026-09-09**, seven files: the `HttpUrlValidatable` concern,
+the three models, `safe_http_url` in `ApplicationHelper`, and two specs. 41 examples, rubocop clean,
+**brakeman 0 warnings**, and `javascript:alert(1)` rejected where `http://` is accepted. Reproduce
+it with `ruby bin/design/build-stage.rb 0`.
+
+**What it does not contain, and this is the third thing building the plan corrected.** The two
+views that render `BroadcastAnnouncement#link` are the widest exposure and are vulnerable on `main`
+— they were the reason for including a render guard at all. They cannot be extracted: the migration
+rewrote them completely, into `essentials_status_pill`, `essentials_row_icon_link` and
+`essentials_row_icon_action`, so taking the file drags the migration into a security PR. The check
+for that caught it — **8 design-system references in a stage that must not have any**.
+
+So stage 0 stops every *new* bad row, and there are **0** bad rows today across all three fields.
+`main`'s two render sites stay as they are until the announcements area is converted. If the
+maintainers want them guarded sooner, that is a two-line hand-written patch against `main`, not an
+extract from this branch, and it is deliberately not in the script.
+
+**Verifying a stage costs a `bundle install` each way.** `main`'s lockfile wants `sprockets`,
+`terser` and `execjs`, which this branch removed, so the gems have to be installed to run the
+stage's specs and reinstalled to come back. Budget for it; do not skip the verification because of
+it.
 
 **The question, which is not mine to answer**: there is no `SECURITY.md` and no disclosure channel
 in `CONTRIBUTING.md`. Opening a public pull request titled "fix stored XSS" tells everyone watching