Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup active storage defaults #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

hasarindaKI
Copy link
Contributor

@hasarindaKI hasarindaKI commented Jun 5, 2023

  1. Installs active_storage migrations
  2. Disables direct upload routes
unless Rails.env.development? || Rails.env.test?
  put "/rails/active_storage/disk/:encoded_token", to: redirect("/404")
end
post "/rails/active_storage/direct_uploads", to: redirect("/404")
  1. Configures service for active_storage in environments
Rails.application.configure do
  config.active_storage.service = :s3
end
  1. Adds aws-sdk-s3 gem for AWS S3
  2. Adds image_processing for variant support
  3. Configures AWS S3 with defaults
s3:
  service: S3
  region: ap-southeast-2
  access_key_id: <%= ENV["RAILS_ASSETS_ACCESS_KEY"] %>
  secret_access_key: <%= ENV["RAILS_ASSETS_SECRET_KEY"] %>
  bucket: <%= ENV.fetch("RAILS_ASSETS_BUCKET_ID", "katalyst-[APP_NAME]-staging-assets") %>
  1. Adds host to default_url_options
development: "https://localhost",
staging:     "https://#{@app_name}-staging.katalyst.com.au",
production:  "https://#{@app_name}-production.katalyst.com.au",
test:        "https://example.com",

Copy link
Contributor

@AlanCornthwaiteKatalyst AlanCornthwaiteKatalyst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not to familiar with the active storage config so i might lean on @sfnelson for that side, but i do have a comment on adding the gem from the template

template.rb Outdated Show resolved Hide resolved
@hasarindaKI hasarindaKI force-pushed the setup/active-storage branch 2 times, most recently from eba61f3 to f4a3921 Compare June 6, 2023 05:32
@hasarindaKI hasarindaKI force-pushed the setup/active-storage branch from f4a3921 to d6fcf05 Compare June 6, 2023 05:38
Copy link
Contributor

@sfnelson sfnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hasarindaKI I'd like to review this with you in person

test: "https://example.com",
}.freeze

Rails.application.routes.default_url_options = { host: hosts[Rails.env.to_sym] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a good long-term strategy. It will require manual changes for non-Katalyst projects.

I think that the staging and production environments should be getting this from their environment (ecs env). Please add a related PR to make that happen if it's not there already.

I think that the default_url_options setting should be in the environment files rather than here.

run("rails active_storage:install")

# setup active_storage services
copy_file("config/storage.yml")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be template

Copy link
Contributor

@sfnelson sfnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this PR is mergeable but we need to have a strategy for new apps. Leaving comments to document next steps.


# setup active_storage services
copy_file("config/storage.yml")
gsub_file("config/storage.yml", "[DEFAULT_BUCKET_ID]", "katalyst-#{@app_name}-staging-assets")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a strategy for connecting this app to the terraform configured bucket name. Could be manual.

@@ -4,4 +4,9 @@
resource :homepage, only: %i[show]

root "homepages#show"

unless Rails.env.development? || Rails.env.test?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local? in Rails 7

unless Rails.env.development? || Rails.env.test?
put "/rails/active_storage/disk/:encoded_token", to: redirect("/404")
end
post "/rails/active_storage/direct_uploads", to: redirect("/404")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should result in a routing error, not a redirect. That way the default rules apply.

Ideally we want to undefined the routes that ActiveStorage engine adds, but I don't know if that's possible. This is a hack approach that needs more thought imo.

Also needs documentation, as this is appropriate for Fringe and Koi but not generalisable to all situations (e.g. what if you have authenticated users and you want to allow them to upload files?

Finally, is this a realistic concern? Have the Rails team discussed why it's acceptable to allow arbitrary users to write to this endpoint? Is there any sort of nonce or CSFR protection that we're unaware of protecting these endpoints for DOS?

Copy link
Contributor

@sobakasu sobakasu Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an open rails issue about this:
rails/rails#34961

A blog post with suggested workaround:
https://givenis.me/securing-rails-active-storage-direct-uploads

This blog post uses authenticate_user, but we could instead cause a 404 by raising a routing error in a before action?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another related open rails issue:
rails/rails#52505

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • There is no way to remove a specific route; but you can remove all active storage routes by setting the draw_routes config option to false or you could overwrite the default active storage route in your routes file
  • There is CSRF protection in DirectUploads but it is available on all the pages as a meta content

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants