From 196d685f8d5c8d2604709867c09a7d3a5ad20ed9 Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Fri, 19 Dec 2025 11:13:06 +0100 Subject: [PATCH 1/2] Implement authorization for Active Storage endpoints Consider blobs attached to any public records accessible to anyone with the URL. --- app/models/account/export.rb | 4 + app/models/board/publishable.rb | 1 + app/models/card.rb | 4 + app/models/comment.rb | 2 +- config/initializers/action_text.rb | 8 + lib/rails_ext/active_storage_authorization.rb | 49 +++++ .../active_storage_authorization_test.rb | 197 ++++++++++++++++++ 7 files changed, 264 insertions(+), 1 deletion(-) create mode 100644 lib/rails_ext/active_storage_authorization.rb create mode 100644 test/integration/active_storage_authorization_test.rb diff --git a/app/models/account/export.rb b/app/models/account/export.rb index c5e3d64c3..1fd2689d2 100644 --- a/app/models/account/export.rb +++ b/app/models/account/export.rb @@ -37,6 +37,10 @@ class Account::Export < ApplicationRecord update!(status: :completed, completed_at: Time.current) end + def accessible_to?(accessor) + accessor == user + end + private def generate_zip Tempfile.new([ "export", ".zip" ]).tap do |tempfile| diff --git a/app/models/board/publishable.rb b/app/models/board/publishable.rb index 6c1ce47d4..8b113cc9e 100644 --- a/app/models/board/publishable.rb +++ b/app/models/board/publishable.rb @@ -15,6 +15,7 @@ module Board::Publishable def published? publication.present? end + alias_method :publicly_accessible?, :published? def publish create_publication! unless published? diff --git a/app/models/card.rb b/app/models/card.rb index dbb45f184..18957cb9b 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -48,6 +48,10 @@ class Card < ApplicationRecord delegate :accessible_to?, to: :board + def publicly_accessible? + published? && board.publicly_accessible? + end + def card self end diff --git a/app/models/comment.rb b/app/models/comment.rb index d0315eea8..30706e8b5 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -15,7 +15,7 @@ class Comment < ApplicationRecord after_create_commit :watch_card_by_creator - delegate :board, :watch_by, to: :card + delegate :publicly_accessible?, :accessible_to?, :board, :watch_by, to: :card def to_partial_path "cards/#{super}" diff --git a/config/initializers/action_text.rb b/config/initializers/action_text.rb index 0f804e57b..6aed0616d 100644 --- a/config/initializers/action_text.rb +++ b/config/initializers/action_text.rb @@ -16,6 +16,14 @@ module ActionText def storage_tracked_record record.try(:storage_tracked_record) end + + def accessible_to?(user) + record.try(:accessible_to?, user) || record.try(:publicly_accessible?) + end + + def publicly_accessible? + record.try(:publicly_accessible?) + end end end end diff --git a/lib/rails_ext/active_storage_authorization.rb b/lib/rails_ext/active_storage_authorization.rb new file mode 100644 index 000000000..55d0de3bc --- /dev/null +++ b/lib/rails_ext/active_storage_authorization.rb @@ -0,0 +1,49 @@ +ActiveSupport.on_load :active_storage_blob do + def accessible_to?(user) + attachments.includes(:record).any? { |attachment| attachment.accessible_to?(user) } || attachments.none? + end + + def publicly_accessible? + attachments.includes(:record).any? { |attachment| attachment.publicly_accessible? } + end +end + +ActiveSupport.on_load :active_storage_attachment do + def accessible_to?(user) + record.try(:accessible_to?, user) + end + + def publicly_accessible? + record.try(:publicly_accessible?) + end +end + +Rails.application.config.to_prepare do + module ActiveStorage::Authorize + extend ActiveSupport::Concern + + include Authentication + + included do + # Ensure require_authentication runs after set_blob. + skip_before_action :require_authentication + before_action :require_authentication, :ensure_accessible, unless: :publicly_accessible_blob? + end + + private + def publicly_accessible_blob? + @blob.publicly_accessible? + end + + def ensure_accessible + unless @blob.accessible_to?(Current.user) + head :forbidden + end + end + end + + ActiveStorage::Blobs::RedirectController.include ActiveStorage::Authorize + ActiveStorage::Blobs::ProxyController.include ActiveStorage::Authorize + ActiveStorage::Representations::RedirectController.include ActiveStorage::Authorize + ActiveStorage::Representations::ProxyController.include ActiveStorage::Authorize +end diff --git a/test/integration/active_storage_authorization_test.rb b/test/integration/active_storage_authorization_test.rb new file mode 100644 index 000000000..85919ea13 --- /dev/null +++ b/test/integration/active_storage_authorization_test.rb @@ -0,0 +1,197 @@ +require "test_helper" + +class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest + setup do + Current.session = sessions(:david) + @account = accounts("37s") + @board = boards(:writebook) + @card = cards(:logo) + @blob = attach_blob_to_card(@card) + end + + test "authenticated user with board access can view blob" do + sign_in_as :david + + get rails_blob_path(@blob, disposition: :inline) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + + test "authenticated user without board access cannot view blob" do + sign_in_as :mike + + get rails_blob_path(@blob, disposition: :inline) + assert_response :forbidden + end + + test "unauthenticated user cannot view blob" do + get rails_blob_path(@blob, disposition: :inline) + assert_response :redirect + assert_match %r{/session/new}, response.location + end + + test "authenticated user with board access can view representation" do + sign_in_as :david + + get rails_representation_path(@blob.representation(resize_to_limit: [ 100, 100 ])) + assert_response :redirect + assert_match %r{rails/active_storage/}, response.location + end + + test "authenticated user without board access cannot view representation" do + sign_in_as :mike + + get rails_representation_path(@blob.representation(resize_to_limit: [ 100, 100 ])) + assert_response :forbidden + end + + test "unauthenticated user can view blob on published board with published card" do + @board.publish + + get rails_blob_path(@blob, disposition: :inline) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + + test "unauthenticated user cannot view blob on published board with draft card" do + @board.publish + + # Create the draft card and attachment with proper Current context + draft_blob = nil + Current.with(account: @account, session: sessions(:david)) do + draft_card = @board.cards.create!(title: "Draft", status: :drafted, creator: users(:david)) + draft_card.image.attach io: file_fixture("moon.jpg").open, filename: "draft.jpg", content_type: "image/jpeg" + draft_blob = draft_card.image.blob + end + + get rails_blob_path(draft_blob, disposition: :inline) + assert_response :redirect + assert_match %r{/session/new}, response.location + end + + # Rich text embeds in cards + + test "authenticated user with board access can view rich text embed in card" do + sign_in_as :david + + blob = attach_blob_as_rich_text_embed(@card) + + get rails_blob_path(blob, disposition: :inline) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + + test "authenticated user without board access cannot view rich text embed in card" do + sign_in_as :mike + + blob = attach_blob_as_rich_text_embed(@card) + + get rails_blob_path(blob, disposition: :inline) + assert_response :forbidden + end + + test "unauthenticated user can view rich text embed in card on published board" do + @board.publish + + blob = attach_blob_as_rich_text_embed(@card) + + get rails_blob_path(blob, disposition: :inline) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + + # Rich text embeds in comments + + test "authenticated user with board access can view rich text embed in comment" do + sign_in_as :david + + comment = comments(:logo_1) + blob = attach_blob_as_rich_text_embed(comment) + + get rails_blob_path(blob, disposition: :inline) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + + test "authenticated user without board access cannot view rich text embed in comment" do + sign_in_as :mike + + comment = comments(:logo_1) + blob = attach_blob_as_rich_text_embed(comment) + + get rails_blob_path(blob, disposition: :inline) + assert_response :forbidden + end + + test "unauthenticated user can view rich text embed in comment on published board" do + @board.publish + + comment = comments(:logo_1) + blob = attach_blob_as_rich_text_embed(comment) + + get rails_blob_path(blob, disposition: :inline) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + + # Account exports + + test "export owner can download their export" do + sign_in_as :david + + blob = create_export_blob_for(users(:david)) + + get rails_blob_path(blob, disposition: :attachment) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + + test "non-owner cannot download another user's export" do + sign_in_as :jz + + blob = create_export_blob_for(users(:david)) + + get rails_blob_path(blob, disposition: :attachment) + assert_response :forbidden + end + + test "unauthenticated user cannot download export" do + blob = create_export_blob_for(users(:david)) + + get rails_blob_path(blob, disposition: :attachment) + assert_response :redirect + assert_match %r{/session/new}, response.location + end + + private + def attach_blob_to_card(card) + Current.with(session: sessions(:david)) do + card.image.attach io: file_fixture("moon.jpg").open, filename: "test.jpg", content_type: "image/jpeg" + card.image.blob + end + end + + def attach_blob_as_rich_text_embed(container) + Current.with(account: @account, session: sessions(:david)) do + blob = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "embed.jpg", + content_type: "image/jpeg" + + attachment_html = ActionText::Attachment.from_attachable(blob).to_html + if container.respond_to?(:description) + container.update!(description: "

