fix(security): reject leading-dash git refs, triage remaining Brakeman findings

index 536b557..5b63a8d 100644 --- a/app/services/git_repository_service.rb +++ b/app/services/git_repository_service.rb @@ -8,6 +8,17 @@ class GitRepositoryService File.join(REPOS_BASE, username, "#{reponame}.git") end + # Git ref/SHA values reach `git` as a single argv entry (Open3.capture3 never + # invokes a shell), so they can't carry shell metacharacters. But a value + # starting with "-" is still parsed by git itself as an option rather than a + # revision (e.g. a branch of "--output=/some/path" could make `git log` write + # to an arbitrary file) — real ref/SHA names can never start with "-" (see + # `git check-ref-format`), so reject anything that does before it reaches git. + def self.safe_rev?(rev) + rev.present? && !rev.start_with?("-") + end + private_class_method :safe_rev? + def self.create_bare_repo(username, reponame) path = repo_path(username, reponame) FileUtils.mkdir_p(path) @@ -38,6 +49,7 @@ class GitRepositoryService end def self.list_tree(path, branch, tree_path = "") + return [] unless safe_rev?(branch) prefix = tree_path.blank? ? "" : "#{tree_path.chomp("/")}/" args = [ "git", "--git-dir", path, "ls-tree", "--long", "#{branch}:#{prefix}" ] out, _err, status = Open3.capture3(*args) @@ -82,6 +94,7 @@ class GitRepositoryService end def self.file_content(path, branch, file_path) + return nil unless safe_rev?(branch) out, _err, status = Open3.capture3("git", "--git-dir", path, "show", "#{branch}:#{file_path}") return nil unless status.success? out @@ -90,6 +103,7 @@ class GitRepositoryService # Object id of the blob at <ref>:<file_path>. Stable for identical content, so # it's the natural cache key for rendered/highlighted output. nil if missing. def self.blob_sha(path, branch, file_path) + return nil unless safe_rev?(branch) out, _err, status = Open3.capture3("git", "--git-dir", path, "rev-parse", "--verify", "--quiet", "#{branch}:#{file_path}") status.success? ? out.strip.presence : nil end @@ -97,6 +111,7 @@ class GitRepositoryService # Byte size of the blob without loading it into memory — lets the blob view # decide up front whether a file is too large to highlight or render. def self.blob_size(path, branch, file_path) + return nil unless safe_rev?(branch) out, _err, status = Open3.capture3("git", "--git-dir", path, "cat-file", "-s", "#{branch}:#{file_path}") status.success? ? out.strip.to_i : nil end @@ -104,11 +119,13 @@ class GitRepositoryService # Full commit SHA a ref currently points at. Permalinks pin to this so they # survive the branch moving on. def self.commit_sha(path, ref) + return nil unless safe_rev?(ref) out, _err, status = Open3.capture3("git", "--git-dir", path, "rev-parse", "--verify", "--quiet", "#{ref}^{commit}") status.success? ? out.strip.presence : nil end def self.commits(path, branch, limit: 20, offset: 0) + return [] unless safe_rev?(branch) format = "%H%x00%h%x00%s%x00%an%x00%ae%x00%ad%x00%cn" out, _err, status = Open3.capture3( "git", "--git-dir", path, "log", @@ -137,6 +154,8 @@ class GitRepositoryService # %B contains embedded newlines, so we cannot rely on splitting by "\n" to # find the end of the format output. Instead we append a sentinel that git # will never emit naturally and split on that. + return nil unless safe_rev?(sha) + sentinel = "SIGIT-EOH" format = "%H%x00%h%x00%s%x00%B%x00%an%x00%ae%x00%ad%x00%T%x00#{sentinel}" out, _err, status = Open3.capture3( @@ -177,6 +196,7 @@ class GitRepositoryService # files are skipped. Returns [] when nothing matches or the branch is unknown. def self.search_code(path, branch, query, limit: 20) return [] if query.to_s.empty? + return [] unless safe_rev?(branch) out, _err, status = Open3.capture3( "git", "--git-dir", path, "grep", @@ -199,6 +219,7 @@ class GitRepositoryService end def self.branch_exists?(path, branch) + return false unless safe_rev?(branch) _out, _err, status = Open3.capture3("git", "--git-dir", path, "rev-parse", "--verify", branch) status.success? end @@ -210,6 +231,7 @@ class GitRepositoryService end def self.commit_count(path, branch) + return 0 unless safe_rev?(branch) out, _err, status = Open3.capture3("git", "--git-dir", path, "rev-list", "--count", branch) return 0 unless status.success? out.strip.to_i @@ -219,6 +241,7 @@ class GitRepositoryService # content version when caching derived artifacts (e.g. Open Graph cards) so # they regenerate exactly when the repository content changes. def self.head_sha(path, branch) + return nil unless safe_rev?(branch) out, _err, status = Open3.capture3("git", "--git-dir", path, "rev-parse", "--verify", "#{branch}^{commit}") status.success? ? out.strip.presence : nil end @@ -226,6 +249,7 @@ class GitRepositoryService # Flat list of every tracked file path on +branch+. One `ls-tree -r` call; # used for cheap primary-language detection. def self.tree_filenames(path, branch) + return [] unless safe_rev?(branch) out, _err, status = Open3.capture3( "git", "--git-dir", path, "ls-tree", "-r", "--name-only", branch )

