From 1f3670e778a03ab47ebe0739d170f880f585b3ca Mon Sep 17 00:00:00 2001
From: Jeremy Walker <jez.walker@gmail.com>
Date: Tue, 27 Oct 2026 13:45:08 +0900
Subject: [PATCH 1/2] Normalise submitted filenames

---
 app/commands/submission/create.rb             | 16 +++++-
 app/commands/submission/prepare_http_files.rb |  2 +-
 app/commands/tooling_job/upload_to_efs.rb     | 15 ++++--
 .../github_solution_syncer/local_git_repo.rb  | 21 +++++++-
 .../api/solutions/submissions_controller.rb   |  2 +
 .../api/v1/solutions_controller.rb            |  2 +
 config/initializers/exceptions.rb             |  2 +
 config/locales/api/en.yml                     |  1 +
 test/commands/submission/create_test.rb       | 37 ++++++++++++++
 .../submission/prepare_http_files_test.rb     |  9 ++++
 .../tooling_job/upload_to_efs_test.rb         | 36 ++++++++++++++
 .../local_git_repo_test.rb                    | 49 +++++++++++++++++++
 .../solutions/submissions_controller_test.rb  | 19 +++++++
 .../api/v1/solutions_controller_test.rb       | 23 +++++++++
 14 files changed, 226 insertions(+), 8 deletions(-)
 create mode 100644 test/commands/tooling_job/upload_to_efs_test.rb
 create mode 100644 test/commands/user/github_solution_syncer/local_git_repo_test.rb

diff --git a/app/commands/submission/create.rb b/app/commands/submission/create.rb
index a65d689577..2fcd969146 100644
--- a/app/commands/submission/create.rb
+++ b/app/commands/submission/create.rb
@@ -8,7 +8,6 @@ def initialize(solution, raw_files, submitted_via, test_results_json = nil)
     @test_results_json = test_results_json
 
     # TODO: (Optional) - Move this into another service
-    # TODO: (Optional) - Consider risks around filenames
     @submitted_files = raw_files.each do |f|
       f[:digest] = Digest::SHA1.hexdigest(f[:content])
     end
@@ -16,6 +15,7 @@ def initialize(solution, raw_files, submitted_via, test_results_json = nil)
 
   def call
     # This needs to be fast.
+    validate_filenames!
     guard!
 
     create_submission!
@@ -33,6 +33,20 @@ def call
 
   delegate :track, :user, to: :solution
 
+  def validate_filenames!
+    raise InvalidSubmissionFilenameError unless submitted_files.all? { |f| valid_filename?(f[:filename]) }
+  end
+
+  # Filenames must be relative paths made up of named segments
+  def valid_filename?(filename)
+    return false unless filename.is_a?(String)
+    return false if filename.empty?
+    return false if filename.start_with?("/")
+    return false if filename.include?("\0") || filename.include?("\\")
+
+    filename.split("/", -1).none? { |segment| segment.empty? || segment == ".." }
+  end
+
   # In this guard we check the last submission that wasn't
   # cancelled or exceptioned.
   def guard!
diff --git a/app/commands/submission/prepare_http_files.rb b/app/commands/submission/prepare_http_files.rb
index 84a9847ad6..ffca4b5c37 100644
--- a/app/commands/submission/prepare_http_files.rb
+++ b/app/commands/submission/prepare_http_files.rb
@@ -11,7 +11,7 @@ def call
         map(&:strip).
         detect { |s| s.start_with?('filename=') }.
         split("=").last.
-        delete('"').gsub(%r{^/}, '')
+        delete('"').tr('\\', '/').gsub(%r{^/}, '')
 
       content = file.read
 
diff --git a/app/commands/tooling_job/upload_to_efs.rb b/app/commands/tooling_job/upload_to_efs.rb
index b05c612406..50734f7b5e 100644
--- a/app/commands/tooling_job/upload_to_efs.rb
+++ b/app/commands/tooling_job/upload_to_efs.rb
@@ -7,12 +7,17 @@ def call
     return if Dir.exist?(efs_dir)
 
     submission_files.each do |file|
