perf: index the listing query, add HTTP validators, escape untrusted fields - #30
Merged
Conversation
…fields
Three independent wins found while profiling.
The homepage sequentially scanned all 46k cached rows to return a few
hundred stories. It also filtered on score alone, so it worked only
because comments happen to carry no score key -- an accident, not a
rule. Scoping it to .stories fixes that and lets a partial index apply:
measured on 46,500 rows, Index Scan 0.196ms against Seq Scan 3.93ms.
The index is partial on type so it stays the size of the story count
rather than the whole cache.
Responses carried no validators, so every request re-rendered and
re-sent the page even when nothing had changed -- 1.4 MB for the largest
thread. The cache only changes when the worker writes, so the newest row
plus the row count is a sound ETag. Repeat requests now 304 without
rendering.
ERB was not escaping, and titles, authors and urls are attacker-
influenced. Nothing exploitable was cached (0 titles contained < or &),
but there was no defense either. Titles, authors and truncated urls are
now escaped. Comment bodies stay raw because HN sends HTML, which is
exactly why a blanket escape_html was the wrong fix. Escaping alone does
not make an href safe, so urls are scheme-checked: a javascript: url
renders as no link at all.
Also removes dead code in the listing: `"/stories/#{id}" || comments_url`
could never fall through, since the string is always truthy.
…hrefs Review caught a bug I introduced myself. While measuring the index I dropped it and re-ran dbmate, which re-dumped db/schema.sql without the index while leaving the migration recorded as applied. A database restored from that schema would skip the migration and silently run without the index forever. Regenerated the dump. safe_url only rescued URI::InvalidURIError, but URI::InvalidComponentError is not a subclass of it: a `mailto:` url escaped the rescue and 500'd the page. Verified, now rescues the URI::Error hierarchy and logs the item. Rejected urls returned "" and both views emitted href="", so the story title looked like a link and reloaded the current page. safe_url now returns nil and the views fall back to the Hacker News discussion, which is what a story with no url already does. The ETag was the newest timestamp plus the row count, which collides on membership changes: a story dropping below the score threshold while another takes its place leaves both inputs untouched and serves a stale 304. Confirmed by test -- it returns 304 with the old scheme and 200 with a digest of the row ids. Dropped Last-Modified rather than describe it as sound. Its one-second granularity makes it strictly weaker than the ETag, and sending both meant the weaker one could answer a conditional request on its own. Tests: homepage url rejection, unparseable urls, in-place comment edits, the membership collision, and the full Cache-Control contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three independent wins found while profiling, plus review fixes.
1. The listing query scanned the whole cache
The homepage sequentially scanned all 46k rows to return a few hundred stories, and it filtered on score alone — so it worked only because comments happen to carry no
scorekey. That's an accident, not a rule.Scoping to
.storiesfixes the correctness issue and lets a partial index apply. Measured on 46,500 rows:The index is partial on
typeso it tracks the story count (~500) rather than the whole cache.2. No HTTP validators
Every request re-rendered and re-sent the page even when nothing had changed — 1.4 MB for the largest thread. The cache only changes when the worker writes, so repeat requests now return 304 without rendering.
3. ERB wasn't escaping
Titles, authors and urls are attacker-influenced. Nothing exploitable was cached (0 titles contained
<or&), but there was no defense either. Comment bodies stay raw because HN sends HTML — which is exactly why a blanketescape_htmlwould have been the wrong fix.Escaping alone doesn't make an href safe, so urls are scheme-checked: a
javascript:url renders as no link at all.Review-driven fixes
A bug I introduced myself. While measuring the index I dropped it and re-ran dbmate, which re-dumped
schema.sqlwithout the index while leaving the migration recorded as applied. Any database restored from that schema would skip the migration and silently run without the index. Regenerated.mailto:returned a 500.URI::InvalidComponentErroris not a subclass ofURI::InvalidURIError, so it escaped my rescue. Verified against the real hierarchy.The ETag collided on membership changes. Newest-timestamp + row-count is unchanged when a story drops below the score threshold and another replaces it — a stale 304. I confirmed this by reverting the formula: the new test returns 304 with the old scheme and 200 with a digest of the row ids.
Dropped
Last-Modifiedrather than document it as sound. Its one-second granularity makes it strictly weaker than the ETag, and sending both let the weaker one answer conditional requests alone.Rejected urls rendered
href="", making a title look like a link that reloads the page. Now falls back to the HN discussion, matching what a story with no url already does.Tests: 118 runs, 314 assertions.