Validate conflict resolution input before calling Gitaly

Marc Shawrequested to merge
630904-validate-conflict-resolution-input into master

What does this MR do and why?

Makes MergeRequests::Conflicts::ResolveService validate the files and commit message it is given before calling Gitaly, so bad input gets a clear 400 instead of a 503/500 or a silently emptied file. The web UI never sends invalid input, but API and GraphQL clients can. This is a step of the epic gitlab-org#23712 and builds on the merged refactor !257833 (merged), which made the service return a ServiceResponse.

Closes #630904 (closed)

Detailed context for AI agents ### Behaviour on master that this fixes

The web conflicts UI only sends file paths and sections it built from the server's own conflict list, so master never validates this input before passing it to Gitaly. An API or GraphQL client (the mutation coming in #630908) can send anything.

  • A path that is not in conflict reaches Gitaly, which answers with an internal error (13:NoMethodError: undefined method 'resolve_lines' for nil). It is mapped to Gitlab::Git::CommandError, so the controller answers 503 "Gitaly is not available" via HandlesGitalyErrors, and a Sentry event is created per attempt.
  • An empty sections hash with no content is treated by Gitaly as a content resolution and commits the file empty. A sections value that is not a hash (for example a string), or a hash whose values are not strings (a nested hash, a number, an array), makes Gitaly answer 13:json: cannot unmarshal ..., again a 503. The controller's permit already strips non-hash sections for web requests, but it keeps nested hashes and non-string values, so those reach the service from any client.
  • A blank (empty string) commit_message reaches Gitaly and raises ArgumentError "3:empty CommitMessage" (INVALID_ARGUMENT), which is a 500. nil already works because FileCollection#resolve does commit_message || default_commit_message.

What changed

All app changes are in app/services/merge_requests/conflicts/resolve_service.rb, plus two new translatable strings in locale/gitlab.pot.

Two new reason constants: REASON_FILE_NOT_IN_CONFLICT = :file_not_in_conflict and REASON_INCOMPLETE_FILE = :incomplete_file.

After the existing can_be_resolved_in_ui? gate, the service validates params[:files] (wrapped with Array.wrap). For each file, in order:

  1. If ListService#file_for_path(old_path, new_path) returns nil, the service returns an error "File %{path} is not in conflict." with reason file_not_in_conflict.
  2. Otherwise, if content is nil and sections is not a non-empty hash of strings, it returns an error "File %{path} needs either sections or content." with reason incomplete_file. "Non-empty hash of strings" is checked with respond_to?(:each_pair) && present? && values.all?(String), so an empty hash, a string, nil and a hash with a non-string value are all refused.

%{path} is the file's new_path, or old_path when new_path is blank. The first failing file wins. Both refusals come out of the controller as a 400 with the message (see below).

Why the validation sits before the counter: it runs after the can_be_resolved_in_ui? gate and before track_resolve_conflict_action and the can_be_merged? check. A request refused for invalid input is therefore not counted as a resolve attempt, the same as the other refusals.

file_for_path reuses the conflict list that can_be_resolved_in_ui? already loaded in the same FileCollection, so the normal case costs no extra Gitaly call. When can_be_resolved_in_ui? was served from its Redis cache (cache_method), the list is fetched now. That cache is keyed by the two commit ids and ignores allow_tree_conflicts, so a tree-conflict-tolerant ListService can cache true for a merge request with a modify/delete conflict, and the service's own non-tolerant listing then raises Gitlab::Git::Conflict::Resolver::ConflictSideMissing from file_for_path. No production caller builds a tolerant lister and calls can_be_resolved_in_ui? on it today (the GraphQL conflict resolvers and diff_helper skip the check), so this is defensive. The service rescues it into the existing REASON_NOT_RESOLVABLE_IN_UI with the existing "cannot be resolved through GitLab" message. For a fixed commit pair the conflict set cannot otherwise change under a cached true. UnsupportedEncoding cannot reach this path, so it is not rescued.

The resolve call now passes params[:commit_message].presence, so a blank string falls back to the default commit message exactly like nil.

Why the controller is unchanged

Projects::MergeRequests::ConflictsController#render_resolve_conflicts_error already maps any reason other than no-push-access/not-resolvable (404) and pre-receive (422) to a 400 with the message. Both new reasons therefore come out as 400s with no controller edit. not_resolvable_in_ui from the ConflictSideMissing rescue comes out as a 404, which matches what master answered for conflicts that cannot be resolved in the UI.

Rollout and risk

  • No behaviour change for the web UI, since it never sends invalid input.
  • No feature flag: the change only turns requests that previously failed with 500/503 (or silently emptied a file) into 400s.
  • No database change.
  • Changelog trailer: Changelog: fixed.

Out of scope (sibling issues of the epic)

Specs

spec/services/merge_requests/conflicts/resolve_service_spec.rb, new contexts:

  • Blank commit message falls back to the default "Merge branch 'conflict-start' into 'conflict-resolvable'" message.
  • A file not in conflict (files/ruby/version_info.rb) is refused with the reason and message, no commit is created, and the counter is not tracked (shared example 'a refusal before resolving').
  • A file with neither sections nor content, for sections = {}, 'head' (a string), { id => ['head'] } (a non-string value) and nil.
  • A tree-conflict-tolerant ListService caching can_be_resolved_in_ui? as true for the conflict-missing-side fixture branch: the service reads the cached true back and returns not_resolvable_in_ui from the ConflictSideMissing rescue, with no stubs.

spec/controllers/projects/merge_requests/conflicts_controller_spec.rb, new context:

  • POSTing a path that is not in conflict returns 400 with the message and creates no commit (end to end, no stubbing).

Verification, run locally on this branch:

bundle exec rspec spec/services/merge_requests/conflicts/resolve_service_spec.rb
25 examples, 0 failures

bundle exec rspec spec/controllers/projects/merge_requests/conflicts_controller_spec.rb
20 examples, 0 failures

With master's resolve_service.rb swapped in, the same service spec file gives 25 examples, 7 failures: exactly the seven new behaviour examples (the example that only checks the cache read passes on master too).

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Marc Shaw

Merge request reports