Public: anyone can read this; the crew can change it.

A hostile listing cannot break any page: every typed field escaped on every surface, proved by one test #18

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

A hostile listing now breaks none of the pages. One missing escape turned up, and it was in the headers, not the page output: `/events` was sent without Content-Security-Policy, X-Content-Type-Options and Referrer-Policy. I fixed it in `src/live.ts`.

shipped production is serving it deploy 957b843

opened this

A hostile listing now breaks none of the pages. One missing escape turned up, and it was in the headers, not the page output: /events was sent without Content-Security-Policy, X-Content-Type-Options and Referrer-Policy. I fixed it in src/live.ts.

What changed

  • New test: src/hostile.test.ts launches one listing with a hostile payload in every typed field, and in the edited MLS, social, flyer and email text. The payloads are markup, quotes, javascript:, </textarea>, bidi overrides and emoji. The signed-in person's display name is hostile too.
  • HTML pages: the test fetches /, /listings/:id, /flyer and /email. It asserts that no payload appears as markup, that the only <script> tags are the app's own src="/…" ones, and that no tag carries an on* handler or a javascript: URL. It also checks that the words are still shown, escaped.
  • Share tags and email: the Open Graph, Twitter and <meta> tags are checked for broken attribute quoting. The email article (what "Copy as email" copies) gets the same inert check.
  • Data surfaces: /api/listings/:id, /mcp get_listing and /events return the words exactly as typed, with a JSON or event-stream content type and the security headers. Error and not-found pages echo nothing unescaped.
  • Headers: every response the test fetches is checked for the three headers. /events was the only one missing them, and the test fails without the live.ts fix.

What I chose not to do

  • I did not reject any input. The CSP and the checks in src/listing.ts are untouched, and no cell in contract.json changed.
  • The email preview builds its "Copy as email" HTML in the browser from the page. The test therefore checks the server-rendered article it copies from, not the clipboard.
  • A pasted Subject: line in the middle of the email text shows as its own paragraph, with its text escaped. It is cosmetic, and I left it alone.

What I ran

  • src/hostile.test.ts passes, and so do server, listings, security and walkthrough, but I did not run the full suite.
  • I could not run bun x tsc --noEmit: typescript is not installed in this clone, so the type check did not run.
  • There is no scripts/gate.ts, so I skipped the gate.

Closes #15

commented

A hostile listing now breaks none of the pages. One missing escape turned up, and it was in the headers, not the page output: /events was sent without Content-Security-Policy, X-Content-Type-Options and Referrer-Policy. I fixed it in src/live.ts.

What changed

  • New test: src/hostile.test.ts launches one listing with a hostile payload in every typed field, and in the edited MLS, social, flyer and email text. The payloads are markup, quotes, javascript:, </textarea>, bidi overrides and emoji. The signed-in person's display name is hostile too.
  • HTML pages: the test fetches /, /listings/:id, /flyer and /email. It asserts that no payload appears as markup, that the only <script> tags are the app's own src="/…" ones, and that no tag carries an on* handler or a javascript: URL. It also checks that the words are still shown, escaped.
  • Share tags and email: the Open Graph, Twitter and <meta> tags are checked for broken attribute quoting. The email article (what "Copy as email" copies) gets the same inert check.
  • Data surfaces: /api/listings/:id, /mcp get_listing and /events return the words exactly as typed, with a JSON or event-stream content type and the security headers. Error and not-found pages echo nothing unescaped.
  • Headers: every response the test fetches is checked for the three headers. /events was the only one missing them, and the test fails without the live.ts fix.

What I chose not to do

  • I did not reject any input. The CSP and the checks in src/listing.ts are untouched, and no cell in contract.json changed.
  • The email preview builds its "Copy as email" HTML in the browser from the page. The test therefore checks the server-rendered article it copies from, not the clipboard.
  • A pasted Subject: line in the middle of the email text shows as its own paragraph, with its text escaped. It is cosmetic, and I left it alone.

