DEV Community

Pavan Kumar Sadashiv
Pavan Kumar Sadashiv

Posted on Edited on

Part 3: Supporting Non-ASCII Branch Names in GitLab Workspaces — AppSec CWE-78 & CWE-116 Deep Dive

#gitlab #security #ruby #devsecops

Series Context: Part 3 of 10 in GitLab Core Engineering: Architecture, MCP & CI/CD Deep Dives.

Featured MR: gitlab-org/gitlab!262103 · Work Item: #596326

The Bug: Why UTF-8 Branches Failed to Clone in GitLab Workspaces

GitLab's Gitlab::GitRefValidator supports valid UTF-8 branch names such as feature/günter-überarbeitung or ветка/тест-機能. However, provisioning a GitLab Workspace on those branches failed during post-start cloning inside the container.

The culprit lived in ee/lib/remote_development/workspace_operations/create/internal_poststart_commands_inserter.rb:

format(
  INTERNAL_POSTSTART_COMMAND_CLONE_PROJECT_SCRIPT,
  project_cloning_successful_file: Shellwords.shellescape(project_cloning_successful_file),
  clone_dir: Shellwords.shellescape(clone_dir),
  project_ref: Shellwords.shellescape(project_ref),
  project_url: Shellwords.shellescape(project_url),
  clone_depth_option: clone_depth_option
)
Enter fullscreen mode Exit fullscreen mode

Look closely at what Ruby's stdlib Shellwords.shellescape actually does. Internally, Ruby's shellescape runs:

str.gsub!(/[^A-Za-z0-9_\-.,:+\/@\n]/, "\\\\\\&")
Enter fullscreen mode Exit fullscreen mode

Notice what isn't in that allowlist: any multibyte / non-ASCII UTF-8 character (ü, Cyrillic, CJK, accents). On a byte-by-byte or non-ASCII basis, Shellwords.shellescape("feature/günter") outputs backslash-escaped characters (feature/g\ünter). Even subtle shell locale differences inside minimal Alpine/Debian workspace containers, combined with git ref handling and template construction, led to mangled refs and broken workspace clones.

Meanwhile, project_ref is user-controlled input interpolated directly into a shell command template (git clone ... --branch %<project_ref>s ...). Any escaping change here sits squarely on a command injection (CWE-78) trust boundary—meaning AppSec scrutiny is mandatory.

Designing shellescape_ref: Minimally Diffable Against Ruby Stdlib

During security review with @erran and @cbonk, we converged on a crucial principle for security-sensitive sanitization helpers: do not invent a clever custom regex from scratch if an AppSec reviewer has to re-prove its safety. Instead, copy Ruby stdlib's Shellwords.shellescape verbatim and change only the single POSIX character class needed to pass non-ASCII characters through:

# Copy of Shellwords.shellescape leaving non-ASCII characters unescaped for git clone --branch.
# Differs only by [:^ascii:] from stdlib:
# https://github.com/ruby/ruby/blob/2b0b7728dc7f0561c35c3d8c4489945c94b783ad/lib/shellwords.rb#L158-L179
#
# WARNING: Output is only safe in an unquoted shell position (e.g. `--branch %<project_ref>s`).
# Quoting it in a template preserves literal backslashes and breaks non-safe branch names.
#
# @see https://gitlab.com/gitlab-org/gitlab/-/work_items/596326
# @param [String] str
# @return [String]
def self.shellescape_ref(str)
  str = str.to_s

  return +"''" if str.empty?

  raise ArgumentError, "NUL character" if str.index("\0")

  str = str.dup

  str.gsub!(%r{[^A-Za-z0-9_\-.,:+/@\n[:^ascii:]]}, "\\\\\\&")

  str.gsub!(/\n/, "'\n'")

  str
end
Enter fullscreen mode Exit fullscreen mode

Why [:^ascii:] (and Not [:ascii:]) Inside [^...]?

