Normalise submitted filenames

app/commands/submission/create.rb

@@ -8,14 +8,14 @@ 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
   end
 
   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!

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

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
@@ -84,6 +86,23 @@ def branch_exists?(name)
       git_output("branch", "--list", name).strip.present?
     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)
       system(*git_command(*args), exception: true)
     end

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: {

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

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

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"

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" }]

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

test/commands/user/github_solution_syncer/local_git_repo_test.rb

@@ -50,6 +50,39 @@ class LocalGitRepoTest < ActiveSupport::TestCase
       end
     end
 
+    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 with_local_repo(new_branch_name)
       syncer = build(:user_github_solution_syncer)
@@ -66,6 +99,18 @@ def with_local_repo(new_branch_name)
       end
     end
 
+    def within_repo
+      Dir.mktmpdir do |parent_dir|
+        dir = "#{parent_dir}/repo"
+        FileUtils.mkdir_p(dir)
+
+        repo = LocalGitRepo.new(mock, "main", "new-branch")
+        repo.instance_variable_set(:@path, dir)
+
+        yield repo, dir
+      end
+    end
+
     def run_git(dir, *args)
       stdout, status = Open3.capture2("git", "-C", dir, *args)
       raise "git #{args.first} failed" unless status.success?

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

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