Merge branch 'feature/track-file-downloads' into staging

Author: tsubik

app/controllers/concerns/secure_send_file.rb

@@ -1,19 +1,29 @@
 # frozen_string_literal: true
 
 class PrivateUploadsController < ApplicationController
-  include SecureSendFile
-
   before_action :authenticate_user!
 
   rescue_from ActionController::MissingFile, with: :raise_not_found_exception
 
   def download
-    filepath = "#{params[:rest]}.#{params[:format]}"
-    secure_send_file allowed_filepath, filepath, disposition: :inline
+    sanitize_filepath
+    send_file @sanitized_filepath, disposition: :inline
   end
 
   private
 
+  def sanitize_filepath
+    filepath = "#{params[:rest]}.#{params[:format]}"
+    allowed_path = File.realpath(allowed_directory)
+    full_path = File.realpath(File.join(allowed_path, filepath))
+
+    raise_not_found_exception unless full_path.start_with?(allowed_path + File::SEPARATOR)
+
+    @sanitized_filepath = full_path
+  rescue Errno::ENOENT
+    raise_not_found_exception
+  end
+
   def authenticate_user!
     if current_user.present?
       super

app/controllers/private_uploads_controller.rb

@@ -1,11 +1,9 @@
 # frozen_string_literal: true
 
 class UploadsController < ApplicationController
-  include SecureSendFile
-
   rescue_from ActionController::MissingFile, with: :raise_not_found_exception
 
-  ALLOWED_MODELS_OVERRIDES = {
+  MODELS_OVERRIDES = {
     "operator_document_file" => "document_file",
     "documents" => "uploaded_document"
   }.freeze
@@ -21,28 +19,39 @@ class UploadsController < ApplicationController
   ].freeze
 
   def download
+    sanitize_filepath
     parse_upload_path
     track_download if trackable_request?
-    secure_send_file allowed_directory, @filepath, disposition: :inline
+    send_file @sanitized_filepath, disposition: :inline
   end
 
   private
 
+  def sanitize_filepath
+    filepath = "#{params[:rest]}.#{params[:format]}"
+    allowed_path = File.realpath(allowed_directory)
+    full_path = File.realpath(File.join(allowed_path, filepath))
+
+    raise_not_found_exception unless full_path.start_with?(allowed_path + File::SEPARATOR)
+
+    @sanitized_filepath = full_path
+    @relative_filepath = full_path.gsub(allowed_path + File::SEPARATOR, "")
+  rescue Errno::ENOENT
+    raise_not_found_exception
+  end
+
   def parse_upload_path
-    path_parts = "#{params[:rest]}.#{params[:format]}".split("/")
-    @filepath = "#{params[:rest]}.#{params[:format]}"
+    path_parts = @relative_filepath.split("/")
 
     raise_not_found_exception if path_parts.length < 4
 
     model_key = path_parts[0].downcase
-    @model_name = ALLOWED_MODELS_OVERRIDES[model_key] || model_key
+    @model_name = MODELS_OVERRIDES[model_key] || model_key
     @filename = path_parts[3..].join("/")
-
-    raise_not_known_model unless allowed_models.include?(@model_name) # extra security check
   end
 
   def track_download
-    TrackDownloadJob.perform_later(request.url, @filename, @model_name)
+    TrackFileDownloadJob.perform_later(request.url, @filename, @model_name)
   end
 
   def trackable_request?
@@ -71,22 +80,11 @@ def admin_panel_request?
     admin_paths.any? { |path| referer.include?(path) }
   end
 
-  def allowed_models
-    Dir.entries(Rails.root.join("uploads"))
-      .select { |entry| File.directory?(Rails.root.join("uploads", entry)) }
-      .reject { |entry| entry.start_with?(".") || entry == "tmp" }
-      .map { |entry| ALLOWED_MODELS_OVERRIDES[entry.downcase] || entry }
-  end
-
   def allowed_directory
     Rails.env.test? ? File.join(Rails.root, "tmp", "uploads") : File.join(Rails.root, "uploads")
   end
 
   def raise_not_found_exception
     raise ActionController::RoutingError, "Not Found"
   end
-
-  def raise_not_known_model
-    raise ActionController::RoutingError, "Not Known Model"
-  end
 end

app/controllers/uploads_controller.rb

@@ -1,4 +1,4 @@
-class TrackDownloadJob < ApplicationJob
+class TrackFileDownloadJob < ApplicationJob
   queue_as :default
 
   def perform(file_url, file_name, model_name)

app/jobs/track_download_job.rb app/jobs/track_file_download_job.rbapp/jobs/track_download_job.rb renamed to app/jobs/track_file_download_job.rb

