From c57c35cd6be477593fc8165fe07a2a961212b54c Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 15 Sep 2026 13:04:08 -0500 Subject: [PATCH 1/3] Harden YARD doc parsing against .yardopts RCE CodeTriage runs YARD against untrusted, user-submitted repositories to build documentation. YARD reads a repo's `.yardopts` file by default, and options like `-e/--load` (Kernel#load), `--query` (instance_eval), and a custom `--template-path` execute code shipped in the repo -- the same class of remote code execution abused against RubyDoc.info. Enable YARD safe mode (which disables those code-execution features) and set `use_yardopts_file = false` so the attacker-controlled `.yardopts` is never read at all. YARD's parser is Ripper-based and static, so disabling these opt-in exec features closes the RCE without affecting doc extraction. --- lib/docs_doctor/parsers/ruby/yard.rb | 11 ++++++++++ test/jobs/parse_docs_test.rb | 31 ++++++++++++++++++++++++++++ test/test_helper.rb | 2 ++ 3 files changed, 44 insertions(+) diff --git a/lib/docs_doctor/parsers/ruby/yard.rb b/lib/docs_doctor/parsers/ruby/yard.rb index 0143e92e1..589189e44 100644 --- a/lib/docs_doctor/parsers/ruby/yard.rb +++ b/lib/docs_doctor/parsers/ruby/yard.rb @@ -86,7 +86,18 @@ def hash_for_entity(obj, repo) def process(exclude = DEFAULT_EXCLUDE) require "yard" + + # We run YARD against untrusted, user-submitted repositories, so it + # must never execute code that ships in the repo. Safe mode disables + # YARD's code-execution features (`-e/--load`, `--query`, custom + # template paths), and ignoring the repo's `.yardopts` file stops YARD + # from reading attacker-controlled options at all. Without this a repo + # could gain remote code execution via a malicious `.yardopts` + # (the same class of RCE abused against RubyDoc.info). + YARD::Config.options[:safe_mode] = true + yard = YARD::CLI::Yardoc.new + yard.use_yardopts_file = false # yard.files = files yard.excluded = exclude # http://rubydoc.org/gems/yard/YARD/Parser/SourceParser#parse-class_method diff --git a/test/jobs/parse_docs_test.rb b/test/jobs/parse_docs_test.rb index 1a0693a70..a733b0d0b 100644 --- a/test/jobs/parse_docs_test.rb +++ b/test/jobs/parse_docs_test.rb @@ -34,4 +34,35 @@ class ParseDocsTest < ActiveJob::TestCase ).in_fork { raise "foo" } end end + + test "process enables YARD safe mode" do + require "yard" + YARD::Config.options[:safe_mode] = false + + Dir.mktmpdir do |repo_dir| + FileUtils.mkdir_p(File.join(repo_dir, "lib")) + File.write(File.join(repo_dir, "lib", "thing.rb"), "class Thing\n def hello\n end\nend\n") + + DocsDoctor::Parsers::Ruby::Yard.new(repo_dir).process + + assert YARD::Config.options[:safe_mode], + "Expected process to run YARD in safe mode" + end + end + + test "process ignores the repo's .yardopts and still parses methods" do + Dir.mktmpdir do |repo_dir| + # If this .yardopts were honored, `--exclude lib` would drop lib/thing.rb + # from parsing and Thing#hello would be missing. + File.write(File.join(repo_dir, ".yardopts"), "--exclude lib\n") + FileUtils.mkdir_p(File.join(repo_dir, "lib")) + File.write(File.join(repo_dir, "lib", "thing.rb"), "class Thing\n def hello\n end\nend\n") + + parser = DocsDoctor::Parsers::Ruby::Yard.new(repo_dir) + parser.process + + assert(parser.yard_objects.any? { |o| o.respond_to?(:path) && o.path == "Thing#hello" }, + "Expected the repo's .yardopts to be ignored so Thing#hello is still parsed") + end + end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 0f3fd785b..ee8cd92e7 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -8,6 +8,8 @@ require "rails/test_help" require "capybara/rails" require "webmock/minitest" +require "tmpdir" +require "fileutils" class ActiveSupport::TestCase # Setup all fixtures in test/fixtures/*.(yml|csv) for all tests in alphabetical order. From 3e77748b65c3e89dbe854ebe7428ac28a9992807 Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 15 Sep 2026 13:04:40 -0500 Subject: [PATCH 2/3] Harden git clone of untrusted repos GithubFetcher::Repo#clone interpolated a repo's clone URL straight into a shell (`cd #{dir} && git clone #{clone_url}`), so a crafted URL could run arbitrary shell commands, and git's own transports (file://, ext::...) could be abused to reach the local filesystem or execute commands. - Allow-list the URL: only https on host github.com (safe_clone_url?), and refuse anything else before running git (UnsafeCloneUrlError). - Run git via Open3 array form (no shell) and put the URL after `--` so it can't be parsed as a git option. - GIT_TERMINAL_PROMPT=0 avoids credential-prompt hangs; --depth 1 --single-branch --no-tags keep the checkout small. --- app/models/github_fetcher/repo.rb | 46 +++++++++++++++++++++++++-- test/unit/github_fetcher/repo_test.rb | 35 ++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/app/models/github_fetcher/repo.rb b/app/models/github_fetcher/repo.rb index 6394a2579..0bd98dd3f 100644 --- a/app/models/github_fetcher/repo.rb +++ b/app/models/github_fetcher/repo.rb @@ -1,7 +1,23 @@ # frozen_string_literal: true +require "open3" + module GithubFetcher class Repo < Resource + UnsafeCloneUrlError = Class.new(StandardError) + + # Only clone from https://github.com. This rejects git's dangerous + # transports (file://, ssh://, ext::…) and any host other than GitHub, in + # case the API response is unexpected or has been tampered with. + def self.safe_clone_url?(url) + return false unless url.is_a?(String) + + uri = URI.parse(url) + uri.scheme == "https" && uri.host == "github.com" + rescue URI::InvalidURIError + false + end + def initialize(options) @api_path = File.join( "repos", @@ -18,13 +34,39 @@ def default_branch # TODO - does this really belong here? Seems like it (and Repo#populate_docs!) # should move into the PopulateDocs job def clone - out = `cd #{dir} && git clone #{clone_url} 2>&1` - raise "Error executing git clone #{clone_url.inspect}: #{out.inspect}" unless $?.success? + url = clone_url + unless self.class.safe_clone_url?(url) + raise UnsafeCloneUrlError, "Refusing to clone unexpected URL: #{url.inspect}" + end + + # Array form runs git directly with no shell, so the URL can never be + # interpreted by /bin/sh. GIT_TERMINAL_PROMPT=0 keeps a bad URL from + # blocking on a credential prompt, and chdir lands the checkout in our + # scratch dir (git creates a subdirectory named after the repo). + out, status = Open3.capture2e( + {"GIT_TERMINAL_PROMPT" => "0"}, + *git_clone_argv(url), + chdir: dir + ) + raise "Error executing git clone #{url.inspect}: #{out.inspect}" unless status.success? dir end private + # Build the git argv as an array so it runs without a shell, and place the + # URL after `--` so a URL beginning with `-` can't be parsed as a git + # option. Shallow/single-branch/no-tags keep the checkout small. + def git_clone_argv(url) + [ + "git", "clone", + "--depth", "1", + "--single-branch", + "--no-tags", + "--", url + ] + end + # TODO - moves w/ clone def dir @dir ||= Dir.mktmpdir diff --git a/test/unit/github_fetcher/repo_test.rb b/test/unit/github_fetcher/repo_test.rb index 6ff18e071..5c13a222e 100644 --- a/test/unit/github_fetcher/repo_test.rb +++ b/test/unit/github_fetcher/repo_test.rb @@ -43,4 +43,39 @@ def fetcher(repo) assert_nil fetcher.default_branch end end + + test "#safe_clone_url? only allows https github.com URLs" do + assert GithubFetcher::Repo.safe_clone_url?("https://github.com/schneems/get_process_mem.git") + + refute GithubFetcher::Repo.safe_clone_url?("http://github.com/a/b.git") + refute GithubFetcher::Repo.safe_clone_url?("https://evil.com/a/b.git") + refute GithubFetcher::Repo.safe_clone_url?("https://github.com.evil.com/a/b.git") + refute GithubFetcher::Repo.safe_clone_url?("ssh://git@github.com/a/b.git") + refute GithubFetcher::Repo.safe_clone_url?("file:///etc/passwd") + refute GithubFetcher::Repo.safe_clone_url?("ext::sh -c id") + refute GithubFetcher::Repo.safe_clone_url?("--upload-pack=touch /tmp/pwn") + refute GithubFetcher::Repo.safe_clone_url?(nil) + end + + test "#clone refuses an unsafe clone_url before running git" do + fetcher = fetcher(repos(:scene_hub_v2)) + fetcher.stubs(:clone_url).returns("file:///tmp/not-a-real-repo-xyz") + + error = assert_raises(StandardError) { fetcher.clone } + assert_match(/refus/i, error.message) + end + + test "#git_clone_argv runs git without a shell and guards option injection" do + fetcher = fetcher(repos(:scene_hub_v2)) + url = "https://github.com/schneems/get_process_mem.git" + + argv = fetcher.send(:git_clone_argv, url) + + assert_equal "git", argv[0] + assert_equal "clone", argv[1] + assert_includes argv, "--depth" + separator_index = argv.index("--") + assert separator_index, "expected a -- separator so the URL cannot be parsed as a git option" + assert_equal url, argv[separator_index + 1] + end end From d970b64731be8adf959c946cb8ef28eb54f7490b Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 15 Sep 2026 13:05:26 -0500 Subject: [PATCH 3/3] Clean up the temp dir cloned for doc parsing GithubFetcher::Repo#cleanup existed but was private and never called, so every populate_docs! run leaked a Dir.mktmpdir clone -- a slow disk-fill. Make cleanup public and guard it on @dir so it only ever removes a dir the fetcher created, and call it from Repo#populate_docs! in an ensure block so the temp dir is removed on every exit path (success, a raising parse, or a failed clone). A caller-supplied `location:`, which we did not clone, is left untouched. --- app/models/github_fetcher/repo.rb | 5 ++--- app/models/repo.rb | 2 ++ test/unit/github_fetcher/repo_test.rb | 16 ++++++++++++++ test/unit/repo_test.rb | 32 +++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/app/models/github_fetcher/repo.rb b/app/models/github_fetcher/repo.rb index 0bd98dd3f..c22165f5b 100644 --- a/app/models/github_fetcher/repo.rb +++ b/app/models/github_fetcher/repo.rb @@ -77,9 +77,8 @@ def clone_url as_json["clone_url"] end - # TODO - this appears to be uncalled... remove? or do we need it and should use it? - def cleanup - FileUtils.remove_entry(dir) + public def cleanup + FileUtils.remove_entry(@dir) if @dir end end end diff --git a/app/models/repo.rb b/app/models/repo.rb index ca112d8ca..0dee287f4 100644 --- a/app/models/repo.rb +++ b/app/models/repo.rb @@ -74,6 +74,8 @@ def populate_docs!(commit_sha: commit_sha_fetcher.commit_sha, location: nil, has parser.process parser.store(self) :success + ensure + fetcher.cleanup end def background_populate_issues! diff --git a/test/unit/github_fetcher/repo_test.rb b/test/unit/github_fetcher/repo_test.rb index 5c13a222e..554987d0e 100644 --- a/test/unit/github_fetcher/repo_test.rb +++ b/test/unit/github_fetcher/repo_test.rb @@ -78,4 +78,20 @@ def fetcher(repo) assert separator_index, "expected a -- separator so the URL cannot be parsed as a git option" assert_equal url, argv[separator_index + 1] end + + test "#cleanup removes the temporary working directory it created" do + fetcher = fetcher(repos(:scene_hub_v2)) + dir = fetcher.send(:dir) # #clone checks the repo out into this scratch dir + assert Dir.exist?(dir) + + fetcher.cleanup + + refute Dir.exist?(dir), "expected the cloned scratch dir to be removed" + end + + test "#cleanup is a safe no-op when nothing was cloned" do + fetcher = fetcher(repos(:scene_hub_v2)) + + assert_nothing_raised { fetcher.cleanup } + end end diff --git a/test/unit/repo_test.rb b/test/unit/repo_test.rb index db34e3605..7a40bc376 100644 --- a/test/unit/repo_test.rb +++ b/test/unit/repo_test.rb @@ -113,4 +113,36 @@ class RepoTest < ActiveSupport::TestCase assert_not repos.include?(subscribed_repo) assert repos.include?(unsubscribed_repo) end + + test "#populate_docs! removes the working directory it cloned" do + repo = repos(:get_process_mem) + cloned_dir = nil + + repo.fetcher.define_singleton_method(:clone) do + dir = send(:dir) + cloned_dir = dir + FileUtils.mkdir_p(File.join(dir, "lib")) + File.write(File.join(dir, "lib", "thing.rb"), "class Thing\n def hello\n end\nend\n") + dir + end + + repo.populate_docs!(commit_sha: "abc123", has_subscribers: true) + + refute_nil cloned_dir + refute Dir.exist?(cloned_dir), + "expected populate_docs! to clean up the temp dir it cloned" + end + + test "#populate_docs! does not delete a caller-provided location" do + repo = repos(:get_process_mem) + location = Dir.mktmpdir + FileUtils.mkdir_p(File.join(location, "lib")) + File.write(File.join(location, "lib", "thing.rb"), "class Thing\n def hello\n end\nend\n") + + repo.populate_docs!(commit_sha: "abc123", location: location, has_subscribers: true) + + assert Dir.exist?(location), "a caller-provided location must not be deleted" + ensure + FileUtils.remove_entry(location) if location && Dir.exist?(location) + end end