-      efs_path = "#{efs_dir}/#{file.filename}"
-      FileUtils.mkdir_p(
-        efs_path.split("/").tap(&:pop).join("/"),
-        mode: 0o777
-      )
+      efs_path = File.expand_path(File.join(base_dir, file.filename))
+
+      # Only write files that sit inside the job's directory
+      next unless efs_path.start_with?("#{base_dir}/")
+
+      FileUtils.mkdir_p(File.dirname(efs_path), mode: 0o777)
       File.open(efs_path, 'w') { |f| f.write(file.utf8_content) }
     end
   end
+
+  private
+  memoize
+  def base_dir = File.expand_path(efs_dir)
 end
diff --git a/app/commands/user/github_solution_syncer/local_git_repo.rb b/app/commands/user/github_solution_syncer/local_git_repo.rb
index 069a19e811..e9be410132 100644
--- a/app/commands/user/github_solution_syncer/local_git_repo.rb
+++ b/app/commands/user/github_solution_syncer/local_git_repo.rb
@@ -25,7 +25,9 @@ def update
     end
 
     def write_file(path, content)
-      full_path = File.join(@path, path)
+      full_path = File.expand_path(File.join(@path, path))
+      return unless working_tree_path?(full_path)
+
       FileUtils.mkdir_p(File.dirname(full_path))
       File.write(full_path, content)
     end
@@ -81,6 +83,23 @@ def push_branch
       git("push", "--set-upstream", "origin", new_branch_name)
     end
 
+    # Only write files that sit inside the repository's working tree,
+    # resolving any links that already exist along the path.
+    def working_tree_path?(full_path)
+      return false if File.symlink?(full_path)
+
+      existing_path = File.dirname(full_path)
+      existing_path = File.dirname(existing_path) until File.exist?(existing_path) || File.symlink?(existing_path)
+      resolved_path = File.join(File.realpath(existing_path), full_path.delete_prefix(existing_path))
+
+      base_path = File.realpath(@path)
+      return false unless resolved_path.start_with?("#{base_path}/")
+
+      !resolved_path.delete_prefix("#{base_path}/").split("/").first.casecmp?(".git")
+    rescue SystemCallError
+      false
+    end
+
     def git(*args)
       git_dir = File.join(@path, '.git')
       work_tree = @path
diff --git a/app/controllers/api/solutions/submissions_controller.rb b/app/controllers/api/solutions/submissions_controller.rb
index 36de968470..b4075fd26d 100644
--- a/app/controllers/api/solutions/submissions_controller.rb
+++ b/app/controllers/api/solutions/submissions_controller.rb
@@ -19,6 +19,8 @@ def create
       submission = Submission::Create.(solution, files, :api, params[:test_results_json])
     rescue DuplicateSubmissionError
       return render_error(400, :duplicate_submission)
