From a117673e54ec77b6053b2354263326f433946886 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 15:01:52 -0700 Subject: [PATCH 1/4] Keep trashed pages out of revision history Pages::EditsController resolves its leaf through PageLeafScoped, which looked up Current.user.leaves without the .active scope that every comparable lookup applies. Trashing a page records an edit that retains the old body, so the full content of a deleted page stayed readable through its history even though the page itself 404s on its own URL. Readers keep their existing access to the history of live pages. That is long-standing behavior and was assessed on its own terms: a reader can already read the page. It doesn't cover content an editor deleted, which is what this closes. --- app/controllers/concerns/page_leaf_scoped.rb | 5 ++- .../pages/edits_controller_test.rb | 44 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/app/controllers/concerns/page_leaf_scoped.rb b/app/controllers/concerns/page_leaf_scoped.rb index 957fba9d..89a868ea 100644 --- a/app/controllers/concerns/page_leaf_scoped.rb +++ b/app/controllers/concerns/page_leaf_scoped.rb @@ -4,7 +4,10 @@ module PageLeafScoped extend ActiveSupport::Concern end private + # Scoped to active leaves, like SetBookLeaf. Trashing a page records an edit that + # keeps the old body, so without this a trashed page's whole content stayed readable + # here even though the page itself 404s on its own URL. def set_leaf - @leaf = Current.user.leaves.find(params[:page_id]) + @leaf = Current.user.leaves.active.find(params[:page_id]) end end diff --git a/test/controllers/pages/edits_controller_test.rb b/test/controllers/pages/edits_controller_test.rb index d490be63..acd35646 100644 --- a/test/controllers/pages/edits_controller_test.rb +++ b/test/controllers/pages/edits_controller_test.rb @@ -35,6 +35,50 @@ class Pages::EditsControllerTest < ActionDispatch::IntegrationTest assert_no_match(/onerror/, response.body) end + test "a trashed page's history is not reachable" do + leaf = leaves(:welcome_page) + leaf.edit leafable_params: { body: "Embargoed announcement: Project X" } + leaf.trashed! + + get page_edit_url(leaf, "latest") + + assert_response :not_found + end + + test "a trashed page's history is not reachable by a reader either" do + leaf = leaves(:welcome_page) + leaf.edit leafable_params: { body: "Embargoed announcement: Project X" } + leaf.trashed! + + sign_in :jz + get page_edit_url(leaf, "latest") + + assert_response :not_found + end + + # Readers may still read the history of a live page. That is long-standing behavior + # and was assessed on its own terms: a reader can already read the page itself. The + # trashed case above is different, because that content is unreachable otherwise. + test "a reader may still read a live page's history" do + leaves(:welcome_page).edit leafable_params: { body: "Updated" } + + sign_in :jz + get page_edit_url(leaves(:welcome_page), "latest") + + assert_response :success + end + + test "a user with no access to the book gets nothing" do + leaf = leaves(:welcome_page) + leaf.edit leafable_params: { body: "Updated" } + accesses(:jz_handbook).destroy! + + sign_in :jz + get page_edit_url(leaf, "latest") + + assert_response :not_found + end + test "show sanitizes dangerous content in current version" do leaves(:welcome_page).edit leafable_params: { body: %() } From f7328f72d6ce6e78785b5adaed287d3da1353578 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 15:02:33 -0700 Subject: [PATCH 2/4] Authorize markdown uploads against current book access The uploads endpoint performed no authorization. It located a signed global id out of the request and attached the file, without consulting the Access table. The only gate was possession of the signed id that markdown_area renders into the page editor, minted with no purpose and no expiry, so it was a bearer capability that was never re-checked. A collaborator whose access had been revoked entirely kept writing files into that book's storage indefinitely. Uploads are now authorized against the book the record belongs to, at the time of the upload: no access row gives a 404, access below editor gives a 403. The signed id also carries a purpose and an expiry now, which bounds a leaked copy, though the authorization check rather than the token is what makes revocation take effect. Finding the book takes a small detour. Editing a page moves its leaf onto a copy and leaves the original held by the edit that records the revision, so a superseded page has no leaf of its own and has to reach its book through that edit. --- .../markdown/uploads_controller.rb | 20 +++- app/models/leafable.rb | 8 ++ lib/rails_ext/action_text_markdown.rb | 7 ++ lib/rails_ext/action_text_tag_helper.rb | 9 +- .../markdown/uploads_controller_test.rb | 93 ++++++++++++++++++- 5 files changed, 129 insertions(+), 8 deletions(-) diff --git a/app/controllers/action_text/markdown/uploads_controller.rb b/app/controllers/action_text/markdown/uploads_controller.rb index 52b55507..3903ead2 100644 --- a/app/controllers/action_text/markdown/uploads_controller.rb +++ b/app/controllers/action_text/markdown/uploads_controller.rb @@ -5,9 +5,9 @@ class ActionText::Markdown::UploadsController < ApplicationController ActiveStorage::Current.url_options = { protocol: request.protocol, host: request.host, port: request.port } end - def create - @record = GlobalID::Locator.locate_signed params[:record_gid] + before_action :set_record, :ensure_editable, only: :create + def create @markdown = @record.safe_markdown_attribute params[:attribute_name] @markdown.uploads.attach [ params[:file] ] @markdown.save! @@ -22,4 +22,20 @@ def show expires_in 1.year, public: true redirect_to @attachment.url end + + private + # The signed id rendered into the page editor says who could upload when it was + # minted, not who may upload now. Resolve the book it belongs to and authorize + # against that, so revoking access takes effect here like it does everywhere else. + def set_record + @record = GlobalID::Locator.locate_signed params[:record_gid], + only: Page, for: ActionText::Markdown::UPLOADS_SIGNED_ID_PURPOSE + @book = Book.accessable_or_published.find_by(id: @record&.owning_book&.id) + + head :not_found unless @book + end + + def ensure_editable + head :forbidden unless @book.editable? + end end diff --git a/app/models/leafable.rb b/app/models/leafable.rb index 6d90efd5..1335b308 100644 --- a/app/models/leafable.rb +++ b/app/models/leafable.rb @@ -6,10 +6,18 @@ module Leafable included do has_one :leaf, as: :leafable, inverse_of: :leafable, touch: true has_one :book, through: :leaf + has_one :edit, as: :leafable delegate :title, to: :leaf end + # Editing a page supersedes its leafable: the leaf moves on to a copy and the original + # is kept by the edit that records the revision, so it no longer has a leaf of its own. + # Its uploads are still served, so the book has to stay findable through the edit. + def owning_book + book || edit&.leaf&.book + end + def searchable_content nil end diff --git a/lib/rails_ext/action_text_markdown.rb b/lib/rails_ext/action_text_markdown.rb index fa0a6229..afc0b2b5 100644 --- a/lib/rails_ext/action_text_markdown.rb +++ b/lib/rails_ext/action_text_markdown.rb @@ -1,5 +1,12 @@ module ActionText class Markdown < Record + # The signed id that authorizes an upload is rendered into the page editor. Binding + # it to a purpose keeps it from being used anywhere else a signed global id is + # accepted, and expiring it bounds how long a copy taken off the page stays good. + # Neither replaces the authorization check in the uploads controller. + UPLOADS_SIGNED_ID_PURPOSE = :markdown_uploads + UPLOADS_SIGNED_ID_EXPIRY = 1.day + DEFAULT_RENDERER_OPTIONS = { filter_html: false } diff --git a/lib/rails_ext/action_text_tag_helper.rb b/lib/rails_ext/action_text_tag_helper.rb index 1f78d958..0dc40ce5 100644 --- a/lib/rails_ext/action_text_tag_helper.rb +++ b/lib/rails_ext/action_text_tag_helper.rb @@ -6,11 +6,18 @@ def markdown_area(record, name, value: nil, **options) data = options.delete(:data) || {} data.reverse_merge! \ - uploads_url: action_text_markdown_uploads_url(record_gid: record.to_signed_global_id.to_s, attribute_name: name, format: "json") + uploads_url: action_text_markdown_uploads_url(record_gid: uploads_signed_id_for(record), attribute_name: name, format: "json") tag.house_md value, name: field_name, data: data, **options end + def uploads_signed_id_for(record) + record.to_signed_global_id( + expires_in: ActionText::Markdown::UPLOADS_SIGNED_ID_EXPIRY, + for: ActionText::Markdown::UPLOADS_SIGNED_ID_PURPOSE + ).to_s + end + def house_toolbar(**options, &block) tag.house_md_toolbar(**options, &block) end diff --git a/test/controllers/action_text/markdown/uploads_controller_test.rb b/test/controllers/action_text/markdown/uploads_controller_test.rb index 48141c1e..727c298f 100644 --- a/test/controllers/action_text/markdown/uploads_controller_test.rb +++ b/test/controllers/action_text/markdown/uploads_controller_test.rb @@ -8,7 +8,7 @@ class ActionText::Markdown::UploadsControllerTest < ActionDispatch::IntegrationT test "attach a file" do assert_changes -> { ActiveStorage::Attachment.count }, 1 do post action_text_markdown_uploads_url, params: { - record_gid: pages(:welcome).to_signed_global_id.to_s, + record_gid: uploads_signed_id_for(pages(:welcome)), attribute_name: "body", file: fixture_file_upload("reading.webp", "image/webp") }, as: :xhr @@ -20,15 +20,98 @@ class ActionText::Markdown::UploadsControllerTest < ActionDispatch::IntegrationT assert JSON.parse(response.body)["fileUrl"].start_with?("/") end - test "view attached file" do - markdown = pages(:welcome).body.tap(&:save!) - markdown.uploads.attach fixture_file_upload("reading.webp", "image/webp") + test "a signed id minted for some other purpose can't be used to upload" do + assert_no_changes -> { ActiveStorage::Attachment.count } do + post action_text_markdown_uploads_url, params: { + record_gid: pages(:welcome).to_signed_global_id.to_s, + attribute_name: "body", + file: fixture_file_upload("reading.webp", "image/webp") + }, as: :xhr + end + + assert_response :not_found + end + + test "an expired signed id can't be used to upload" do + record_gid = uploads_signed_id_for(pages(:welcome)) + + travel ActionText::Markdown::UPLOADS_SIGNED_ID_EXPIRY + 1.hour do + assert_no_changes -> { ActiveStorage::Attachment.count } do + post action_text_markdown_uploads_url, params: { + record_gid: record_gid, + attribute_name: "body", + file: fixture_file_upload("reading.webp", "image/webp") + }, as: :xhr + end + end + + assert_response :not_found + end + + test "a revoked collaborator can't upload with a signed id minted while an editor" do + record_gid = uploads_signed_id_for(pages(:welcome)) + accesses(:kevin_handbook).destroy! + + assert_no_changes -> { ActiveStorage::Attachment.count } do + post action_text_markdown_uploads_url, params: { + record_gid: record_gid, + attribute_name: "body", + file: fixture_file_upload("reading.webp", "image/webp") + }, as: :xhr + end + + assert_response :not_found + end - attachment = pages(:welcome).body.uploads.last + test "a downgraded editor can't upload" do + record_gid = uploads_signed_id_for(pages(:welcome)) + accesses(:kevin_handbook).update! level: :reader + + assert_no_changes -> { ActiveStorage::Attachment.count } do + post action_text_markdown_uploads_url, params: { + record_gid: record_gid, + attribute_name: "body", + file: fixture_file_upload("reading.webp", "image/webp") + }, as: :xhr + end + + assert_response :forbidden + end + + test "a reader can't upload" do + sign_in :jz + + assert_no_changes -> { ActiveStorage::Attachment.count } do + post action_text_markdown_uploads_url, params: { + record_gid: uploads_signed_id_for(pages(:welcome)), + attribute_name: "body", + file: fixture_file_upload("reading.webp", "image/webp") + }, as: :xhr + end + + assert_response :forbidden + end + + test "view attached file" do + attachment = attach_upload_to_welcome_page get action_text_markdown_upload_url(slug: attachment.slug) assert_response :redirect assert_match /\/rails\/active_storage\/.*\/reading\.webp/, @response.redirect_url end + + private + def uploads_signed_id_for(record) + record.to_signed_global_id( + expires_in: ActionText::Markdown::UPLOADS_SIGNED_ID_EXPIRY, + for: ActionText::Markdown::UPLOADS_SIGNED_ID_PURPOSE + ).to_s + end + + def attach_upload_to_welcome_page + markdown = pages(:welcome).body.tap(&:save!) + markdown.uploads.attach fixture_file_upload("reading.webp", "image/webp") + pages(:welcome).body.uploads.last + end end From 55f816e8a4957d9c10f0b687a7648c8c49c983c1 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 15:03:03 -0700 Subject: [PATCH 3/4] Gate uploaded attachments on access to their book /u/*slug served any attachment to anyone holding the URL, with no session required, and cached the response publicly for a year. Uploads belonging to an unpublished book are as private as the book, so this was a way to read them without an access row, and the public cache directive put them into shared caches on the way. Attachments of published books are served and cached as before. Otherwise the request now needs an access row on the owning book, and the response is marked private. --- .../markdown/uploads_controller.rb | 21 +++++++++- .../markdown/uploads_controller_test.rb | 41 +++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/app/controllers/action_text/markdown/uploads_controller.rb b/app/controllers/action_text/markdown/uploads_controller.rb index 3903ead2..f7d4292a 100644 --- a/app/controllers/action_text/markdown/uploads_controller.rb +++ b/app/controllers/action_text/markdown/uploads_controller.rb @@ -6,6 +6,7 @@ class ActionText::Markdown::UploadsController < ApplicationController end before_action :set_record, :ensure_editable, only: :create + before_action :set_attachment, :ensure_attachment_readable, only: :show def create @markdown = @record.safe_markdown_attribute params[:attribute_name] @@ -18,8 +19,12 @@ def create end def show - @attachment = ActiveStorage::Attachment.find_by! slug: "#{params[:slug]}.#{params[:format]}" - expires_in 1.year, public: true + if @book&.published? + expires_in 1.year, public: true + else + expires_in 5.minutes, public: false + end + redirect_to @attachment.url end @@ -38,4 +43,16 @@ def set_record def ensure_editable head :forbidden unless @book.editable? end + + def set_attachment + @attachment = ActiveStorage::Attachment.find_by! slug: "#{params[:slug]}.#{params[:format]}" + @book = @attachment.record.try(:record).try(:owning_book) + end + + # 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. + def ensure_attachment_readable + head :not_found unless @book.nil? || @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 727c298f..93472f32 100644 --- a/test/controllers/action_text/markdown/uploads_controller_test.rb +++ b/test/controllers/action_text/markdown/uploads_controller_test.rb @@ -93,6 +93,7 @@ class ActionText::Markdown::UploadsControllerTest < ActionDispatch::IntegrationT end test "view attached file" do + books(:handbook).update! published: true attachment = attach_upload_to_welcome_page get action_text_markdown_upload_url(slug: attachment.slug) @@ -101,6 +102,46 @@ class ActionText::Markdown::UploadsControllerTest < ActionDispatch::IntegrationT assert_match /\/rails\/active_storage\/.*\/reading\.webp/, @response.redirect_url end + test "an attachment of a published book is publicly cacheable" do + books(:handbook).update! published: true + attachment = attach_upload_to_welcome_page + + get action_text_markdown_upload_url(slug: attachment.slug) + + assert_match "public", @response.headers["Cache-Control"] + end + + test "an attachment of an unpublished book is not served to anonymous clients" do + books(:handbook).update! published: false + attachment = attach_upload_to_welcome_page + + reset! + get action_text_markdown_upload_url(slug: attachment.slug) + + assert_response :not_found + end + + test "an attachment of an unpublished book is not served to a user without access" do + books(:handbook).update! published: false + attachment = attach_upload_to_welcome_page + + accesses(:kevin_handbook).destroy! + 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 + + sign_in :jz + get action_text_markdown_upload_url(slug: attachment.slug) + + assert_response :redirect + assert_no_match "public", @response.headers["Cache-Control"].to_s + end + private def uploads_signed_id_for(record) record.to_signed_global_id( From b4e74b3f53ce85807a1710c6a90f42b0fd878630 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Mon, 3 Aug 2026 15:15:44 -0700 Subject: [PATCH 4/4] Restrict revision history to editors The only link to this screen is in the page editor, which is editor-gated, so the interface has always expressed this restriction and the controller simply never enforced it. No reader reaches revision history through the UI; it takes a hand-constructed URL. Revision history shows text an editor removed from a page. That is content a reader can't otherwise see even when the page itself is still readable, so 'a reader can already read the page' doesn't fully cover it. Git history shows no decision to allow reader access here: the Current.user.leaves lookup arrived in a URL-stability refactor, and the one permissions commit around it was about who may write. --- app/controllers/pages/edits_controller.rb | 9 +++++++++ test/controllers/pages/edits_controller_test.rb | 13 +++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/app/controllers/pages/edits_controller.rb b/app/controllers/pages/edits_controller.rb index da3596fc..7a67d5ce 100644 --- a/app/controllers/pages/edits_controller.rb +++ b/app/controllers/pages/edits_controller.rb @@ -1,12 +1,21 @@ class Pages::EditsController < ApplicationController include PageLeafScoped + before_action :ensure_editable before_action :set_edit def show end private + # The only link to this screen lives in the page editor, which is already editor-gated, + # so the interface has always expressed this restriction and the controller simply + # didn't enforce it. Revision history shows text an editor removed from a page, which + # a reader can't otherwise see even when the page itself is still readable. + def ensure_editable + head :forbidden unless @leaf.book.editable? + end + def set_edit if params[:id] == "latest" @edit = @leaf.edits.last diff --git a/test/controllers/pages/edits_controller_test.rb b/test/controllers/pages/edits_controller_test.rb index acd35646..d9776be6 100644 --- a/test/controllers/pages/edits_controller_test.rb +++ b/test/controllers/pages/edits_controller_test.rb @@ -56,15 +56,20 @@ class Pages::EditsControllerTest < ActionDispatch::IntegrationTest assert_response :not_found end - # Readers may still read the history of a live page. That is long-standing behavior - # and was assessed on its own terms: a reader can already read the page itself. The - # trashed case above is different, because that content is unreachable otherwise. - test "a reader may still read a live page's history" do + test "a reader can't read a live page's history" do leaves(:welcome_page).edit leafable_params: { body: "Updated" } sign_in :jz get page_edit_url(leaves(:welcome_page), "latest") + assert_response :forbidden + end + + test "an editor can still read a live page's history" do + leaves(:welcome_page).edit leafable_params: { body: "Updated" } + + get page_edit_url(leaves(:welcome_page), "latest") + assert_response :success end