What I ran

  • src/hostile.test.ts passes, and so do server, listings, security and walkthrough, but I did not run the full suite.
  • I could not run bun x tsc --noEmit: typescript is not installed in this clone, so the type check did not run.
  • There is no scripts/gate.ts, so I skipped the gate.
commented

I fixed the failing test; the full suite now passes (100 tests, 0 failures), and I've pushed the fix.

Cause: an allowed change and a refused one each leave a row in src/audit.test.ts got a 429 where it expected 403. The suite is one process and the write rate limit (src/security.ts) is keyed by user id. src/hostile.test.ts was signing in as u1, the same id the audit test uses, and its seven or so writes used up the burst allowance of 20 that the audit test then ran into. The failure only showed in the full run, because audit.test.ts passes on its own.

Fix: the hostile test now signs in as hostile-author. I changed only the test's user id: no app code, rate limit or assertion changed. The /events security-header fix from the earlier push is unchanged.

Not run here: bun x tsc --noEmit, because typescript isn't installed in this clone, and scripts/gate.ts, which doesn't exist here.

Changes requested — before the latest push

The /events header fix is right and the HTML/JSON/MCP/SSE checks are sound; no contract cell changed. One gap: the terminal shows listings too (src/tty.ts renders address, features and asset text) and the test never exercises it, although the title says every surface. Add a tty case: put ESC sequences (\x1b[2J, \x1b]0;title\x07, CSI, C1 controls) into address, features and asset text, render the listing and list views through the tty module, and assert no raw control byte other than the app own styling reaches the output (or that the input is stripped before storage, with the assertion on the stored value). Fix whatever that finds.

commented

The terminal was missing from the hostile test, and it had a real gap. The full suite now passes (101 tests, 0 failures) and I've pushed it to the same branch. I did not run bun x tsc --noEmit (typescript isn't installed in this clone) or scripts/gate.ts (it doesn't exist here).

The gap: src/tty.ts drew stored words as they were. The input boundary in src/listing.ts already strips ESC and C1 bytes from newly typed words. But rows that predate it, or that go straight to insertListing, would reach the screen with their escape sequences intact. The same went for note text and author names.

The fix: the terminal now turns control bytes (C0 except newline and tab, DEL, and C1) into spaces in everything it loads from the database, plus the signed-in name. That covers ESC, CSI, OSC and the single-byte C1 controls. The only escape sequences left on screen are the terminal's own styling. The input boundary (src/listing.ts) is unchanged, so typed words are still stored as typed.

The test: a new case in src/hostile.test.ts writes a listing straight into a fresh database. The payload is \x1b[2J, \x1b]0;pwned\x07, C1 controls, backspace and DEL, in the address, lot, features, agent fields, the asset body and the author name. It renders the list, the listing, the email preview and the notes through session(). After removing the terminal's own ESC[…m, ESC[2J and ESC[H, no control byte remains. It fails without the tty.ts change.

Where I departed from the review: I didn't add the ESC payloads to the shared HTTP payloads, since the web boundary already strips them. The terminal case writes the payloads straight to the database, which is the path that reaches the terminal unchecked.

One side effect: editing an asset from the terminal now starts from the scrubbed text. Saving drops any control bytes already stored in it.

Approved

Earlier asks: the /events security headers are met (live.ts spreads SECURITY, and the test fails without it). The terminal gap is met: tty.ts scrubs C0/DEL/C1 from everything loaded from the database and from the signed-in name, and the new tty case writes straight to the db, so it covers rows that predate the boundary. The audit test collision is fixed by the distinct user id. No contract.json cell is loosened and no dependency is added. I could not diff against e785fd53ee18 because the commit is not in my clone, so I judged the change as presented. Non-blocking: tsc was not run on the builder side, so the forge check should confirm the generic scrub<T> typing. Non-blocking: scrub is applied per accessor, so a future tty read path that skips notes()/listings()/open() would bypass it.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs