Validate GitHub syncer branch names

app/commands/user/github_solution_syncer/local_git_repo.rb

@@ -31,7 +31,7 @@ def write_file(path, content)
     end
 
     def commit_all(message)
-      return if `git -C #{@path} status --porcelain`.strip.empty?
+      return if git_output("status", "--porcelain").strip.empty?
 
       git("add", ".")
       git("commit", "-m", message)
@@ -69,22 +69,36 @@ def clone_repo
     end
 
     def create_branch
-      existing = `git -C #{@path} branch --list #{new_branch_name}`.strip
-      if existing.empty?
-        git("checkout", "-b", new_branch_name)
-      else
+      if branch_exists?(new_branch_name)
         git("checkout", new_branch_name)
+      else
+        git("checkout", "-b", new_branch_name)
       end
     end
 
     def push_branch
       git("push", "--set-upstream", "origin", new_branch_name)
     end
 
+    def branch_exists?(name)
+      git_output("branch", "--list", name).strip.present?
+    end
+
     def git(*args)
+      system(*git_command(*args), exception: true)
+    end
+
+    def git_output(*args)
+      stdout, status = Open3.capture2(*git_command(*args))
+      raise "git #{args.first} failed" unless status.success?
+
+      stdout
+    end
+
+    def git_command(*args)
       git_dir = File.join(@path, '.git')
       work_tree = @path
-      system("git", "--git-dir=#{git_dir}", "--work-tree=#{work_tree}", *args, exception: true)
+      ["git", "--git-dir=#{git_dir}", "--work-tree=#{work_tree}", *args]
     end
 
     def repo_url

app/controllers/settings/github_syncer_controller.rb

@@ -19,9 +19,18 @@ def update
       %w[
         enabled
       ]
-    current_user.github_solution_syncer.update!(
-      params.require(:github_solution_syncer).permit(permitted)
-    )
+    syncer = current_user.github_solution_syncer
+    updated = Exercism.without_bullet do
+      syncer.update(params.require(:github_solution_syncer).permit(permitted))
+    end
+    return if updated
+
+    render json: {
+      error: {
+        type: :invalid_github_solution_syncer,
+        message: syncer.errors.full_messages.to_sentence
+      }
+    }, status: :bad_request
   end
 
   def sync_everything

app/models/user/github_solution_syncer.rb

@@ -2,6 +2,7 @@ class User::GithubSolutionSyncer < ApplicationRecord
   belongs_to :user
 
   validates :installation_id, :repo_full_name, presence: true
+  validate :main_branch_name_is_valid, if: :main_branch_name_changed?
 
   enum processing_method: { commit: 1, pull_request: 2 }
 
@@ -17,6 +18,28 @@ def processing_method = super.to_sym
   def commit_to_main? = processing_method == :commit
   def create_pr? = processing_method == :pull_request
 
+  # This follows the rules of git check-ref-format, limited to
+  # a conservative set of characters and a sensible length.
+  def self.valid_branch_name?(name)
+    return false unless name.is_a?(String)
+    return false unless name.length.between?(1, MAX_BRANCH_NAME_LENGTH)
+    return false unless name.match?(BRANCH_NAME_CHARACTERS)
+    return false if name.start_with?("-", "/")
+    return false if name.end_with?("/", ".")
+    return false if name.include?("..") || name.include?("//")
+
+    name.split("/").none? { |part| part.start_with?(".") || part.end_with?(".lock") }
+  end
+
   DEFAULT_COMMIT_MESSAGE_TEMPLATE = "[Sync $sync_object] $track_slug/$exercise_slug/$iteration_idx".freeze
   DEFAULT_PATH_TEMPLATE = "solutions/$track_slug/$exercise_slug/$iteration_idx".freeze
+  MAX_BRANCH_NAME_LENGTH = 100
+  BRANCH_NAME_CHARACTERS = %r{\A[A-Za-z0-9._/-]+\z}
+
+  private
+  def main_branch_name_is_valid
+    return if self.class.valid_branch_name?(main_branch_name)
+
+    errors.add(:main_branch_name, :invalid)
+  end
 end

test/commands/user/github_solution_syncer/local_git_repo_test.rb

