perf: index the listing query, add HTTP validators, escape untrusted fields

app.rb

@@ -41,6 +41,48 @@ def self.log_device
     quiet_logs? ? IO::NULL : $stdout
   end
 
+  # 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
+    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. 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?
+
+      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, 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
+      etag Digest::SHA256.hexdigest("#{newest&.to_f}-#{records.map(&:id).join(',')}")
+    end
+  end
+
   before do
     @title = 'HackiNews'
   end
@@ -50,8 +92,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 +111,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

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 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';
+
+-- migrate:down
+drop index index_items_on_story_time;

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
 );
 
 
@@ -52,14 +56,24 @@ 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
 --
 
+\unrestrict dbmate
+
 
 --
 -- Dbmate schema migrations
 --
 
 INSERT INTO public.schema_migrations (version) VALUES
-    ('20200229014547');
+    ('20200229014547'),
+    ('20260830030000');

tests/app_test.rb

@@ -234,4 +234,181 @@ 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: '<script>alert(1)</script>', score: 100
+
+    get '/'
+
+    refute_includes last_response.body, '<script>alert(1)</script>'
+    assert_includes last_response.body, '&lt;script&gt;'
+  end
+
+  def test_story_title_and_author_are_escaped_on_the_story_page
+    create_item id: 1, title: '<img src=x onerror=alert(1)>', score: 100, by: '<b>evil</b>'
+
+    get '/stories/1'
+
+    refute_includes last_response.body, '<img src=x onerror=alert(1)>'
+    refute_includes last_response.body, '<b>evil</b>'
+    assert_includes last_response.body, '&lt;img src=x'
+  end
+
+  def test_comment_author_is_escaped
+    create_item id: 2, type: 'comment', by: '<script>x</script>', text: 'hi'
+    create_item id: 1, score: 100, kids: [2]
+
+    get '/stories/1'
+
+    refute_includes last_response.body, '<script>x</script>'
+  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 <a href="https://x.test">this</a>'
+    create_item id: 1, score: 100, kids: [2]
+
+    get '/stories/1'
+
+    assert_includes last_response.body, '<a href="https://x.test">this</a>'
+  end
+
+  def test_javascript_urls_are_not_rendered_as_links
+    create_item id: 1, title: 'Bad', score: 100, url: 'javascript:alert(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
+
+  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
+    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&amp;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'
+    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
+    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_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: '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
+
+    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 --------------------------------------------------------
+
+  # 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

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)

views/_comments.erb

@@ -6,7 +6,7 @@
           <% if comment.hidden? %>
             <span class="uk-text-meta">[deleted]</span>
           <% else %>
-            <%= comment.data['by'] %>
+            <%= h comment.data['by'] %>
           <% end %>
           <% comment_time = Time.at(comment.data['time']).utc %>
           <a href="#<%= comment.data['id'] %>" class="uk-text-meta comment" title="<%= comment_time %>">

views/index.erb

@@ -21,22 +21,22 @@
         <tr id="<%= story.data['id'] %>" date="<%= story.data['time'] %>">
           <td class="uk-text-center" colspan=2>
             <div class="" uk-grid>
-              <a href="<%= "/stories/#{story.id}" || story.comments_url %>" class="comments uk-visible@s uk-text-right uk-display-inline-block uk-width-1-2">
+              <a href="/stories/<%= story.id %>" class="comments uk-visible@s uk-text-right uk-display-inline-block uk-width-1-2">
                 <%= story.data['descendants'] %>
               </a>
-              <a href="<%= "/stories/#{story.id}" || story.comments_url %>" 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 %>">
+              <a href="/stories/<%= story.id %>" 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'] %>
               </a>
             </div>
           </td>
           <td class="story">
-            <% if story.data['url'] %>
-              <a href="<%= story.data['url'] %>" class="uk-display-block">
-                <%= story.data['title'] %>
-                <span class="uk-text-meta">(<%= story.truncated_url %>)</span>
+            <% if (story_url = safe_url(story.data['url'], item_id: story.id)) %>
+              <a href="<%= story_url %>" class="uk-display-block">
+                <%= h story.data['title'] %>
+                <span class="uk-text-meta">(<%= h story.truncated_url %>)</span>
               </a>
             <% else %>
-              <a href="https://news.ycombinator.com/item?id=<%= story.data['id'] %>"><%= story.data['title'] %></a>
+              <a href="https://news.ycombinator.com/item?id=<%= story.data['id'] %>"><%= h story.data['title'] %></a>
             <% end %>
           </td>
         </tr>

views/layout.erb

@@ -1,7 +1,7 @@
 <!doctype html>
 <html>
   <head>
-    <title><%= @title %></title>
+    <title><%= h @title %></title>
     <meta name="viewport" content="width=device-width, initial-scale=1">
 
     <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/uikit@3.3.3/dist/css/uikit.min.css" />