DEV Community

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

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
)

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

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

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

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}")

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

  • 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!
  • 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][%r{--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

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

  • 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.
  • 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.
  • 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

  • Part 1: Taming EE::Project & Extracting Ai::Catalog (!269060)
  • Part 2: Decoupling EE::Group Routing via Ai::DuoSettings::Paths (!269059)
  • Part 3 (This Article): Escaping Non-ASCII Git Refs in Workspaces - AppSec CWE-78 & CWE-116 (!262103)
  • Part 4: Exposing Grape Routes to MCP - list_pipeline_schedules & Auth Traps (!264856)
  • Part 5: Vacuous RSpec Mocks & Live Upstream Flag Rebases (!269061)
  • Part 6: Sequential description_edits & Quick-Action Defense in save_work_item (!264862)
  • Part 7: Self-Correcting MCP Errors & JSON Schema Fixes (!264852, !264853, !264864)
  • Part 8: Consolidating SavePipelineService & Active Polling Hints (!264858, !264863)
  • Part 9: Surviving Dangerfile, Commit Trailers & Predictive CI Pipelines
  • Part 10: Security Cleanups, Organization empty? & Review Etiquette (!261178, !261030, !264859)
Read on DEV Community ↗ ← Back to News

Comments

No comments yet. Start the discussion.