+    rescue InvalidSubmissionFilenameError
+      return render_error(400, :invalid_filename)
     end
 
     render json: {
diff --git a/app/controllers/api/v1/solutions_controller.rb b/app/controllers/api/v1/solutions_controller.rb
index 0b1c9edb04..9767bc6811 100644
--- a/app/controllers/api/v1/solutions_controller.rb
+++ b/app/controllers/api/v1/solutions_controller.rb
@@ -69,6 +69,8 @@ def update
       Iteration::Create.(solution, submission)
     rescue DuplicateSubmissionError
       return render_error(400, :duplicate_submission)
+    rescue InvalidSubmissionFilenameError
+      return render_error(400, :invalid_filename)
     end
 
     render json: {}, status: :created
diff --git a/config/initializers/exceptions.rb b/config/initializers/exceptions.rb
index dbaaaca168..228ae93a5a 100644
--- a/config/initializers/exceptions.rb
+++ b/config/initializers/exceptions.rb
@@ -10,6 +10,8 @@ class TrainerCriteriaNotFulfilledError < RuntimeError; end
 
 class DuplicateSubmissionError < RuntimeError; end
 
+class InvalidSubmissionFilenameError < RuntimeError; end
+
 class DuplicateVideoError < RuntimeError; end
 
 class ExerciseLockedError < RuntimeError; end
diff --git a/config/locales/api/en.yml b/config/locales/api/en.yml
index 58d069b54f..02e25ea284 100644
--- a/config/locales/api/en.yml
+++ b/config/locales/api/en.yml
@@ -7,6 +7,7 @@ en:
       not_trainer: "You do not have trainer permissions for this track"
       not_maintainer: "You do not have maintainer permissions"
       duplicate_submission: "No files you submitted have changed since your last submission"
+      invalid_filename: "One or more of the filenames you submitted are not valid"
       duplicate_video: "The video has already been submitted by someone else"
       exercise_locked: "This exercise is locked"
       exercise_not_found: "The exercise you specified could not be found"
diff --git a/test/commands/submission/create_test.rb b/test/commands/submission/create_test.rb
index 8ac293559f..9cd3e1c77c 100644
--- a/test/commands/submission/create_test.rb
+++ b/test/commands/submission/create_test.rb
@@ -64,6 +64,43 @@ class Submission::CreateTest < ActiveSupport::TestCase
     Submission::Create.(solution.reload, files, :cli)
   end
 
+  test "accepts nested filenames" do
+    solution = create :concept_solution
+    files = [{ filename: "lib/sub.dir/foo..bar.rb", content: "foo" }]
+
+    submission = Submission::Create.(solution, files, :cli)
+
+    assert_equal "lib/sub.dir/foo..bar.rb", submission.files.first.filename
+  end
+
+  test "raises for invalid filenames" do
+    solution = create :concept_solution
+
+    [
+      nil,
+      "",
+      "/foo.rb",
+      "../foo.rb",
+      "subdir/../../foo.rb",
+      "subdir/..",
+      "subdir//foo.rb",
+      "subdir/",
+      "foo\0.rb",
+      "subdir\\foo.rb"
+    ].each do |filename|
+      files = [
+        { filename: "valid.rb", content: "valid" },
+        { filename:, content: "foo" }
+      ]
+
+      assert_raises InvalidSubmissionFilenameError, "Expected #{filename.inspect} to be invalid" do
+        Submission::Create.(solution, files, :cli)
+      end
+    end
+
+    assert_empty solution.reload.submissions
+  end
+
   test "awards rookie badge" do
     # Generic setup
     files = [{ filename: 'foo.bar', content: "foobar" }]
diff --git a/test/commands/submission/prepare_http_files_test.rb b/test/commands/submission/prepare_http_files_test.rb
index 5992254180..4a1f04c4b9 100644
--- a/test/commands/submission/prepare_http_files_test.rb
+++ b/test/commands/submission/prepare_http_files_test.rb
@@ -25,6 +25,15 @@ class Submission::PrepareHttpFilesTest < ActiveSupport::TestCase
     assert_equal content_2, second_file[:content]
   end
 
+  test "uses forward slashes in filenames" do
+    headers = "Content-Disposition: form-data; name=\"files[]\"; filename=\"subdir\\foobar.rb\"\r\nContent-Type: application/octet-stream\r\n" # rubocop:disable Layout/LineLength
+    file = mock(read: "content", headers:)
+
+    files = Submission::PrepareHttpFiles.([file])
+
+    assert_equal "subdir/foobar.rb", files.first[:filename]
+  end
+
   test "raises if file is too large" do
     filename = "subdir/foobar.rb"
     content = Array.new(1.megabyte + 1, 'x')
diff --git a/test/commands/tooling_job/upload_to_efs_test.rb b/test/commands/tooling_job/upload_to_efs_test.rb
new file mode 100644
index 0000000000..d1fbdab73b
--- /dev/null
+++ b/test/commands/tooling_job/upload_to_efs_test.rb
@@ -0,0 +1,36 @@
+require 'test_helper'
+
+class ToolingJob::UploadToEFSTest < ActiveSupport::TestCase
+  test "writes files into the directory" do
+    file = Struct.new(:filename, :utf8_content)
+    Dir.mktmpdir do |tmp_dir|
+      efs_dir = "#{tmp_dir}/job"
+      files = [
+        file.new("foo.rb", "foo"),
+        file.new("subdir/bar.rb", "bar")
+      ]
+
+      ToolingJob::UploadToEFS.(efs_dir, files)
+
+      assert_equal "foo", File.read("#{efs_dir}/foo.rb")
+      assert_equal "bar", File.read("#{efs_dir}/subdir/bar.rb")
+    end
+  end
+
+  test "ignores files with invalid filenames" do
+    file = Struct.new(:filename, :utf8_content)
+    Dir.mktmpdir do |tmp_dir|
+      efs_dir = "#{tmp_dir}/jobs/job"
+      files = [
+        file.new("../../foo.rb", "foo"),
+        file.new("subdir/../../bar.rb", "bar"),
+        file.new("baz.rb", "baz")
+      ]
+
+      ToolingJob::UploadToEFS.(efs_dir, files)
+
+      written = Dir.glob("#{tmp_dir}/**/*").select { |path| File.file?(path) }
+      assert_equal ["#{efs_dir}/baz.rb"], written
+    end
+  end
+end
diff --git a/test/commands/user/github_solution_syncer/local_git_repo_test.rb b/test/commands/user/github_solution_syncer/local_git_repo_test.rb
new file mode 100644
index 0000000000..45e4219d96
--- /dev/null
+++ b/test/commands/user/github_solution_syncer/local_git_repo_test.rb
@@ -0,0 +1,49 @@
+require 'test_helper'
+
+class User::GithubSolutionSyncer::LocalGitRepoTest < ActiveSupport::TestCase
+  test "write_file writes files into the working tree" do
+    within_repo do |repo, dir|
+      repo.write_file("solutions/ruby/bob/1/bob.rb", "bob")
+
+      assert_equal "bob", File.read("#{dir}/solutions/ruby/bob/1/bob.rb")
+    end
+  end
+
+  test "write_file ignores invalid paths" do
+    within_repo do |repo, dir|
+      repo.write_file("../foo.rb", "foo")
+      repo.write_file(".git/config", "config")
+      repo.write_file("solutions/../.git/info/foo", "foo")
+
+      refute File.exist?(File.join(File.dirname(dir), "foo.rb"))
+      refute File.exist?("#{dir}/.git")
+    end
+  end
+
+  test "write_file does not follow links out of the working tree" do
+    Dir.mktmpdir do |outside_dir|
+      within_repo do |repo, dir|
+        File.symlink(outside_dir, "#{dir}/linked")
+        File.symlink("#{outside_dir}/target.rb", "#{dir}/link.rb")
+
+        repo.write_file("linked/foo.rb", "foo")
+        repo.write_file("link.rb", "foo")
+
+        assert_empty Dir.children(outside_dir)
+      end
+    end
+  end
+
+  private
+  def within_repo
+    Dir.mktmpdir do |parent_dir|
+      dir = "#{parent_dir}/repo"
+      FileUtils.mkdir_p(dir)
+
+      repo = User::GithubSolutionSyncer::LocalGitRepo.new(mock, "main", "new-branch")
+      repo.instance_variable_set(:@path, dir)
+
+      yield repo, dir
+    end
+  end
+end
diff --git a/test/controllers/api/solutions/submissions_controller_test.rb b/test/controllers/api/solutions/submissions_controller_test.rb
index b8d695231e..9569eebd48 100644
--- a/test/controllers/api/solutions/submissions_controller_test.rb
+++ b/test/controllers/api/solutions/submissions_controller_test.rb
@@ -150,6 +150,25 @@ class API::Solutions::SubmissionsControllerTest < API::BaseTestCase
     assert_equal expected, actual
   end
 
+  test "create should catch invalid filenames" do
+    setup_user
+    solution = create :concept_solution, user: @current_user
+
+    post api_solution_submissions_path(solution.uuid),
+      params: { files: [{ filename: "../foo.rb", content: "foo" }] },
+      headers: @headers,
+      as: :json
+
+    assert_response :bad_request
+    expected = { error: {
+      type: "invalid_filename",
+      message: I18n.t('api.errors.invalid_filename')
+    } }
+    actual = JSON.parse(response.body, symbolize_names: true)
+    assert_equal expected, actual
+    assert_empty solution.reload.submissions
+  end
+
   test "create should catch duplicate submission" do
     setup_user
     solution = create :concept_solution, user: @current_user
diff --git a/test/controllers/api/v1/solutions_controller_test.rb b/test/controllers/api/v1/solutions_controller_test.rb
index 72dc3de8d0..235fd0dce9 100644
--- a/test/controllers/api/v1/solutions_controller_test.rb
+++ b/test/controllers/api/v1/solutions_controller_test.rb
@@ -313,6 +313,29 @@ class API::V1::SolutionsControllerTest < API::BaseTestCase
     # Run the jobs through to make sure nothing unexpected happens
     perform_enqueued_jobs
   end
+  test "update should catch invalid filenames" do
+    setup_user
+    solution = create :concept_solution, user: @current_user
+
+    http_files = [SecureRandom.uuid]
+    files = [{ filename: "../foo.rb", content: "foo" }]
+    Submission::PrepareHttpFiles.expects(:call).with(http_files).returns(files)
+
+    patch api_v1_solution_path(solution.uuid),
+      params: { files: http_files },
+      headers: @headers,
+      as: :json
+
+    assert_response :bad_request
+    expected = { error: {
+      type: "invalid_filename",
+      message: I18n.t('api.errors.invalid_filename')
+    } }
+    actual = JSON.parse(response.body, symbolize_names: true)
+    assert_equal expected, actual
+    assert_empty solution.reload.submissions
+  end
+
   test "update should catch duplicate submission" do
     setup_user
     solution = create :concept_solution, user: @current_user

From 41c4504c2aea516901d177f7c31b82887bfed33d Mon Sep 17 00:00:00 2001
From: Jeremy Walker <jez.walker@gmail.com>
Date: Tue, 27 Oct 2026 15:26:39 +0900
Subject: [PATCH 2/2] Leave CLI filename parsing unchanged

---
 app/commands/submission/prepare_http_files.rb       | 2 +-
 test/commands/submission/prepare_http_files_test.rb | 9 ---------
 2 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/app/commands/submission/prepare_http_files.rb b/app/commands/submission/prepare_http_files.rb
index ffca4b5c37..84a9847ad6 100644
--- a/app/commands/submission/prepare_http_files.rb
+++ b/app/commands/submission/prepare_http_files.rb
@@ -11,7 +11,7 @@ def call
         map(&:strip).
         detect { |s| s.start_with?('filename=') }.
         split("=").last.
-        delete('"').tr('\\', '/').gsub(%r{^/}, '')
+        delete('"').gsub(%r{^/}, '')
 
       content = file.read
 
diff --git a/test/commands/submission/prepare_http_files_test.rb b/test/commands/submission/prepare_http_files_test.rb
index 4a1f04c4b9..5992254180 100644
--- a/test/commands/submission/prepare_http_files_test.rb
+++ b/test/commands/submission/prepare_http_files_test.rb
@@ -25,15 +25,6 @@ class Submission::PrepareHttpFilesTest < ActiveSupport::TestCase
     assert_equal content_2, second_file[:content]
   end
 
-  test "uses forward slashes in filenames" do
-    headers = "Content-Disposition: form-data; name=\"files[]\"; filename=\"subdir\\foobar.rb\"\r\nContent-Type: application/octet-stream\r\n" # rubocop:disable Layout/LineLength
-    file = mock(read: "content", headers:)
-
-    files = Submission::PrepareHttpFiles.([file])
-
-    assert_equal "subdir/foobar.rb", files.first[:filename]
-  end
-
   test "raises if file is too large" do
     filename = "subdir/foobar.rb"
     content = Array.new(1.megabyte + 1, 'x')