diff --git a/app.rb b/app.rb
index 6b9a2b3..f2b27a4 100644
--- a/app.rb
+++ b/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
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..822e726
--- /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 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;
diff --git a/db/schema.sql b/db/schema.sql
index b30cd13..2c2c616 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
);
@@ -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');
diff --git a/tests/app_test.rb b/tests/app_test.rb
index 929c14b..f98b56c 100644
--- a/tests/app_test.rb
+++ b/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: '', 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)'
+
+ 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&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
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 %>