[deploy] Validate filenames server-side (#7405)

app/controllers/application_controller.rb

@@ -5,6 +5,7 @@ class ApplicationController < ActionController::Base
   include ValidRequest
   include Pundit
   include FastlyHeaders
+  include ImageUploads
 
   rescue_from ActionView::MissingTemplate, with: :routing_error
 
@@ -77,30 +78,4 @@ class ApplicationController < ActionController::Base
   def touch_current_user
     current_user.touch
   end
-
-  def log_image_data_to_datadog
-    images = Array.wrap(
-      params.dig("user", "profile_image") ||
-      params.dig("podcast", "image") ||
-      params.dig("organization", "profile_image") ||
-      params["image"],
-    )
-
-    raise if images.empty?
-
-    images.each do |image|
-      tags = [
-        "controller:#{params['controller']}",
-        "action:#{params['action']}",
-        "content_type:#{image.content_type}",
-        "original_filename:#{image.original_filename}",
-        "tempfile:#{image.tempfile}",
-        "size:#{image.size}",
-      ]
-
-      DatadogStatsClient.increment("image_upload_error", tags: tags)
-    end
-
-    raise
-  end
 end

app/controllers/concerns/image_uploads.rb

@@ -0,0 +1,36 @@
+module ImageUploads
+  extend ActiveSupport::Concern
+
+  MAX_FILENAME_LENGTH = 250
+  FILENAME_TOO_LONG_MESSAGE = "filename too long - the max is #{MAX_FILENAME_LENGTH} characters.".freeze
+
+  def long_filename?(image)
+    image&.original_filename && image.original_filename.length > MAX_FILENAME_LENGTH
+  end
+
+  def log_image_data_to_datadog
+    images = Array.wrap(
+      params.dig("user", "profile_image") ||
+      params.dig("podcast", "image") ||
+      params.dig("organization", "profile_image") ||
+      params["image"],
+    )
+
+    raise if images.empty?
+
+    images.each do |image|
+      tags = [
+        "controller:#{params['controller']}",
+        "action:#{params['action']}",
+        "content_type:#{image.content_type}",
+        "original_filename:#{image.original_filename}",
+        "tempfile:#{image.tempfile}",
+        "size:#{image.size}",
+      ]
+
+      DatadogStatsClient.increment("image_upload_error", tags: tags)
+    end
+
+    raise
+  end
+end

app/controllers/image_uploads_controller.rb

@@ -12,6 +12,13 @@ class ImageUploadsController < ApplicationController
       raise RateLimitChecker::UploadRateLimitReached if rate_limiter.limit_by_action("image_upload")
       raise CarrierWave::IntegrityError if params[:image].blank?
 
+      unless valid_filename?
+        respond_to do |format|
+          format.json { render json: { error: FILENAME_TOO_LONG_MESSAGE }, status: :unprocessable_entity }
+        end
+        return
+      end
+
       uploaders = upload_images(params[:image], rate_limiter)
     rescue RateLimitChecker::UploadRateLimitReached
       respond_to do |format|
@@ -46,6 +53,11 @@ class ImageUploadsController < ApplicationController
 
   private
 
+  def valid_filename?
+    images = Array.wrap(params.dig("image"))
+    images.none? { |image| long_filename?(image) }
+  end
+
   def upload_images(images, rate_limiter)
     Array.wrap(images).map do |image|
       ArticleImageUploader.new.tap do |uploader|

app/controllers/organizations_controller.rb

@@ -6,6 +6,12 @@ class OrganizationsController < ApplicationController
     @tab = "organization"
     @user = current_user
     @tab_list = @user.settings_tab_list
+
+    unless valid_filename?
+      render template: "users/edit"
+      return
+    end
+
     @organization = Organization.new(organization_params)
     authorize @organization
     if @organization.save
@@ -23,6 +29,11 @@ class OrganizationsController < ApplicationController
     @tab_list = @user.settings_tab_list
     set_organization
 
+    unless valid_filename?
+      render template: "users/edit"
+      return
+    end
+
     if @organization.update(organization_params.merge(profile_updated_at: Time.current))
       flash[:settings_notice] = "Your organization was successfully updated."
       redirect_to "/settings/organization"
@@ -85,4 +96,17 @@ class OrganizationsController < ApplicationController
     not_found unless @organization
     authorize @organization
   end
+
+  def valid_filename?
+    image = params.dig("organization", "profile_image")
+    return true unless long_filename?(image)
+
+    if action_name == "create"
+      @organization = Organization.new(organization_params.except(:profile_image))
+      authorize @organization
+    end
+
+    @organization.errors.add(:profile_image, FILENAME_TOO_LONG_MESSAGE)
+    false
+  end
 end

app/controllers/podcasts_controller.rb

@@ -14,6 +14,11 @@ class PodcastsController < ApplicationController
   end
 
   def create
+    unless valid_filename?
+      render :new
+      return
+    end
+
     @podcast = Podcast.new(podcast_params)
     @podcast.creator = current_user
 
@@ -52,4 +57,18 @@ class PodcastsController < ApplicationController
   ensure
     Bullet.enable = previous_value
   end
+
+  def valid_filename?
+    image = params.dig("podcast", "image")
+    return true unless long_filename?(image)
+
+    @podcast = Podcast.new(podcast_params.except(:image))
+    @podcast.creator = current_user
+    @podcast.errors.add(:image, FILENAME_TOO_LONG_MESSAGE)
+
+    @podcasts = Podcast.available.order(title: :asc)
+    @podcast_index = true
+
+    false
+  end
 end

app/controllers/users_controller.rb

@@ -44,6 +44,12 @@ class UsersController < ApplicationController
   # PATCH/PUT /users/:id.:format
   def update
     set_tabs(params["user"]["tab"])
+
+    unless valid_filename?
+      render :edit, status: :bad_request
+      return
+    end
+
     if @user.update(permitted_attributes(@user))
       RssReaderFetchUserWorker.perform_async(@user.id) if @user.feed_url.present?
       notice = "Your profile was successfully updated."
@@ -343,4 +349,16 @@ class UsersController < ApplicationController
     # last one is a fallback in case both are nil
     range.cover? user_identity_age
   end
+
+  def valid_filename?
+    image = params.dig("user", "profile_image")
+    return true unless long_filename?(image)
+
+    @user.errors.add(:profile_image, FILENAME_TOO_LONG_MESSAGE)
+
+    Honeycomb.add_field("error", @user.errors.messages)
+    Honeycomb.add_field("errored", true)
+
+    false
+  end
 end

spec/requests/image_uploads_spec.rb

@@ -82,6 +82,12 @@ RSpec.describe "ImageUploads", type: :request do
 
         expect(DatadogStatsClient).to have_received(:increment).with("image_upload_error", tags)
       end
+
+      it "returns error if image file name is too long" do
+        allow(image).to receive(:original_filename).and_return("#{'a_very_long_filename' * 15}.png")
+        post "/image_uploads", headers: headers, params: { image: image }
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
     end
 
     context "when uploading rate limiting works" do

spec/requests/organizations_update_spec.rb

@@ -43,7 +43,7 @@ RSpec.describe "OrganizationsUpdate", type: :request do
     end.to raise_error(ActiveRecord::RecordNotFound)
   end
 
-  it "catches error if image file name is too long" do
+  it "catches error if profile image file name is too long" do
     organization = user.organizations.first
     allow(Organization).to receive(:find_by).and_return(organization)
     allow(organization).to receive(:update).and_raise(Errno::ENAMETOOLONG)
@@ -57,4 +57,15 @@ RSpec.describe "OrganizationsUpdate", type: :request do
 
     expect(DatadogStatsClient).to have_received(:increment).with("image_upload_error", tags)
   end
+
+  it "returns error if profile image file name is too long" do
+    organization = user.organizations.first
+    allow(Organization).to receive(:find_by).and_return(organization)
+    image = fixture_file_upload("files/800x600.png", "image/png")
+    allow(image).to receive(:original_filename).and_return("#{'a_very_long_filename' * 15}.png")
+
+    put "/organizations/#{org_id}", params: { organization: { id: org_id, profile_image: image } }
+
+    expect(response.body).to include("filename too long")
+  end
 end

spec/requests/podcasts/podcast_create_spec.rb

@@ -89,5 +89,13 @@ RSpec.describe "Podcast Create", type: :request do
 
       expect(DatadogStatsClient).to have_received(:increment).with("image_upload_error", tags)
     end
+
+    it "returns error if image file name is too long" do
+      image = fixture_file_upload("files/podcast.png", "image/png")
+      allow(image).to receive(:original_filename).and_return("#{'a_very_long_filename' * 15}.png")
+      valid_attributes[:image] = image
+      post podcasts_path, params: { podcast: valid_attributes }
+      expect(response.body).to include("Suggest a Podcast")
+    end
   end
 end

spec/requests/user/user_organization_spec.rb

@@ -49,7 +49,7 @@ RSpec.describe "UserOrganization", type: :request do
     end
   end
 
-  it "catches error if image file name is too long" do
+  it "catches error if profile image file name is too long" do
     sign_in user
     org_params = build(:organization).attributes
     org_params["profile_image"] = fixture_file_upload("files/800x600.png", "image/png")
@@ -66,6 +66,18 @@ RSpec.describe "UserOrganization", type: :request do
     expect(DatadogStatsClient).to have_received(:increment).with("image_upload_error", tags)
   end
 
+  it "returns error if profile image file name is too long" do
+    sign_in user
+    org_params = build(:organization).attributes
+    image = fixture_file_upload("files/800x600.png", "image/png")
+    allow(image).to receive(:original_filename).and_return("#{'a_very_long_filename' * 15}.png")
+    org_params["profile_image"] = image
+    allow(Organization).to receive(:new).and_return(organization)
+
+    post "/organizations", params: { organization: org_params }
+    expect(response.body).to include("filename too long")
+  end
+
   context "when leaving an org" do
     let(:org_member) { create(:user, :org_member) }
 

spec/requests/user/user_settings_spec.rb

@@ -151,6 +151,15 @@ RSpec.describe "UserSettings", type: :request do
       expect(DatadogStatsClient).to have_received(:increment).with("image_upload_error", tags)
     end
 
+    it "returns error if Profile image file name is too long" do
+      profile_image = fixture_file_upload("files/800x600.png", "image/png")
+      allow(profile_image).to receive(:original_filename).and_return("#{'a_very_long_filename' * 15}.png")
+
+      put "/users/#{user.id}", params: { user: { tab: "profile", profile_image: profile_image } }
+
+      expect(response).to have_http_status(:bad_request)
+    end
+
     context "when requesting an export of the articles" do
       def send_request(flag = true)
         put "/users/#{user.id}", params: {