index 1472c83..ae0aa8c 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -7,8 +7,8 @@ "check_name": "Execute", "message": "Possible command injection", "file": "app/services/git_repository_service.rb", - "line": 93, - "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref/path is passed to git as a single literal argv entry and cannot inject shell commands." + "line": 107, + "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref/path is passed to git as a single literal argv entry and cannot inject shell commands. GitRepositoryService.safe_rev? also rejects any ref/SHA starting with \"-\" before it reaches git, closing the git-argument-injection surface (e.g. a branch of \"--output=...\")." }, { "warning_type": "Command Injection", @@ -17,8 +17,8 @@ "check_name": "Execute", "message": "Possible command injection", "file": "app/services/git_repository_service.rb", - "line": 100, - "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref/path is passed to git as a single literal argv entry and cannot inject shell commands." + "line": 115, + "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref/path is passed to git as a single literal argv entry and cannot inject shell commands. GitRepositoryService.safe_rev? also rejects any ref/SHA starting with \"-\" before it reaches git, closing the git-argument-injection surface (e.g. a branch of \"--output=...\")." }, { "warning_type": "Command Injection", @@ -27,8 +27,78 @@ "check_name": "Execute", "message": "Possible command injection", "file": "app/services/git_repository_service.rb", - "line": 107, - "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref is passed to git as a single literal argv entry and cannot inject shell commands." + "line": 123, + "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref is passed to git as a single literal argv entry and cannot inject shell commands. GitRepositoryService.safe_rev? also rejects any ref/SHA starting with \"-\" before it reaches git, closing the git-argument-injection surface (e.g. a branch of \"--output=...\")." + }, + { + "warning_type": "Command Injection", + "warning_code": 14, + "fingerprint": "fc944c7823b102c74e5d4bfd30183d1f2776b80f00b453e05571815b2fbc26f1", + "check_name": "Execute", + "message": "Possible command injection", + "file": "app/services/git_repository_service.rb", + "line": 55, + "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref/path is passed to git as a single literal argv entry and cannot inject shell commands. GitRepositoryService.safe_rev? also rejects any ref/SHA starting with \"-\" before it reaches git, closing the git-argument-injection surface (e.g. a branch of \"--output=...\")." + }, + { + "warning_type": "Command Injection", + "warning_code": 14, + "fingerprint": "d4ffd91b7a130c0d1e51bc90c8b66c51242d4ec7f21201b3acab11f7a3fb7b6b", + "check_name": "Execute", + "message": "Possible command injection", + "file": "app/services/git_repository_service.rb", + "line": 98, + "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref/path is passed to git as a single literal argv entry and cannot inject shell commands. GitRepositoryService.safe_rev? also rejects any ref/SHA starting with \"-\" before it reaches git, closing the git-argument-injection surface (e.g. a branch of \"--output=...\")." + }, + { + "warning_type": "Command Injection", + "warning_code": 14, + "fingerprint": "c932f5558ef2885051eeba4c72f436d871323489099c7361cb75f3f3e4d2f155", + "check_name": "Execute", + "message": "Possible command injection", + "file": "app/services/git_repository_service.rb", + "line": 133, + "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref is passed to git as a single literal argv entry and cannot inject shell commands. GitRepositoryService.safe_rev? also rejects any ref/SHA starting with \"-\" before it reaches git, closing the git-argument-injection surface (e.g. a branch of \"--output=...\")." + }, + { + "warning_type": "Command Injection", + "warning_code": 14, + "fingerprint": "d802f010326572ecab89535d3c449ac7b6bc9b18b3568197eb302d6875d42c58", + "check_name": "Execute", + "message": "Possible command injection", + "file": "app/services/git_repository_service.rb", + "line": 245, + "note": "False positive: Open3.capture3 is called with a separate argument list (no shell), so the interpolated ref is passed to git as a single literal argv entry and cannot inject shell commands. GitRepositoryService.safe_rev? also rejects any ref/SHA starting with \"-\" before it reaches git, closing the git-argument-injection surface (e.g. a branch of \"--output=...\")." + }, + { + "warning_type": "Command Injection", + "warning_code": 14, + "fingerprint": "4aff271d89cea00ac9c30356cfc76de7710b2279191dfa8cd3b682cb748cb405", + "check_name": "Execute", + "message": "Possible command injection", + "file": "app/controllers/api/v1/repos_controller.rb", + "line": 42, + "note": "False positive: default_branch is always the literal string \"main\" set a few lines above (never derived from request params), so nothing user-controlled reaches this Open3.capture3 call." + }, + { + "warning_type": "Mass Assignment", + "warning_code": 105, + "fingerprint": "faef13fd7ff08bed1e6f8739746e5d6af3b6ec0515cb4bc73d856d973d1140b2", + "check_name": "PermitAttributes", + "message": "Potentially dangerous key allowed for mass assignment", + "file": "app/controllers/api/v1/cloud_sessions_controller.rb", + "line": 75, + "note": "False positive: `role` here is CloudMessage#role, a chat-message role (\"user\"/\"assistant\"/\"system\") restricted by `validates :role, inclusion: { in: ROLES }` in the model — not a user/account privilege field. It's passed as an explicit keyword arg to append_message!, not mass-assigned to a model." + }, + { + "warning_type": "Cross-Site Scripting", + "warning_code": 4, + "fingerprint": "8ef2373ad74f844d863ba6076d392d7b797459ca89a3b30d19137c87a0662fee", + "check_name": "LinkToHref", + "message": "Potentially unsafe model attribute in `link_to` href", + "file": "app/views/admin/repositories/show.html.erb", + "line": 15, + "note": "False positive: Repository#html_url is built from ENV[\"SIGITSI_URL\"] (trusted, admin-set) plus the owning user's username and the repo name, both restricted by model format validations to safe charsets (letters, digits, hyphen/underscore/dot) — no URL scheme (e.g. javascript:) can appear in this value." } ], "brakeman_version": "8.0.5"