From c401cb6bb4511ea3846ef96bcf4313c5efc8f21f Mon Sep 17 00:00:00 2001 From: Jeremy Walker Date: Tue, 27 Oct 2026 13:46:00 +0900 Subject: [PATCH] Validate GitHub syncer branch names --- .../github_solution_syncer/local_git_repo.rb | 26 ++++-- .../settings/github_syncer_controller.rb | 15 +++- app/models/user/github_solution_syncer.rb | 23 ++++++ .../local_git_repo_test.rb | 79 +++++++++++++++++++ .../settings/github_syncer_controller_test.rb | 37 +++++++++ .../user/github_solution_syncer_test.rb | 62 ++++++++++++++- 6 files changed, 230 insertions(+), 12 deletions(-) create mode 100644 test/commands/user/github_solution_syncer/local_git_repo_test.rb create mode 100644 test/controllers/settings/github_syncer_controller_test.rb 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..870270dc54 100644 --- a/app/commands/user/github_solution_syncer/local_git_repo.rb +++ b/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,11 +69,10 @@ 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 @@ -81,10 +80,25 @@ 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 diff --git a/app/controllers/settings/github_syncer_controller.rb b/app/controllers/settings/github_syncer_controller.rb index b40b74d32d..c9a9eb6a16 100644 --- a/app/controllers/settings/github_syncer_controller.rb +++ b/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 diff --git a/app/models/user/github_solution_syncer.rb b/app/models/user/github_solution_syncer.rb index 7ad25264c0..1ca526f296 100644 --- a/app/models/user/github_solution_syncer.rb +++ b/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 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..8aedaac1c5 --- /dev/null +++ b/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 diff --git a/test/controllers/settings/github_syncer_controller_test.rb b/test/controllers/settings/github_syncer_controller_test.rb new file mode 100644 index 0000000000..3fe583a345 --- /dev/null +++ b/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 diff --git a/test/models/user/github_solution_syncer_test.rb b/test/models/user/github_solution_syncer_test.rb index 14d4d49bda..de31e23a20 100644 --- a/test/models/user/github_solution_syncer_test.rb +++ b/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