Match the whole string in Gitlab::Utils.to_boolean

What does this MR do and why?

Gitlab::Utils.to_boolean matched its tokens with /^(true|t|yes|y|1|on)$/i and /^(false|f|no|n|0|off)$/i. In Ruby ^ and $ match at every line boundary, so a string with one line equal to a token converted as that token: "false\ntrue" and "garbage\ntrue" returned true, and "garbage\noff" returned false, instead of the default.

This MR anchors both patterns to the whole string with \A and \z, as the Ruby secure coding guidelines recommend. A value with a line break anywhere now returns the default.

\A and \Z is the alternative, if you prefer it. With \z, a value with one trailing newline, such as "true\n", also returns the default instead of converting as its token; \Z keeps that case as it was and changes only line breaks elsewhere. I checked every caller (below) and found none that gets a trailing newline from GitLab's own install methods, so I chose \z. Switching is a two-character change, plus moving the three trailing-newline rows of the specs ("true\n", "off\n", "1\n") to an example that expects their token.

!260362 (draft) copies the same two patterns into gems/gitlab-platform-core/lib/gitlab/instance_identity.rb, so that copy needs the same change. That gem is not on master yet, so this MR does not touch it.

Nine of the 13 new examples fail on master and all pass with this MR. The other four are values with a line break that already returned the default and must keep doing so.

Callers checked, and what I ran

The 309 calls outside specs on master read request and API parameters, environment variables (over 110 calls), CI/CD variables, integration properties and settings, values GitLab itself wrote to the database or Redis, HTTP headers and interactive prompts. What can carry a line break:

  • Files: no caller reads a file. The prompts in bin/feature-flag, bin/audit-event-type and ee/bin/custom-ability call Readline.readline(...)&.strip first.
  • Environment variables: Omnibus writes gitlab_rails['env'] with env_dir, and chpst -e sets each variable to the first line of its file, without the newline (https://smarden.org/runit/chpst.8). docker run -e, export and command substitution add none either. The one route I found that keeps a trailing newline is a Kubernetes variable taken from a Secret or ConfigMap key created from a file that ends in one; such a true now reads as the default.
  • HTTP headers (DNT, the registry's database header, the ETag skip header) cannot hold a line break.
  • Ci::Build#debug_mode? reads CI_DEBUG_TRACE and CI_DEBUG_SERVICES. GitLab Runner parses them with Go's strconv.ParseBool, which already rejects "true\n", so Rails and the runner now agree on such a value.
  • SupplyChain (behind slsa_provenance_statement) reads ATTEST_BUILD_ARTIFACTS and ATTEST_CONTAINER_IMAGES from .gitlab-ci.yml with default: true. Only a block scalar can end such a value in a newline; false written that way would now read as the default.
  • bin/spring keeps its own copy of the truthy pattern for ENABLE_SPRING, a development-only switch. I left it as it is; I can change it here too if you prefer.

Checks, Ruby 3.3.11:

  • gems/gitlab-utils with its own bundle: the .to_boolean examples with master's patterns, 18 examples, 9 failures (the nine new rows of the first table); with this MR, 18 examples, 0 failures; the whole gem, 1090 examples, 0 failures, 2 pending. With \A and \Z instead, only the three trailing-newline rows fail.
  • spec/lib/backup/options_spec.rb, spec/lib/supply_chain_spec.rb and the #debug_mode? examples of spec/models/ci/build_spec.rb: 196 examples, 0 failures.
  • RuboCop with the gem's configuration and scripts/lint/commit_linter.rb: clean.

This work comes from maintaining gitlab-mcp-server, whose upstream-bugs.md records everything done on its dependencies and sibling projects.

References

Closes #637433

Screenshots or screen recordings

Not a UI change.

How to set up and validate locally

  1. In gems/gitlab-utils, run bundle install.

  2. Run:

    bundle exec ruby -Ilib -e 'require "gitlab/utils"; ["false\ntrue", "garbage\ntrue", "garbage\noff", "\nyes", "true\n", "true", "fals"].each { |v| puts "#{v.inspect} => #{Gitlab::Utils.to_boolean(v).inspect}" }'
  3. On master the first five values print true, true, false, true and true. With this MR they print nil, and "true" and "fals" still print true and nil.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports

Loading
Loading