From 4af02201d01ec500e0a6884b521d050adcb78a28 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 31 Aug 2026 06:39:27 -0700 Subject: [PATCH 1/2] Fail closed when an upload's owning book can't be resolved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensure_attachment_readable listed @book.nil? as the first allowed condition, so an attachment whose owning book couldn't be resolved short-circuited the check and was served — the opposite of the unpublished-uploads-stay-private invariant the guard exists to hold. A live attachment reaches this state when its leaf is severed without the destroy cascade that would otherwise purge it. Drop the nil branch and use safe navigation, so a nil book fails the check and 404s. Published/accessible books are unaffected. --- .../action_text/markdown/uploads_controller.rb | 6 ++++-- .../markdown/uploads_controller_test.rb | 15 +++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/app/controllers/action_text/markdown/uploads_controller.rb b/app/controllers/action_text/markdown/uploads_controller.rb index f7d4292a..3da17c36 100644 --- a/app/controllers/action_text/markdown/uploads_controller.rb +++ b/app/controllers/action_text/markdown/uploads_controller.rb @@ -51,8 +51,10 @@ def set_attachment # An unpublished book's uploads are as private as the book itself. Serving them to # anyone holding the URL made this a way to read them without an access row, and - # caching them publicly for a year put them in shared caches besides. + # caching them publicly for a year put them in shared caches besides. An attachment + # whose owning book can't be resolved has no book to authorize against, so fail + # closed rather than letting the nil short-circuit the check. def ensure_attachment_readable - head :not_found unless @book.nil? || @book.published? || @book.accessable? + head :not_found unless @book&.published? || @book&.accessable? end end diff --git a/test/controllers/action_text/markdown/uploads_controller_test.rb b/test/controllers/action_text/markdown/uploads_controller_test.rb index 93472f32..46064664 100644 --- a/test/controllers/action_text/markdown/uploads_controller_test.rb +++ b/test/controllers/action_text/markdown/uploads_controller_test.rb @@ -131,6 +131,21 @@ class ActionText::Markdown::UploadsControllerTest < ActionDispatch::IntegrationT assert_response :not_found end + test "an attachment whose owning book can't be resolved is not served" do + books(:handbook).update! published: false + attachment = attach_upload_to_welcome_page + + # Sever the leaf without cascading the destroy, leaving the attachment live but + # its owning book unresolvable. The guard must fail closed on the nil book. + pages(:welcome).leaf.delete + assert_nil pages(:welcome).reload.owning_book + + reset! + get action_text_markdown_upload_url(slug: attachment.slug) + + assert_response :not_found + end + test "an attachment of an unpublished book is served to a reader, but not publicly cached" do books(:handbook).update! published: false attachment = attach_upload_to_welcome_page From 836336e2c14161f97ff1179635116cb5ef1699b6 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 31 Aug 2026 13:03:01 -0700 Subject: [PATCH 2/2] Use _path helper in the orphaned-attachment test Matches the AGENTS.md controller/integration-test convention; no cross-host or absolute-URL behavior is under test here. --- .../controllers/action_text/markdown/uploads_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/action_text/markdown/uploads_controller_test.rb b/test/controllers/action_text/markdown/uploads_controller_test.rb index 46064664..029cd30d 100644 --- a/test/controllers/action_text/markdown/uploads_controller_test.rb +++ b/test/controllers/action_text/markdown/uploads_controller_test.rb @@ -141,7 +141,7 @@ class ActionText::Markdown::UploadsControllerTest < ActionDispatch::IntegrationT assert_nil pages(:welcome).reload.owning_book reset! - get action_text_markdown_upload_url(slug: attachment.slug) + get action_text_markdown_upload_path(slug: attachment.slug) assert_response :not_found end