Accept string scope ids, and refuse values that are not ids

Marcos Rocharequested to merge
mc_rocha-policy-store-scope-id-coercion into master

What does this MR do and why?

Gitlab::PolicyStore::ScopeTranspiler kept a scope id only when it was already an Integer, and silently dropped anything else. A form-encoded request sends anything else: Grape leaves the values inside a Hash parameter as the strings they arrived as, so policy_scope[projects][including][][id]=5 reaches the transpiler as "5".

Dropping an id does not narrow a policy, it mis-scopes it, in one of two directions:

  • Under including: the criterion compiles to input.project.id in set(), which matches nothing, so a policy scoped to one project applies to none.
  • Under excluding: the scope_excluded body disappears while scope_included if { true } stays, so the policy applies to every project, the one the author excluded included.

The transpiler now coerces a run of ASCII digits into an id, and raises ValidationError on a value it cannot read as one. It refuses rather than drops because both failures above are invisible at authoring time and a 400 is not. The same holds for a criterion that is not a list of ids at all, which is what policy_scope[projects][excluding]=5 sends when the client omits the [].

A coerced id is then bounded by the widest a bigint holds. That bound is on the value rather than on the text it arrived as, because the two transports disagree about type: JSON.parse yields an Integer for a bare number while form encoding yields a String, so a bound on digit count only ever guarded one of them. It also refuses 0, -2, and 10**25, which used to be interpolated into a set no row can match.

An entry that names no id is left alone, because that is authored intent rather than a failed value. [{}], [nil], and {"type" => "personal"} all compile as they did.

Design decisions

A value is never rendered in full to be refused. An Integer wide enough to fail the ceiling is superlinear to render, so echoing it would move the cost from the program into the error message. An out-of-range id is named by its bound, and anything that is not a String by its type. A String is named, elided at 64 characters, because there the value is what tells a caller what to fix, except when the encoding is what refused it: "5".encode("UTF-32BE") renders identically to an id we accept.

Whitespace is refused rather than trimmed. " 5", "5 ", and "5\n" are all errors, so a client that does not trim its input gets a 400 rather than a working policy. Trimming is the same leniency this MR removes, and a caller cannot tell a trimmed id from an authored one after the fact.

A stored scope holding a now-refused id will fail an unrelated rename, because the transpiler emits the policy name and so a rename recompiles from the stored policy_scope. Nothing is at risk while the only repository is per-process and in-memory, but it is a migration question for the persistent adapter.

How to set up and validate locally

Every step runs in gdk rails console. The gem needs no other setup.

  1. Confirm a string id now reaches the compiled program. Verify the output contains input.project.id in {5}, where master compiles the same input to input.project.id in set()
puts Gitlab::PolicyStore::ScopeTranspiler.new(
  { "projects" => { "including" => [{ "id" => "5" }] } },
  policy_name: "Scoped policy"
).transpile
  1. Confirm a value that is not an id is refused rather than dropped, in either direction and whether it arrives inside a list or in place of one. Verify all three print a message naming what was refused, because on master both excluding scopes compile to no scope_excluded body and to scope_included if { true }, applying the policy to every project instead of excluding one
[{ "including" => ["3", "1; injected"] }, { "excluding" => ["1; injected"] }, { "excluding" => "5" }].each do |projects|
  Gitlab::PolicyStore::ScopeTranspiler.new({ "projects" => projects }, policy_name: "Scoped policy").transpile
rescue Gitlab::PolicyStore::ValidationError => error
  puts error.message
end
  1. Confirm an id outside the range a bigint holds is refused whichever transport delivered it. Verify all four print policy_scope carries an id outside the range 1 to 9223372036854775807, because on master the strings were dropped to in set() while the Integers were interpolated into a set no row can match
["9999999999999999999", 10**25, "-2", -2].each do |id|
  Gitlab::PolicyStore::ScopeTranspiler.new(
    { "projects" => { "including" => [id] } },
    policy_name: "Scoped policy"
  ).transpile
rescue Gitlab::PolicyStore::ValidationError => error
  puts error.message
end
  1. Confirm an entry that names no id is still accepted, since declaring a criterion with no id is authored intent. Verify the output contains input.project.id in set() and no error
puts Gitlab::PolicyStore::ScopeTranspiler.new(
  { "projects" => { "including" => [{}] } },
  policy_name: "Scoped policy"
).transpile
  1. Confirm a value too wide to render is refused without rendering it. Verify the Integer is refused in a fraction of the time the last line reports for rendering it once, and that the String is echoed only up to its 64-character cap
require "benchmark"

huge = 10**5_000_000

[huge, "9" * 5_000_000].each do |id|
  elapsed = Benchmark.realtime do
    Gitlab::PolicyStore::ScopeTranspiler.new(
      { "projects" => { "including" => [id] } },
      policy_name: "Scoped policy"
    ).transpile
  rescue Gitlab::PolicyStore::ValidationError => error
    puts "#{error.message.length} characters: #{error.message}"
  end

  puts format("%.6f seconds", elapsed)
end

puts format("%.6f seconds to render the Integer once", Benchmark.realtime { huge.to_s })

References

Edited by Marcos Rocha

Merge request reports