Description with image: #{attachment_html}

") + else + container.update!(body: "

Body with image: #{attachment_html}

") + end + + blob.reload + end + end + + def create_export_blob_for(user) + export = Account::Export.create!(account: @account, user: user) + export.file.attach io: StringIO.new("test export content"), filename: "export.zip", content_type: "application/zip" + export.file.blob + end +end From f66a2dc1ba9be31c311a6081c3e389740c3408b7 Mon Sep 17 00:00:00 2001 From: Rosa Gutierrez Date: Fri, 26 Dec 2025 19:33:26 +0100 Subject: [PATCH 2/2] Consider user avatars always public Avatars are purposely accessible without authentication (5e3b5b6d7cfe95c20d856aa3384e7a07f44998d2) because they can be in public collections. Trying to restrict this by checking whether they're in fact present in some public collection is rather expensive, so let's keep them public. --- app/models/user/avatar.rb | 5 ++++ .../active_storage_authorization_test.rb | 23 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/app/models/user/avatar.rb b/app/models/user/avatar.rb index 400738cb1..c62d87e09 100644 --- a/app/models/user/avatar.rb +++ b/app/models/user/avatar.rb @@ -22,6 +22,11 @@ module User::Avatar avatar.variable? ? avatar.variant(:thumb) : avatar end + # Avatars are always publicly accessible + def publicly_accessible? + true + end + private def avatar_content_type_allowed if !ALLOWED_AVATAR_CONTENT_TYPES.include?(avatar.content_type) diff --git a/test/integration/active_storage_authorization_test.rb b/test/integration/active_storage_authorization_test.rb index 85919ea13..1ebb35feb 100644 --- a/test/integration/active_storage_authorization_test.rb +++ b/test/integration/active_storage_authorization_test.rb @@ -134,6 +134,22 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest assert_match %r{rails/active_storage}, response.location end + test "unauthenticated user can view avatar" do + blob = attach_avatar_to(users(:david)) + + get rails_blob_path(blob, disposition: :inline) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + + test "unauthenticated user can view avatar thumbnail" do + blob = attach_avatar_to(users(:david)) + + get rails_representation_path(blob.representation(resize_to_fill: [ 256, 256 ])) + assert_response :redirect + assert_match %r{rails/active_storage}, response.location + end + # Account exports test "export owner can download their export" do @@ -194,4 +210,11 @@ class ActiveStorageAuthorizationTest < ActionDispatch::IntegrationTest export.file.attach io: StringIO.new("test export content"), filename: "export.zip", content_type: "application/zip" export.file.blob end + + def attach_avatar_to(user) + Current.with(account: @account, session: sessions(:david)) do + user.avatar.attach io: file_fixture("moon.jpg").open, filename: "avatar.jpg", content_type: "image/jpeg" + user.avatar.blob + end + end end