diff --git a/app/controllers/action_text/markdown/uploads_controller.rb b/app/controllers/action_text/markdown/uploads_controller.rb index 52b55507..f7d4292a 100644 --- a/app/controllers/action_text/markdown/uploads_controller.rb +++ b/app/controllers/action_text/markdown/uploads_controller.rb @@ -5,9 +5,10 @@ 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 + before_action :set_attachment, :ensure_attachment_readable, only: :show + def create @markdown = @record.safe_markdown_attribute params[:attribute_name] @markdown.uploads.attach [ params[:file] ] @markdown.save! @@ -18,8 +19,40 @@ 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 + + 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 + + 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/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/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/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..93472f32 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,139 @@ 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 + + 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 - attachment = pages(:welcome).body.uploads.last + 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 + books(:handbook).update! published: true + 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 + + 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( + 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 diff --git a/test/controllers/pages/edits_controller_test.rb b/test/controllers/pages/edits_controller_test.rb index d490be63..d9776be6 100644 --- a/test/controllers/pages/edits_controller_test.rb +++ b/test/controllers/pages/edits_controller_test.rb @@ -35,6 +35,55 @@ 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 + + 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 + + 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: %() }