At one point in review, an AppSec discussion asked whether [:ascii:] vs [:^ascii:] had a negation typo. Let's trace Ruby's Onigmo character class math carefully because getting this backwards is catastrophic:

  • We want to match and escape any character that is:
    • NOT an ASCII safe character (A-Za-z0-9_-.,:+/@\n), AND
    • NOT a non-ASCII character ([:^ascii:]).
  • Because the outer class is a negated set [^ ... ], listing [:^ascii:] inside [^ ... ] means "do NOT match non-ASCII characters"—i.e., leave non-ASCII characters untouched, while matching and backslash-escaping every ASCII metacharacter (;, &, |, `, $, (, ), ', ", spaces, tabs, >, <, \).
  • Conversely, putting [:ascii:] inside [^...] would mean "do NOT match ASCII"—which would leave ; and $() completely unescaped! Always verify double negations in character classes with executable round-trip tests.

Every Detail Preserved From Stdlib

  • Empty string -> "''": Ensures an empty branch doesn't collapse --branch <url> into consuming the URL as the branch argument.
  • NUL byte guard (\0): Raises ArgumentError, "NUL character" immediately (identical to Ruby 3.x Shellwords.shellescape).
  • Newlines -> "'\n'": Prevents backslash-newline line continuation inside /bin/sh.
  • Unquoted-position warning: Backslash escaping only works when interpolated unquoted (--branch %<project_ref>s). Inside '...' or "...", backslashes become literal characters in the branch name!

Two AppSec Findings That Made the Fix Stronger

After @erran performed an AppSec triage review, two concrete findings surfaced under CWE-78 and CWE-116—and neither had been obvious before review!

1. CWE-78 Even Inside an RSpec Spec File!

Earlier in the PR, to prove that POSIX /bin/sh actually parsed the escaped --branch string back into the exact original UTF-8 project_ref, the spec executed:

# Flagged by AppSec as CWE-78 (OS Command Injection) in test code
output, status = Open3.capture2("sh", "-c", "printf '%s' #{escaped_ref}")
Enter fullscreen mode Exit fullscreen mode

Why did AppSec flag Open3.capture2("sh", "-c", ...) even inside a test (_spec.rb)?

  1. Test suites are scanned by SAST rules (CWE-78: Improper Neutralization of Special Elements used in an OS Command)—and if a regression ever broke shellescape_ref, running the spec would actually execute injected shell commands on CI runners!
  2. Spawning subprocesses in a fast_spec_helper unit test slows specs down unnecessarily.

The fix: remove require "open3" and process spawning completely, and verify the exact contract deterministically in Ruby using Shellwords.shellsplit:

it "matches Shellwords.shellescape except for non-ASCII characters and round-trips via Shellwords.shellsplit" do
  escaped_url = Regexp.escape(Shellwords.shellescape(project.http_url_to_repo))
  escaped_ref = clone_command[:exec][:commandLine][/--branch (.*) "#{escaped_url}"/m, 1]

  expected_escaped = if project_ref.empty?
                       "''"
                     else
                       project_ref.each_char.map do |char|
                         if char.ascii_only?
                           char == "\n" ? "'\n'" : Shellwords.shellescape(char)
                         else
                           char
                         end
                       end.join
                     end

  expect(escaped_ref).to eq(expected_escaped)
  expect(Shellwords.shellsplit(escaped_ref)).to eq([project_ref])
end
Enter fullscreen mode Exit fullscreen mode

That single parameterized block across 10 adversarial inputs ("", "feature/günter-überarbeitung", "ветка/тест-機能", "feature/\\ü", newlines, ;&|><, $(whoami), `id`, quotes, tabs) proves both character-level equivalence and shell token round-trip safety without ever forking /bin/sh.

2. CWE-116 Explicit Security Review Comment

On security-reviewed functions in InternalPoststartCommandsInserter, GitLab documents every interpolated template parameter in a # SECURITY REVIEWED: header block. We updated the comment to explicitly state both the escaping rule and its template invariant:

  • project_ref escapes ASCII outside A-Za-z0-9_-.,:+/@\n, passes non-ASCII through, and stays unquoted in INTERNAL_POSTSTART_COMMAND_CLONE_PROJECT_SCRIPT.

Following commit ff1827c321, AppSec (@erran) applied ~"security-review::completed" and ~"AppSecWorkflow::complete" with a green CI pipeline (#3053234994).

Takeaways for GitLab Engineers

  1. Diff against stdlib: When adapting Shellwords (or any security sanitizer), match stdlib structure line-for-line so security reviewers can audit the exact delta in seconds.
  2. Never spawn sh -c in specs to test escaping: Use pure-Ruby parser round-trips (Shellwords.shellsplit) so SAST scanners stay clean and tests never execute broken inputs.
  3. Document template-context invariants: Escaping without documenting unquoted vs single-quoted position invites someone later to add quotes "%<project_ref>s" and break escaping silently.

Series Navigation

Top comments (0)