Merge pull request #463 from basecamp/security/revision-and-upload

Authorize revision history and markdown uploads against current book access

Author: jeremy

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

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

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

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

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
     }

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

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

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: %(<img src=x onerror="alert(1)">) }