From a34ebb47b5d171dbd1fa54d8d4a4251f47049510 Mon Sep 17 00:00:00 2001 From: Eric Boehs Date: Sat, 29 Aug 2026 21:46:05 -0500 Subject: [PATCH 1/2] perf: index the listing query, add HTTP validators, escape untrusted 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. --- app.rb | 41 ++++++- .../20260830030000_add_story_time_index.sql | 12 ++ db/schema.sql | 11 +- tests/app_test.rb | 116 ++++++++++++++++++ tests/test_helper.rb | 4 +- views/_comments.erb | 2 +- views/index.erb | 12 +- views/layout.erb | 2 +- views/story.erb | 10 +- 9 files changed, 192 insertions(+), 18 deletions(-) create mode 100644 db/migrations/20260830030000_add_story_time_index.sql diff --git a/app.rb b/app.rb index 6b9a2b3..5814d62 100644 --- a/app.rb +++ b/app.rb @@ -41,6 +41,37 @@ def self.log_device quiet_logs? ? IO::NULL : $stdout end + # HN item text is HTML and is rendered raw by design. Everything else the API + # gives us is attacker-influenced plain text and must be escaped. + SAFE_URL_SCHEMES = %w[http https].freeze + + helpers do + def h(value) + Rack::Utils.escape_html value.to_s + end + + # Escaping alone does not make an href safe: a javascript: url survives + # escaping and still runs on click. + def safe_url(value) + uri = URI.parse value.to_s + return '' unless SAFE_URL_SCHEMES.include?(uri.scheme) + + h uri.to_s + rescue URI::InvalidURIError + '' + end + + # The cache only changes when the worker writes, so the newest row plus the + # row count is a sound validator. Repeat requests then skip rendering + # entirely, which matters most on the largest threads (~1.4 MB of HTML). + def cache_for(records) + cache_control :public, :must_revalidate, max_age: 30 + newest = records.filter_map(&:updated_at).max + last_modified newest if newest + etag "#{newest&.to_f}-#{records.size}" + end + end + before do @title = 'HackiNews' end @@ -50,8 +81,12 @@ def self.log_device @min_score = (params['min_score'] || 50).to_i @next_min_score = MIN_SCORES[(MIN_SCORES.index(@min_score) || 0) + 1] || Float::INFINITY - @stories = Item.min_score(@min_score).by_time.limit 500 + # .stories is what lets the partial index apply, and it stops the listing + # depending on the accident that comments carry no score key. + @stories = Item.stories.min_score(@min_score).by_time.limit 500 @stories = @stories.title_matches @q if @q + @stories = @stories.to_a + cache_for @stories erb :index end @@ -65,12 +100,14 @@ def self.log_device # One query for the whole thread; every record shares the index, so # rendering nested replies issues no further queries. - @story = Item.thread(id)[id] + thread = Item.thread(id) + @story = thread[id] unless @story @not_found_message = 'Story not found. It may have been pruned from the cache.' halt 404 end + cache_for thread.values @title += " - #{@story.data['title']}" erb :story end diff --git a/db/migrations/20260830030000_add_story_time_index.sql b/db/migrations/20260830030000_add_story_time_index.sql new file mode 100644 index 0000000..b190893 --- /dev/null +++ b/db/migrations/20260830030000_add_story_time_index.sql @@ -0,0 +1,12 @@ +-- migrate:up + +-- The homepage reads the newest stories above a score threshold. Without this +-- it sequentially scans every cached row (46k+) to return a few hundred +-- stories. Partial on type so the index stays roughly the size of the story +-- count rather than the whole cache, and ordered to match `by_time`. +create index index_items_on_story_time + on items ((data->'time') desc) + where (data->>'type') = 'story'; + +-- migrate:down +drop index index_items_on_story_time; diff --git a/db/schema.sql b/db/schema.sql index b30cd13..30dcf39 100644 --- a/db/schema.sql +++ b/db/schema.sql @@ -1,8 +1,12 @@ +\restrict dbmate + -- Dumped from database version 16.15 (Postgres.app) +-- Dumped by pg_dump version 18.6 (Postgres.app) SET statement_timeout = 0; SET lock_timeout = 0; SET idle_in_transaction_session_timeout = 0; +SET transaction_timeout = 0; SET client_encoding = 'UTF8'; SET standard_conforming_strings = on; SELECT pg_catalog.set_config('search_path', '', false); @@ -32,7 +36,7 @@ CREATE TABLE public.items ( -- CREATE TABLE public.schema_migrations ( - version character varying NOT NULL + version character varying(128) NOT NULL ); @@ -56,10 +60,13 @@ ALTER TABLE ONLY public.schema_migrations -- PostgreSQL database dump complete -- +\unrestrict dbmate + -- -- Dbmate schema migrations -- INSERT INTO public.schema_migrations (version) VALUES - ('20200229014547'); + ('20200229014547'), + ('20260830030000'); diff --git a/tests/app_test.rb b/tests/app_test.rb index 929c14b..2c0c998 100644 --- a/tests/app_test.rb +++ b/tests/app_test.rb @@ -234,4 +234,120 @@ def test_out_of_range_story_id_returns_404 assert_equal 404, last_response.status end + + # --- escaping ------------------------------------------------------------- + + def test_story_titles_are_escaped_on_the_homepage + create_item id: 1, title: '', score: 100 + + get '/' + + refute_includes last_response.body, '' + assert_includes last_response.body, '<script>' + end + + def test_story_title_and_author_are_escaped_on_the_story_page + create_item id: 1, title: '', score: 100, by: 'evil' + + get '/stories/1' + + refute_includes last_response.body, '' + refute_includes last_response.body, 'evil' + assert_includes last_response.body, '<img src=x' + end + + def test_comment_author_is_escaped + create_item id: 2, type: 'comment', by: '', text: 'hi' + create_item id: 1, score: 100, kids: [2] + + get '/stories/1' + + refute_includes last_response.body, '' + end + + # HN comment bodies are HTML and are intentionally rendered raw. + def test_comment_html_is_still_rendered + create_item id: 2, type: 'comment', text: 'see this' + create_item id: 1, score: 100, kids: [2] + + get '/stories/1' + + assert_includes last_response.body, 'this' + end + + def test_javascript_urls_are_not_rendered_as_links + create_item id: 1, title: 'Bad', score: 100, url: 'javascript:alert(1)' + + get '/stories/1' + + refute_includes last_response.body, 'javascript:alert(1)' + end + + def test_ordinary_urls_still_render + create_item id: 1, title: 'Good', score: 100, url: 'https://example.test/a?b=1&c=2' + + get '/stories/1' + + assert_includes last_response.body, 'https://example.test/a?b=1&c=2' + end + + # --- caching -------------------------------------------------------------- + + def test_homepage_sends_cache_validators + create_item id: 1, title: 'A', score: 100 + + get '/' + + assert last_response.headers['ETag'], 'expected an ETag' + assert last_response.headers['Last-Modified'], 'expected Last-Modified' + assert_includes last_response.headers['Cache-Control'], 'public' + end + + def test_homepage_returns_304_when_unchanged + create_item id: 1, title: 'A', score: 100 + get '/' + etag = last_response.headers['ETag'] + + get '/', {}, 'HTTP_IF_NONE_MATCH' => etag + + assert_equal 304, last_response.status + assert_empty last_response.body + end + + def test_story_returns_304_when_unchanged + create_item id: 2, type: 'comment', text: 'hi' + create_item id: 1, score: 100, kids: [2] + get '/stories/1' + etag = last_response.headers['ETag'] + + get '/stories/1', {}, 'HTTP_IF_NONE_MATCH' => etag + + assert_equal 304, last_response.status + end + + def test_etag_changes_when_a_comment_is_added + create_item id: 1, score: 100 + get '/stories/1' + before = last_response.headers['ETag'] + + create_item id: 2, type: 'comment', text: 'new' + Item.find(1).update! data: Item.find(1).data.merge('kids' => [2]) + get '/stories/1' + + refute_equal before, last_response.headers['ETag'] + assert_equal 200, last_response.status + end + + # --- listing scope -------------------------------------------------------- + + # The listing used to rely on comments never carrying a score key. + def test_homepage_lists_only_stories + create_item id: 1, title: 'Real story', score: 100 + create_item id: 2, type: 'comment', text: 'not a story', score: 500 + + get '/' + + assert_includes last_response.body, 'Real story' + refute_includes last_response.body, 'not a story' + end end diff --git a/tests/test_helper.rb b/tests/test_helper.rb index 3d61290..d9206f5 100644 --- a/tests/test_helper.rb +++ b/tests/test_helper.rb @@ -31,10 +31,12 @@ def after_teardown module ItemFactory def create_item(id:, title: 'Hello', score: 100, type: 'story', kids: nil, updated_at: Time.now.utc, url: nil, time: Time.now.to_i, - deleted: false, dead: false) + deleted: false, dead: false, by: nil, text: nil) data = { 'id' => id, 'title' => title, 'score' => score, 'type' => type, 'time' => time } data['kids'] = kids if kids data['url'] = url if url + data['by'] = by if by + data['text'] = text if text data['deleted'] = true if deleted data['dead'] = true if dead Item.create!(id: id, data: data, created_at: updated_at, updated_at: updated_at) diff --git a/views/_comments.erb b/views/_comments.erb index 798e685..210a7d6 100644 --- a/views/_comments.erb +++ b/views/_comments.erb @@ -6,7 +6,7 @@ <% if comment.hidden? %> [deleted] <% else %> - <%= comment.data['by'] %> + <%= h comment.data['by'] %> <% end %> <% comment_time = Time.at(comment.data['time']).utc %> diff --git a/views/index.erb b/views/index.erb index f114f2b..b56061c 100644 --- a/views/index.erb +++ b/views/index.erb @@ -21,22 +21,22 @@
- " class="comments uk-visible@s uk-text-right uk-display-inline-block uk-width-1-2"> + <%= story.data['descendants'] %> - " class="points uk-text-right uk-display-inline-block uk-width-1-2 uk-margin-right-small <%= 'uk-text-primary' if story.data['score'] > @next_min_score %>"> + <%= story.data['score'] %>
<% if story.data['url'] %> - - <%= story.data['title'] %> - (<%= story.truncated_url %>) + + <%= h story.data['title'] %> + (<%= h story.truncated_url %>) <% else %> - <%= story.data['title'] %> + <%= h story.data['title'] %> <% end %> diff --git a/views/layout.erb b/views/layout.erb index ca16749..57f5d6e 100644 --- a/views/layout.erb +++ b/views/layout.erb @@ -1,7 +1,7 @@ - <%= @title %> + <%= h @title %> diff --git a/views/story.erb b/views/story.erb index 3dbe7e8..2515af7 100644 --- a/views/story.erb +++ b/views/story.erb @@ -1,17 +1,17 @@