@@ -0,0 +1,79 @@
+require "test_helper"
+
+class User::GithubSolutionSyncer
+  class LocalGitRepoTest < ActiveSupport::TestCase
+    test "create_branch creates a new branch" do
+      with_local_repo("feature/new") do |repo, dir|
+        repo.send(:create_branch)
+
+        assert_equal "feature/new", current_branch(dir)
+      end
+    end
+
+    test "create_branch checks out a branch that already exists" do
+      with_local_repo("existing") do |repo, dir|
+        run_git(dir, "branch", "existing")
+        run_git(dir, "commit", "--allow-empty", "-m", "Second commit")
+
+        repo.send(:create_branch)
+
+        assert_equal "existing", current_branch(dir)
+        assert_equal "Initial commit", last_commit_message(dir)
+      end
+    end
+
+    test "create_branch treats the branch name as a literal argument" do
+      name = "odd name $(touch marker)"
+      with_local_repo(name) do |repo, dir|
+        assert_raises(RuntimeError) { repo.send(:create_branch) }
+
+        refute File.exist?(File.join(dir, "marker"))
+        refute File.exist?(Rails.root.join("marker"))
+      end
+    end
+
+    test "commit_all commits changed files" do
+      with_local_repo("main") do |repo, dir|
+        repo.write_file("solutions/ruby/bob/1/bob.rb", "class Bob\nend\n")
+        repo.commit_all("Sync Bob")
+
+        assert_equal "Sync Bob", last_commit_message(dir)
+        assert_empty run_git(dir, "status", "--porcelain")
+      end
+    end
+
+    test "commit_all does nothing when there are no changes" do
+      with_local_repo("main") do |repo, dir|
+        repo.commit_all("Nothing to sync")
+
+        assert_equal "Initial commit", last_commit_message(dir)
+      end
+    end
+
+    private
+    def with_local_repo(new_branch_name)
+      syncer = build(:user_github_solution_syncer)
+      repo = LocalGitRepo.new(syncer, "main", new_branch_name, token: "token")
+
+      Dir.mktmpdir do |dir|
+        run_git(dir, "init", "--initial-branch=main")
+        run_git(dir, "config", "user.name", "Test")
+        run_git(dir, "config", "user.email", "test@example.com")
+        run_git(dir, "commit", "--allow-empty", "-m", "Initial commit")
+
+        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?
+
+      stdout
+    end
+
+    def current_branch(dir) = run_git(dir, "rev-parse", "--abbrev-ref", "HEAD").strip
+    def last_commit_message(dir) = run_git(dir, "log", "-1", "--format=%s").strip
+  end
+end

test/controllers/settings/github_syncer_controller_test.rb

@@ -0,0 +1,37 @@
+require "test_helper"
+
+class Settings::GithubSyncerControllerTest < ActionDispatch::IntegrationTest
+  test "update saves a valid branch name" do
+    user = create :user, :insider
+    syncer = create(:user_github_solution_syncer, user:)
+    sign_in!(user)
+
+    patch settings_github_syncer_path,
+      params: { github_solution_syncer: { processing_method: "commit", main_branch_name: "trunk" } },
+      as: :json
+
+    assert_response :success
+    syncer.reload
+    assert_equal "trunk", syncer.main_branch_name
+    assert_equal :commit, syncer.processing_method
+  end
+
+  test "update returns an error for an invalid branch name" do
+    user = create :user, :insider
+    syncer = create(:user_github_solution_syncer, user:)
+    sign_in!(user)
+
+    patch settings_github_syncer_path,
+      params: { github_solution_syncer: { processing_method: "commit", main_branch_name: "my branch" } },
+      as: :json
+
+    assert_response :bad_request
+    error = response.parsed_body["error"]
+    assert_equal "invalid_github_solution_syncer", error["type"]
+    assert_equal "Main branch name is invalid", error["message"]
+
+    syncer.reload
+    assert_equal "main", syncer.main_branch_name
+    assert_equal :pull_request, syncer.processing_method
+  end
+end

test/models/user/github_solution_syncer_test.rb

@@ -1,7 +1,63 @@
 require "test_helper"
 
 class User::GithubSolutionSyncerTest < ActiveSupport::TestCase
-  # test "the truth" do
-  #   assert true
-  # end
+  test "accepts typical branch names" do
+    syncer = create(:user_github_solution_syncer)
+
+    %w[main master trunk develop release/v1.2 feature/my_branch iHiD-solutions a].each do |name|
+      syncer.main_branch_name = name
+      assert syncer.valid?, "Expected #{name.inspect} to be valid"
+    end
+  end
+
+  test "rejects branch names git would refuse or that we don't support" do
+    syncer = create(:user_github_solution_syncer)
+
+    [
+      "",
+      " ",
+      "my branch",
+      "-main",
+      "/main",
+      "main/",
+      "main.",
+      ".main",
+      "feature/.main",
+      "main..dev",
+      "feature//main",
+      "main.lock",
+      "feature.lock/main",
+      "main~1",
+      "main^",
+      "main:dev",
+      "main?",
+      "main*",
+      "main[1]",
+      "main\\dev",
+      "main@{1}",
+      "main;dev",
+      "main$dev",
+      "main`dev`",
+      "main|dev",
+      "main&dev",
+      "main\ndev",
+      "mäin",
+      "a" * 101
+    ].each do |name|
+      syncer.main_branch_name = name
+      refute syncer.valid?, "Expected #{name.inspect} to be invalid"
+      assert_includes syncer.errors[:main_branch_name], "is invalid"
+    end
+  end
+
+  test "accepts a branch name at the maximum length" do
+    syncer = create(:user_github_solution_syncer)
+    syncer.main_branch_name = "a" * 100
+
+    assert syncer.valid?
+  end
+
+  test "defaults to main" do
+    assert_equal "main", create(:user_github_solution_syncer).main_branch_name
+  end
 end