@@ -0,0 +1,148 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+RSpec.describe UploadsController, type: :request do
+  before(:all) do
+    @etc_dir = Rails.root.join("tmp", "etc")
+    FileUtils.mkdir_p(@etc_dir)
+
+    @observation_report = create(:observation_report)
+    @document_file = create(:document_file) # operator_document_file
+    @donor = create(:donor) # donor logo
+
+    File.write(@etc_dir.join("passwd.txt"), "private")
+  end
+
+  before do
+    allow(TrackFileDownloadJob).to receive(:perform_later)
+  end
+
+  describe "GET /uploads/*path" do
+    context "successful file downloads" do
+      it "downloads existing files" do
+        get @document_file.attachment.url
+
+        expect(response).to have_http_status(:ok)
+        expect(response.headers["Content-Disposition"]).to include("inline")
+      end
+    end
+
+    context "file not found scenarios" do
+      it "returns 404 for non-existent files" do
+        get "/uploads/operator_document_file/123/456/nonexistent.pdf"
+
+        expect(response).to have_http_status(:not_found)
+      end
+
+      it "returns 404 for non-existent directories" do
+        get "/uploads/operator_document_file/999/888/test.pdf"
+
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "invalid path handling" do
+      it "returns 404 for insufficient path segments" do
+        get "/uploads/operator_document_file/123.pdf"
+
+        expect(response).to have_http_status(:not_found)
+      end
+
+      it "returns 404 for unknown models" do
+        get "/uploads/unknown_model/123/456/test.pdf"
+
+        expect(response).to have_http_status(:not_found)
+      end
+
+      it "returns 404 for empty paths" do
+        get "/uploads/.pdf"
+
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "download tracking" do
+      it "tracks downloads for trackable models with regular browsers" do
+        get @document_file.attachment.url, headers: {
+          "User-Agent" => "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36"
+        }
+
+        expect(response).to have_http_status(:ok)
+        expect(TrackFileDownloadJob).to have_received(:perform_later).with(
+          request.url, @document_file.attachment.filename, "document_file"
+        )
+
+        get @observation_report.attachment.url, headers: {
+          "User-Agent" => "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36"
+        }
+
+        expect(response).to have_http_status(:ok)
+        expect(TrackFileDownloadJob).to have_received(:perform_later).with(
+          request.url, @observation_report.attachment.filename, "observation_report"
+        )
+      end
+
+      it "does not track bot requests" do
+        get @document_file.attachment.url, headers: {
+          "User-Agent" => "Googlebot/2.1"
+        }
+
+        expect(response).to have_http_status(:ok)
+        expect(TrackFileDownloadJob).not_to have_received(:perform_later)
+      end
+
+      it "does not track admin panel requests" do
+        admin_referers = [
+          "https://example.com/admin",
+          "https://example.com/admin/documents",
+          "https://example.com/observations-tool",
+          "https://example.com/observations-tool/reports"
+        ]
+
+        admin_referers.each do |referer|
+          get @document_file.attachment.url, headers: {
+            "User-Agent" => "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36",
+            "Referer" => referer
+          }
+
+          expect(response).to have_http_status(:ok)
+          expect(TrackFileDownloadJob).not_to have_received(:perform_later)
+        end
+      end
+
+      it "does not track for not trackable model" do
+        get @donor.logo.url, headers: {
+          "User-Agent" => "Mozilla/5.0 (Chrome/91.0) Safari/537.36"
+        }
+
+        expect(response).to have_http_status(:ok)
+        expect(TrackFileDownloadJob).not_to have_received(:perform_later)
+      end
+    end
+
+    context "edge cases and error handling" do
+      it "handles requests without user agent" do
+        get @observation_report.attachment.url
+
+        expect(response).to have_http_status(:ok)
+        expect(TrackFileDownloadJob).to have_received(:perform_later)
+      end
+
+      it "handles requests without referer" do
+        get @document_file.attachment.url, headers: {
+          "User-Agent" => "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36"
+        }
+
+        expect(response).to have_http_status(:ok)
+        expect(TrackFileDownloadJob).to have_received(:perform_later)
+      end
+
+      it "prevents directory traversal" do
+        get "/uploads/operator_document_file/../../etc/passwd.txt"
+
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+  end
+end

spec/integration/uploads_spec.rb

@@ -0,0 +1,77 @@
+require "rails_helper"
+
+RSpec.describe TrackFileDownloadJob, type: :job do
+  let(:file_url) { "https://example.com/files/document.pdf" }
+  let(:file_name) { "document.pdf" }
+  let(:model_name) { "User" }
+  let(:measurement_id) { "G-XXXXXXXXXX" }
+  let(:api_secret) { "test_api_secret" }
+
+  before do
+    ENV["GA4_MEASUREMENT_ID"] = measurement_id
+    ENV["GA4_API_SECRET"] = api_secret
+  end
+
+  after do
+    ENV.delete("GA4_MEASUREMENT_ID")
+    ENV.delete("GA4_API_SECRET")
+  end
+
+  describe "#perform" do
+    context "when environment variables are present" do
+      let(:expected_payload) do
+        {
+          events: [{
+            name: "file_download",
+            params: {
+              file_name: file_name,
+              file_extension: "pdf",
+              file_url: file_url,
+              link_url: file_url,
+              model_name: model_name
+            }
+          }]
+        }
+      end
+
+      let(:expected_params) do
+        {
+          measurement_id: measurement_id,
+          api_secret: api_secret
+        }
+      end
+
+      it "sends a GA4 event with correct payload" do
+        expect(HTTP).to receive(:post).with(
+          "https://www.google-analytics.com/mp/collect",
+          params: expected_params,
+          json: expected_payload
+        )
+
+        described_class.perform_now(file_url, file_name, model_name)
+      end
+    end
+
+    context "when measurement_id is blank" do
+      before do
+        ENV["GA4_MEASUREMENT_ID"] = ""
+      end
+
+      it "does not send GA4 event" do
+        expect(HTTP).not_to receive(:post)
+        described_class.perform_now(file_url, file_name, model_name)
+      end
+    end
+
+    context "when api_secret is blank" do
+      before do
+        ENV["GA4_API_SECRET"] = ""
+      end
+
+      it "does not send GA4 event" do
+        expect(HTTP).not_to receive(:post)
+        described_class.perform_now(file_url, file_name, model_name)
+      end
+    end
+  end
+end