From b81767c1b11ccc97a1c3ab5a959513bc8424f1dc Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 23 Mar 2023 12:01:04 +0000
Subject: [PATCH] SECURITY: Limit URL length for theme remote (#20787)

---
 app/controllers/admin/themes_controller.rb    | 8 ++++++++
 lib/url_helper.rb                             | 5 ++++-
 spec/lib/url_helper_spec.rb                   | 6 ++++++
 spec/requests/admin/themes_controller_spec.rb | 6 ++++++
 4 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/app/controllers/admin/themes_controller.rb b/app/controllers/admin/themes_controller.rb
index c25c2c17677..26733c5e9a5 100644
--- a/app/controllers/admin/themes_controller.rb
+++ b/app/controllers/admin/themes_controller.rb
@@ -3,6 +3,8 @@
 require "base64"
 
 class Admin::ThemesController < Admin::AdminController
+  MAX_REMOTE_LENGTH = 10_000
+
   skip_before_action :check_xhr, only: %i[show preview export]
   before_action :ensure_admin
 
@@ -86,6 +88,12 @@ class Admin::ThemesController < Admin::AdminController
         render json: @theme.errors, status: :unprocessable_entity
       end
     elsif remote = params[:remote]
+      if remote.length > MAX_REMOTE_LENGTH
+        error =
+          I18n.t("themes.import_error.not_allowed_theme", { repo: remote[0..MAX_REMOTE_LENGTH] })
+        return render_json_error(error, status: 422)
+      end
+
       begin
         guardian.ensure_allowed_theme_repo_import!(remote.strip)
       rescue Discourse::InvalidAccess
diff --git a/lib/url_helper.rb b/lib/url_helper.rb
index 320f3babd28..432464798b2 100644
--- a/lib/url_helper.rb
+++ b/lib/url_helper.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 class UrlHelper
+  MAX_URL_LENGTH = 100_000
+
   # At the moment this handles invalid URLs that browser address bar accepts
   # where second # is not encoded
   #
@@ -74,9 +76,10 @@ class UrlHelper
   end
 
   def self.normalized_encode(uri)
-    validated = nil
     url = uri.to_s
 
+    raise ArgumentError.new(:uri, "URL is too long") if url.length > MAX_URL_LENGTH
+
     # Ideally we will jump straight to `Addressable::URI.normalized_encode`. However,
     # that implementation has some edge-case issues like https://github.com/sporkmonger/addressable/issues/472.
     # To temporaily work around those issues for the majority of cases, we try parsing with `::URI`.
diff --git a/spec/lib/url_helper_spec.rb b/spec/lib/url_helper_spec.rb
index e95f65c14fc..e98d5ac829d 100644
--- a/spec/lib/url_helper_spec.rb
+++ b/spec/lib/url_helper_spec.rb
@@ -164,6 +164,12 @@ RSpec.describe UrlHelper do
         "https://test.com/original/3X/b/5/575bcc2886bf7a39684b57ca90be85f7d399bbc7.png?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AK8888999977/20200130/us-west-1/s3/aws4_request&X-Amz-Date=20200130T064355Z&X-Amz-Expires=15&X-Amz-SignedHeaders=host&X-Amz-Security-Token=blahblah+blahblah/blah//////////wEQA==&X-Amz-Signature=test"
       expect(UrlHelper.normalized_encode(presigned_url)).not_to eq(encoded_presigned_url)
     end
+
+    it "raises error if too long" do
+      expect do UrlHelper.normalized_encode("https://#{"a" * 100_000}.com") end.to raise_error(
+        ArgumentError,
+      )
+    end
   end
 
   describe "#local_cdn_url" do
diff --git a/spec/requests/admin/themes_controller_spec.rb b/spec/requests/admin/themes_controller_spec.rb
index 0b09e698e3b..5c494c2b395 100644
--- a/spec/requests/admin/themes_controller_spec.rb
+++ b/spec/requests/admin/themes_controller_spec.rb
@@ -241,6 +241,12 @@ RSpec.describe Admin::ThemesController do
         expect(response.status).to eq(422)
       end
 
+      it "fails to import with a failing status" do
+        post "/admin/themes/import.json", params: { remote: "https://#{"a" * 10_000}.com" }
+
+        expect(response.status).to eq(422)
+      end
+
       it "can lookup a private key by public key" do
         Discourse.redis.setex("ssh_key_abcdef", 1.hour, "rsa private key")