From b601b2bbb6ae29147405a4b82dd69b3e83a16a75 Mon Sep 17 00:00:00 2001 From: Christoph Haas Date: Mon, 16 Jun 2025 00:24:11 +0200 Subject: [PATCH] move shrine files out of public. use x-sendfile to send files for security. --- .dockerignore | 1 + .gitignore | 12 +-- Procfile.dev | 2 +- app/controllers/packages_controller.rb | 141 ++++++++++++------------- app/helpers/packages_helper.rb | 12 ++- app/models/ability.rb | 43 ++++---- app/views/packages/details.slim | 2 +- config/environments/development.rb | 9 ++ config/environments/production.rb | 9 ++ config/initializers/shrine.rb | 14 +-- config/routes.rb | 59 +++++++---- 11 files changed, 164 insertions(+), 140 deletions(-) diff --git a/.dockerignore b/.dockerignore index c05f171..706873c 100644 --- a/.dockerignore +++ b/.dockerignore @@ -54,6 +54,7 @@ # Ignore test screenshots /public/shrine /public/cache +/shrine /doc /test diff --git a/.gitignore b/.gitignore index 644767f..ca7e523 100644 --- a/.gitignore +++ b/.gitignore @@ -17,16 +17,8 @@ !/tmp/.keep /public/assets/ /public/system/ -# old Paperclip path -/public/screenshots/ -# old Shrine path -/public/screenshot/ -# new Shrine path -/public/shrine/ -/public/cache/screenshot/ -/public/live/ -/public/packs-test/ -/public/packs/ +/public/cache/ +/shrine/ # Ignore pidfiles, but keep the directory. /tmp/pids/* diff --git a/Procfile.dev b/Procfile.dev index 7eaba23..faea657 100644 --- a/Procfile.dev +++ b/Procfile.dev @@ -1,2 +1,2 @@ -web: env RUBY_DEBUG_OPEN=true bin/rails server +web: env RUBY_DEBUG_OPEN=true HTTP_PORT=3001 bin/thrust bin/rails server css: bun run build:css --watch diff --git a/app/controllers/packages_controller.rb b/app/controllers/packages_controller.rb index c1a19ae..780b074 100644 --- a/app/controllers/packages_controller.rb +++ b/app/controllers/packages_controller.rb @@ -26,7 +26,10 @@ class PackagesController < ApplicationController render 'notfound', status: 404 else @page = params[:page] - @screenshots = screenshots_visible_to_user(@package).paginate(page: @page, per_page: 6) + # @screenshots = screenshots_visible_to_user(@package).paginate(page: @page, per_page: 6) + @screenshots = @package.screenshots.accessible_by(current_ability, :view).paginate( + page: @page, per_page: 6 + ) end end @@ -231,78 +234,64 @@ class PackagesController < ApplicationController def approve_screenshot @screenshot = Screenshot.find(params[:id]) - if can? :approve, @screenshot - @screenshot.approve! - auditlog 'Screenshot approved', - package: @screenshot.package, screenshot: @screenshot + return unless can? :approve, @screenshot - # Increase the approval counter for the user (social scoring) - @screenshot.user.approved_screenshots += 1 - @screenshot.user.save! + @screenshot.approve! + auditlog 'Screenshot approved', + package: @screenshot.package, screenshot: @screenshot - flash['notice'] = 'Screenshot approved.' - redirect_back(fallback_location: package_path(name: @screenshot.package.name)) - else - head :forbidden - end + # Increase the approval counter for the user (social scoring) + @screenshot.user.approved_screenshots += 1 + @screenshot.user.save! + + flash['notice'] = 'Screenshot approved.' + redirect_back(fallback_location: package_path(name: @screenshot.package.name)) + elsename + head :forbidden end - # Returns a 160x120 thumbnail image if posssible. + # Returns either… + # - 160x120 thumbnail image + # - 320x240 small image + # - full-size screenshot + # # If the package is not found it returns a dummy image along with status 404. # If the package is found but has no screenshots then it also returns a # dummy image along with status 404. - def thumbnail - @package = Package.find_by(name: params[:name]) + # + # Return a specific screenshot (if the screenshot_id parameter is given) + # or just the first (newest) one. + def send_image + size = params[:size] # :small, :large or :thumb + @package = Package.find_by(name: params[:name]) # package name + @screenshot_id = params[:screenshot_id] # screenshot ID (optional) unless @package - logger.debug "Cannot render thumbnail for packages #{params[:name]} – no such package" - thumbnail404 - return - end - - # Called as /thumbnail-with-version/:name/:version - @screenshot = if params[:version] - @package.best_screenshot_for_version(params[:version]) - # Called as /thumbnail/:name - else - @package.screenshots.approved.first - end - - # Return a 404 if the package has no screenshots or the image was not found - unless @screenshot - logger.debug "Cannot render thumbnail for packages #{params[:name]} – no screenshot found" - thumbnail404 - return - end - - redirect_to @screenshot.simage_url(:thumb) - end - - # Returns a large screenshot image if posssible. - # If the package is not found it returns a dummy image along with status 404. - # If the package is found but has no screenshots then it also returns a - # dummy image along with status 404. - def screenshot - @package = Package.find_by(name: params[:name]) - unless @package - screenshot404 + Rails.logger.debug 'no such package -> 404' + screenshot404 if %i[large small].include?(size) + thumbnail404 if size == :thumb return end # Called as /screenshot-with-version/:name/:version - @screenshot = if params[:version] - @package.best_screenshot_for_version(params[:version]) - # Called as /screenshot/:name - else - @package.screenshots.approved.first - end + # or /thumbnail-with-version/:name/:version + @image = if params[:version] + @package.best_screenshot_for_version(params[:version]) + # Called as /screenshot/:name + else + # @package.screenshots.approved.first + @package.screenshots.accessible_by(current_ability, :view).first + end # Return a 404 if the package has no screenshots or the image was not found - unless @screenshot - screenshot404 + unless @image + Rails.logger.debug 'no such image -> 404' + screenshot404 if %i[large small].include?(size) + thumbnail404 if size == :thumb return end - redirect_to @screenshot.simage_url(:large) + send_file(File.join(@image.simage.storage.directory, @image.simage(size).id), + disposition: 'inline') end # Receives a form with a simple text field 'description' so that users can update @@ -341,7 +330,7 @@ class PackagesController < ApplicationController # Return packages matching the criteria given by parameters def query_packages - packages = Package #.order(visits: :desc) + packages = Package # .order(visits: :desc) # text search if params[:search].present? @@ -370,26 +359,26 @@ class PackagesController < ApplicationController # end # Get reviews of this package from the Ubuntu API - def get_ubuntu_reviews(packagename) - # Use the URL defined in the configuration to get a JSON string - url = Rails.configuration.ubuntu_reviews_api_url % packagename - logger.debug "Loading Ubuntu reviews for package #{packagename} from #{url}" + # def get_ubuntu_reviews(packagename) + # # Use the URL defined in the configuration to get a JSON string + # url = Rails.configuration.ubuntu_reviews_api_url % packagename + # logger.debug "Loading Ubuntu reviews for package #{packagename} from #{url}" - body = open(url).read - # Turn JSON into a Ruby data structure - json = JSON.parse(body) - # Only show english reviews - # TODO: Support further languages - json = json.select { |x| x['language'] == 'en' } - # Sort by 'usefulness_total' (how many people found this review useful) - json.sort { |x, y| y['usefulness_total'].to_i <=> x['usefulness_total'].to_i } - end + # body = open(url).read + # # Turn JSON into a Ruby data structure + # json = JSON.parse(body) + # # Only show english reviews + # # TODO: Support further languages + # json = json.select { |x| x['language'] == 'en' } + # # Sort by 'usefulness_total' (how many people found this review useful) + # json.sort { |x, y| y['usefulness_total'].to_i <=> x['usefulness_total'].to_i } + # end - def params_screenshot_description - params.require(:screenshot).permit(:description) - end + # def params_screenshot_description + # params.require(:screenshot).permit(:description) + # end - def screenshots_visible_to_user(package) - package.screenshots.accessible_by(current_ability, :view).order('created_at DESC') - end + # def screenshots_visible_to_user(package) + # package.screenshots.accessible_by(current_ability, :view).order('created_at DESC') + # end end diff --git a/app/helpers/packages_helper.rb b/app/helpers/packages_helper.rb index 50a235c..d7dec2c 100644 --- a/app/helpers/packages_helper.rb +++ b/app/helpers/packages_helper.rb @@ -11,10 +11,12 @@ module PackagesHelper end def small_img(screenshot, cls: 'thumbnail') - if screenshot && screenshot.simage(:small) + if screenshot&.simage(:small) image = screenshot.simage(:small) + # image = screenshot_image_path(name: screenshot.package.name) + image_tag( - image.url, + small_image_with_id_path(name: screenshot.package.name, screenshot_id: screenshot.id), width: image.width, height: image.height, alt: screenshot.caption, @@ -40,11 +42,11 @@ module PackagesHelper # Return a readable status of a screenshot and a matching CSS color class def status(screenshot) if screenshot.approved && !screenshot.hidden - [(icon('eye', class: 'icon') + ' Public'), 'public'] + ["#{icon('eye', class: 'icon')} Public".html_safe, 'public'] elsif screenshot.hidden - [(icon('eye-closed', class: 'icon') + ' Hidden'), 'hidden'] + ["#{icon('eye-closed', class: 'icon')} Hidden".html_safe, 'hidden'] elsif !screenshot.approved - [(icon('hourglass', class: 'icon') + ' Unapproved'), 'unapproved'] + ["#{icon('hourglass', class: 'icon')} Unapproved".html_safe, 'unapproved'] end end end diff --git a/app/models/ability.rb b/app/models/ability.rb index 1231449..160a507 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -4,7 +4,6 @@ class Ability include CanCan::Ability def initialize(user) - # Define abilities for the passed in user here. For example: # # user ||= User.new # guest user (not logged in) @@ -33,29 +32,29 @@ class Ability # https://github.com/CanCanCommunity/cancancan/wiki/Defining-Abilities # Everybody can see public screenshots - can :view, Screenshot, approved: true, hidden:false + can :view, Screenshot, approved: true, hidden: false - if user.present? # Logged-in users - # Allow to view own uploads (even not-yet-approved) - can :view, Screenshot, user_id: user.id - can :destroy, Screenshot, user_id: user.id + return unless user.present? # Logged-in users - if user.admin_role? - can :approve, Screenshot - can :destroy, Screenshot - can :hide, Screenshot - can :unhide, Screenshot - can :destroy, User - can :view, Screenshot - can :destroy, Package - can :view, Log - end - if user.moderator_role? - can :approve, Screenshot - can :hide, Screenshot - can :unhide, Screenshot - can :view, Screenshot - end + # Allow to view own uploads (even not-yet-approved) + can :view, Screenshot, user_id: user.id + can :destroy, Screenshot, user_id: user.id + + if user.admin_role? + can :approve, Screenshot + can :destroy, Screenshot + can :hide, Screenshot + can :unhide, Screenshot + can :destroy, User + can :view, Screenshot + can :destroy, Package + can :view, Log end + return unless user.moderator_role? + + can :approve, Screenshot + can :hide, Screenshot + can :unhide, Screenshot + can :view, Screenshot end end diff --git a/app/views/packages/details.slim b/app/views/packages/details.slim index fea46ad..03c9609 100644 --- a/app/views/packages/details.slim +++ b/app/views/packages/details.slim @@ -24,7 +24,7 @@ = render(partial: 'admin_buttons', locals: {screenshot: screenshot}) / Photoswipe needs to know the dimensions of the full-size image to zoom to a.foobar.black [ - href=url_for(screenshot.simage_url(:large)) + href=screenshot_image_with_id_path(screenshot.package, screenshot.id) data-pswp-width=screenshot.simage.width data-pswp-height=screenshot.simage.height ] diff --git a/config/environments/development.rb b/config/environments/development.rb index ed4855d..a3f7d01 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -74,6 +74,15 @@ Rails.application.configure do # routes, locales, etc. This feature depends on the listen gem. config.file_watcher = ActiveSupport::EventedFileUpdateChecker + # Let the "thrust" web server handle sending images. It allows to route all + # images through the Rails application and prevent unauthorized access to + # images that are pending moderation. (A common problem with spammers uploading + # smartphone screenshots of card games.) + config.action_dispatch.x_sendfile_header = 'X-Sendfile' + + # Where Shrine stores the screenshots + config.shrine_storage_path = ENV.fetch('SHRINE_STORAGE_PATH', 'shrine') + config.package_sources = [ { description: 'Debian Unstable (Sid)', type: 'apt', url: 'http://ftp.de.debian.org/debian/dists/sid', diff --git a/config/environments/production.rb b/config/environments/production.rb index 5a1bdec..050081e 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -85,6 +85,15 @@ Rails.application.configure do # Do not dump schema after migrations. config.active_record.dump_schema_after_migration = false + # Let the "thrust" web server handle sending images. It allows to route all + # images through the Rails application and prevent unauthorized access to + # images that are pending moderation. (A common problem with spammers uploading + # smartphone screenshots of card games.) + config.action_dispatch.x_sendfile_header = 'X-Sendfile' + + # Where Shrine stores the screenshots + config.shrine_storage_path = ENV.fetch('SHRINE_STORAGE_PATH', 'shrine') + config.package_sources = [ { description: 'Debian Unstable (Sid)', type: 'apt', url: 'http://ftp.de.debian.org/debian/dists/sid', diff --git a/config/initializers/shrine.rb b/config/initializers/shrine.rb index da0a179..82d4de0 100644 --- a/config/initializers/shrine.rb +++ b/config/initializers/shrine.rb @@ -1,13 +1,15 @@ # See also: https://github.com/erikdahlstrand/shrine-rails-example/blob/master/config/initializers/shrine.rb -require "shrine" -require "shrine/storage/file_system" +require 'shrine' +require 'shrine/storage/file_system' # both `cache` and `store` storages are needed Shrine.storages = { - # Saves to ./public/screenshot/ID/image/… - cache: Shrine::Storage::FileSystem.new("public", prefix: "cache"), - store: Shrine::Storage::FileSystem.new("public", prefix: "shrine"), + # Saves to ./shrine/files/screenshot/ID/image/… + cache: Shrine::Storage::FileSystem.new(Rails.application.config.shrine_storage_path, + prefix: 'cache'), + store: Shrine::Storage::FileSystem.new(Rails.application.config.shrine_storage_path, + prefix: 'files') } # See plugin documentation at https://shrinerb.com/docs/plugins/activerecord @@ -18,6 +20,6 @@ Shrine.plugin :instrumentation Shrine.plugin :determine_mime_type, analyzer: :marcel, log_subscriber: nil Shrine.plugin :cached_attachment_data Shrine.plugin :restore_cached_data -Shrine.plugin :derivatives # up front processing +Shrine.plugin :derivatives # up front processing # Shrine.plugin :derivation_endpoint, # on-the-fly processing # secret_key: Rails.application.secret_key_base diff --git a/config/routes.rb b/config/routes.rb index 78b5b99..55ada0b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,7 +3,7 @@ Rails.application.routes.draw do # Reveal health status on /up that returns 200 if the app boots with no exceptions, otherwise 500. # Can be used by load balancers and uptime monitors to verify that the app is live. - get "up" => "rails/health#show", as: :rails_health_check + get 'up' => 'rails/health#show', as: :rails_health_check Healthcheck.routes(self) get 'admin/status' @@ -19,8 +19,8 @@ Rails.application.routes.draw do devise_for :users, controllers: { # registrations: "users/registrations", # passwords: "users/passwords", - omniauth_callbacks: "users/omniauth_callbacks", - sessions: "users/my_sessions" + omniauth_callbacks: 'users/omniauth_callbacks', + sessions: 'users/my_sessions' } get 'packages' => 'packages#grid', as: :packages_grid @@ -34,29 +34,49 @@ Rails.application.routes.draw do get 'my/screenshots' get 'my/moderate_list', as: :moderate_list get 'my/logs', as: :logs - get 'package/:name' => 'packages#details', as: :package, name: /[^\/]+/ + get 'package/:name' => 'packages#details', as: :package, name: %r{[^/]+} # get 'package_reviews/:name' => 'packages#reviews', as: :package_reviews, name: /[^\/]+/ get 'upload', to: redirect('/packages'), as: :upload_legacy # legacy upload form - post 'uploadfile' => 'packages#legacy_uploadfile', name: /[^\/]+/ - get 'upload/:name' => 'packages#upload', as: :upload, name: /[^\/]+/ - get 'upload/:name/json' => 'packages#upload', as: :upload_json, name: /[^\/]+/, returns: :json - post 'upload/:name' => 'packages#upload_receive', as: :upload_receive, name: /[^\/]+/ - post 'upload/:name/json' => 'packages#upload_receive', as: :upload_receive_json, name: /[^\/]+/, returns: :json + post 'uploadfile' => 'packages#legacy_uploadfile', name: %r{[^/]+} + get 'upload/:name' => 'packages#upload', as: :upload, name: %r{[^/]+} + get 'upload/:name/json' => 'packages#upload', as: :upload_json, name: %r{[^/]+}, returns: :json + post 'upload/:name' => 'packages#upload_receive', as: :upload_receive, name: %r{[^/]+} + post 'upload/:name/json' => 'packages#upload_receive', as: :upload_receive_json, name: %r{[^/]+}, + returns: :json # TODO: "get" is probably the wrong method to delete a screenshot get 'delete_screenshot/:id' => 'packages#delete_screenshot', as: :delete_screenshot get 'hide_screenshot/:id' => 'packages#hide_screenshot', as: :hide_screenshot get 'unhide_screenshot/:id' => 'packages#unhide_screenshot', as: :unhide_screenshot - patch 'update_screenshot_description/:id' => 'packages#update_screenshot_description', as: :update_screenshot_description - #post 'report_screenshot/:id' => 'packages#report_screenshot', as: :report_screenshot + patch 'update_screenshot_description/:id' => 'packages#update_screenshot_description', + as: :update_screenshot_description + # post 'report_screenshot/:id' => 'packages#report_screenshot', as: :report_screenshot # TODO: "get" is probably the wrong method to delete a screenshot get 'approve_screenshot/:id' => 'packages#approve_screenshot', as: :approve_screenshot get 'about' => 'welcome#about' - get 'thumbnail/:name' => 'packages#thumbnail', as: :thumbnail_image, name: /[^\/]+/ - get 'thumbnail-404/:name' => 'packages#thumbnail', name: /[^\/]+/ - get 'thumbnail-with-version/:name/:version' => 'packages#thumbnail', name: /[^\/]+/, version: /\d.*/ - get 'screenshot/:name' => 'packages#screenshot', as: :screenshot_image, name: /[^\/]+/ - get 'screenshot-404/:name' => 'packages#screenshot', name: /[^\/]+/ - get 'screenshot-with-version/:name/:version' => 'packages#screenshot', name: /[^\/]+/, version: /\d.*/ + + get 'thumbnail/:name' => 'packages#send_image', as: :thumbnail_image, name: %r{[^/]+}, + defaults: { size: :thumb } + get 'thumbnail-404/:name' => 'packages#send_image', name: %r{[^/]+} + get 'thumbnail-with-version/:name/:version' => 'packages#send_image', name: %r{[^/]+}, + version: /\d.*/ + + # Return small image for a specific package + # TODO: needed at all? + get 'small/:name' => 'packages#send_image', as: :small_image, name: %r{[^/]+}, + defaults: { size: :small } + # Return small image by specific screenshot_id + get 'small/:name/:screenshot_id' => 'packages#send_image', as: :small_image_with_id, name: %r{[^/]+}, + defaults: { size: :small } + + # Return large image for a specific package + get 'screenshot/:name' => 'packages#send_image', as: :screenshot_image, name: %r{[^/]+}, + defaults: { size: :large } + # Return large image by specific screenshot_id + get 'screenshot/:name/:screenshot_id' => 'packages#send_image', as: :screenshot_image_with_id, name: %r{[^/]+}, + defaults: { size: :large } + get 'screenshot-404/:name' => 'packages#send_image', name: %r{[^/]+} + get 'screenshot-with-version/:name/:version' => 'packages#send_image', name: %r{[^/]+}, + version: /\d.*/ # Legacy URLs get 'with_screenshots', to: redirect('/packages?show=with') @@ -65,14 +85,15 @@ Rails.application.routes.draw do get 'json/package/:name' => 'json#package', as: :json_package, defaults: { format: :json } get 'json/packages' => 'json#packages', as: :json_packages, defaults: { format: :json } get 'json/screenshots' => 'json#screenshots', as: :json_screenshots, defaults: { format: :json } - get 'json/packages-without-screenshots' => 'json#packages_without_screenshots', defaults: { format: :json } + get 'json/packages-without-screenshots' => 'json#packages_without_screenshots', + defaults: { format: :json } # The priority is based upon order of creation: first created -> highest priority. # See how all your routes lay out with "rake routes". # You can have the root of your site routed with "root" root 'welcome#home' - #root to: 'packages#home' + # root to: 'packages#home' # Example of regular route: # get 'products/:id' => 'catalog#view'