<% if @story.data['url'] %> - - <%= @story.data['title'] %> - (<%= @story.truncated_url %>) + + <%= h @story.data['title'] %> + (<%= h @story.truncated_url %>) <% else %> - <%= @story.data['title'] %> + <%= h @story.data['title'] %> <% end %>

<%= @story.data['score'] %> points | - by <%= @story.data['by'] %> | + by <%= h @story.data['by'] %> | <%= @story.data['descendants'] %> Comments

From f1084425214df6ad3b3f66d9ba680eb477e37c57 Mon Sep 17 00:00:00 2001 From: Eric Boehs Date: Sat, 29 Aug 2026 21:56:34 -0500 Subject: [PATCH 2/2] fix: address review - schema dump, URI errors, ETag membership, dead 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. --- app.rb | 39 ++++++---- .../20260830030000_add_story_time_index.sql | 6 +- db/schema.sql | 7 ++ tests/app_test.rb | 75 +++++++++++++++++-- views/index.erb | 4 +- views/story.erb | 4 +- 6 files changed, 107 insertions(+), 28 deletions(-) diff --git a/app.rb b/app.rb index 5814d62..f2b27a4 100644 --- a/app.rb +++ b/app.rb @@ -41,8 +41,10 @@ def self.log_device quiet_logs? ? IO::NULL : $stdout end - # HN item text is HTML and is rendered raw by design. Everything else the API - # gives us is attacker-influenced plain text and must be escaped. + # HN item text is HTML and is rendered raw by design; this trusts HN's own + # sanitization. Any other source would have to be sanitized before render. + # Everything else the API gives us is attacker-influenced plain text and must + # be escaped. SAFE_URL_SCHEMES = %w[http https].freeze helpers do @@ -51,24 +53,33 @@ def h(value) end # Escaping alone does not make an href safe: a javascript: url survives - # escaping and still runs on click. - def safe_url(value) - uri = URI.parse value.to_s - return '' unless SAFE_URL_SCHEMES.include?(uri.scheme) + # escaping and still runs on click. Returns nil rather than "" so callers + # can fall back to a real link instead of rendering a dead one. + def safe_url(value, item_id: nil) + return nil if value.blank? - h uri.to_s - rescue URI::InvalidURIError - '' + uri = URI.parse value.to_s + return h(uri.to_s) if SAFE_URL_SCHEMES.include?(uri.scheme) + + App.logger.warn "Item #{item_id}: rejected url scheme #{uri.scheme.inspect}" + nil + rescue URI::Error => e + # InvalidComponentError is not a subclass of InvalidURIError, so the whole + # URI::Error hierarchy has to be caught or a bad url 500s the page. + App.logger.warn "Item #{item_id}: unparseable url (#{e.class})" + nil end - # The cache only changes when the worker writes, so the newest row plus the - # row count is a sound validator. Repeat requests then skip rendering - # entirely, which matters most on the largest threads (~1.4 MB of HTML). + # The cache only changes when the worker writes, and every write bumps + # updated_at, so the newest timestamp catches content edits. The id digest + # additionally catches membership changes that leave both the count and the + # newest timestamp untouched -- a story dropping below the score threshold + # while another takes its place. Last-Modified is deliberately not sent: + # its one-second granularity makes it a weaker validator than the ETag. def cache_for(records) cache_control :public, :must_revalidate, max_age: 30 newest = records.filter_map(&:updated_at).max - last_modified newest if newest - etag "#{newest&.to_f}-#{records.size}" + etag Digest::SHA256.hexdigest("#{newest&.to_f}-#{records.map(&:id).join(',')}") end end diff --git a/db/migrations/20260830030000_add_story_time_index.sql b/db/migrations/20260830030000_add_story_time_index.sql index b190893..822e726 100644 --- a/db/migrations/20260830030000_add_story_time_index.sql +++ b/db/migrations/20260830030000_add_story_time_index.sql @@ -1,9 +1,9 @@ -- migrate:up -- The homepage reads the newest stories above a score threshold. Without this --- it sequentially scans every cached row (46k+) to return a few hundred --- stories. Partial on type so the index stays roughly the size of the story --- count rather than the whole cache, and ordered to match `by_time`. +-- it sequentially scans the whole cache to return a few hundred stories, so +-- the cost grows with cached comments rather than with stories. Partial on +-- type so the index tracks the story count, and ordered to match `by_time`. create index index_items_on_story_time on items ((data->'time') desc) where (data->>'type') = 'story'; diff --git a/db/schema.sql b/db/schema.sql index 30dcf39..2c2c616 100644 --- a/db/schema.sql +++ b/db/schema.sql @@ -56,6 +56,13 @@ ALTER TABLE ONLY public.schema_migrations ADD CONSTRAINT schema_migrations_pkey PRIMARY KEY (version); +-- +-- Name: index_items_on_story_time; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_items_on_story_time ON public.items USING btree (((data -> 'time'::text)) DESC) WHERE ((data ->> 'type'::text) = 'story'::text); + + -- -- PostgreSQL database dump complete -- diff --git a/tests/app_test.rb b/tests/app_test.rb index 2c0c998..f98b56c 100644 --- a/tests/app_test.rb +++ b/tests/app_test.rb @@ -278,9 +278,34 @@ def test_comment_html_is_still_rendered def test_javascript_urls_are_not_rendered_as_links create_item id: 1, title: 'Bad', score: 100, url: 'javascript:alert(1)' - get '/stories/1' + capture_log { get '/stories/1' } + + assert_equal 200, last_response.status + assert_includes last_response.body, 'Bad' + refute_includes last_response.body, 'javascript:' + # Falls back to the HN discussion rather than rendering a dead href="". + assert_includes last_response.body, 'news.ycombinator.com/item?id=1' + end - refute_includes last_response.body, 'javascript:alert(1)' + def test_javascript_urls_are_not_rendered_on_the_homepage + create_item id: 1, title: 'Bad', score: 100, url: 'javascript:alert(1)' + + capture_log { get '/' } + + assert_equal 200, last_response.status + assert_includes last_response.body, 'Bad' + refute_includes last_response.body, 'javascript:' + end + + # mailto: raises URI::InvalidComponentError, which is not a subclass of + # URI::InvalidURIError and would otherwise 500 the page. + def test_unparseable_url_does_not_error_the_page + create_item id: 1, title: 'Odd', score: 100, url: 'mailto:x' + + capture_log { get '/stories/1' } + + assert_equal 200, last_response.status + assert_includes last_response.body, 'Odd' end def test_ordinary_urls_still_render @@ -299,8 +324,11 @@ def test_homepage_sends_cache_validators get '/' assert last_response.headers['ETag'], 'expected an ETag' - assert last_response.headers['Last-Modified'], 'expected Last-Modified' - assert_includes last_response.headers['Cache-Control'], 'public' + cache_control = last_response.headers['Cache-Control'] + + assert_includes cache_control, 'public' + assert_includes cache_control, 'must-revalidate' + assert_includes cache_control, 'max-age=30' end def test_homepage_returns_304_when_unchanged @@ -325,17 +353,50 @@ def test_story_returns_304_when_unchanged assert_equal 304, last_response.status end - def test_etag_changes_when_a_comment_is_added + def test_adding_a_comment_invalidates_the_story_cache create_item id: 1, score: 100 get '/stories/1' before = last_response.headers['ETag'] - create_item id: 2, type: 'comment', text: 'new' + create_item id: 2, type: 'comment', text: 'brand new reply' Item.find(1).update! data: Item.find(1).data.merge('kids' => [2]) + + get '/stories/1', {}, 'HTTP_IF_NONE_MATCH' => before + + assert_equal 200, last_response.status + assert_includes last_response.body, 'brand new reply' + end + + def test_editing_a_comment_invalidates_the_story_cache + create_item id: 2, type: 'comment', text: 'original' + create_item id: 1, score: 100, kids: [2] get '/stories/1' + before = last_response.headers['ETag'] + + Item.find(2).update! data: Item.find(2).data.merge('text' => 'edited text') + + get '/stories/1', {}, 'HTTP_IF_NONE_MATCH' => before - refute_equal before, last_response.headers['ETag'] assert_equal 200, last_response.status + assert_includes last_response.body, 'edited text' + end + + # A story leaving the score filter while another replaces it leaves both the + # row count and the newest timestamp unchanged, so the ETag must also depend + # on which rows are in the result. + def test_membership_change_invalidates_the_homepage_cache + stamp = 2.hours.ago + 501.times { |i| create_item id: i + 1, title: "Story #{i + 1}", score: 100, time: i, updated_at: stamp } + get '/' + before = last_response.headers['ETag'] + + # Drop one displayed story below the threshold without touching the newest. + demoted = Item.order(Arel.sql("data->'time' desc")).offset(10).first + demoted.update! data: demoted.data.merge('score' => 1), updated_at: stamp + + get '/', {}, 'HTTP_IF_NONE_MATCH' => before + + assert_equal 200, last_response.status, 'membership changed, must not 304' end # --- listing scope -------------------------------------------------------- diff --git a/views/index.erb b/views/index.erb index b56061c..cad52a6 100644 --- a/views/index.erb +++ b/views/index.erb @@ -30,8 +30,8 @@ - <% if story.data['url'] %> - + <% if (story_url = safe_url(story.data['url'], item_id: story.id)) %> + <%= h story.data['title'] %> (<%= h story.truncated_url %>) diff --git a/views/story.erb b/views/story.erb index 2515af7..f9adaf1 100644 --- a/views/story.erb +++ b/views/story.erb @@ -1,7 +1,7 @@