From a 500 to a real error: validating comment length across 9 GitLab GraphQL mutations

Here's a bug you've probably shipped yourself. The model has a validation, the API docs mention the limit, and the API never checks it. Everything works until someone sends input that's too long, and then the user gets "Internal server error" instead of a message they can act on.

This is the story of gitlab-org/gitlab!269616. It added a length check to the comment argument of nine GraphQL mutations in GitLab's vulnerability management, and it went through four rounds of CI fixes and review. The MR touched 20 files, most of them specs and generated docs.

The issue: a documented limit nobody enforced

The issue was gitlab-org/gitlab#637728, opened by Lorenz van Herwaarden. It started with a GitLab Duo review comment on an earlier MR. Here's the gist:

• Vulnerabilities::StateTransition has validates :comment, length: { maximum: 50_000 }.
• Several GraphQL mutations say "(maximum 50,000 characters)" in their comment description, but don't enforce it.
• A longer comment reaches Vulnerabilities::StateTransition.create!, which raises ActiveRecord::RecordInvalid. Nothing rescues it, so the user gets a generic "Internal server error".
• Bulk dismiss is worse. It uses StateTransition.insert_all!, which skips model validations, so an oversized comment only fails at the database check constraint (char_length(comment) <= 50000).

The issue also explained why rescuing the error in the services was the wrong fix:

A rescue would run after CreateService may already have saved the vulnerability.

So the fix had to happen before anything is written: at the GraphQL argument.

The plan

I asked to work on it with this plan:

My plan is to add validates: { length: { maximum: 50_000 } } to the comment arguments on the security-finding and vulnerability state-transition mutations (including the shared VulnerabilityStateTransitions concern), following the existing GraphQL argument pattern, then add a spec per mutation for a comment over 50,000 characters so the error is returned before anything is written.

Lorenz assigned me and improved the scope right away. The issue had missed two mutations that also take a comment, securityFindingRevertToDetected and vulnerabilitiesDismiss, so Lorenz added them to the description. Lorenz also asked me to include Subashis Chakraborty from the team as a reviewer.

The final list was nine mutations:

• vulnerabilityConfirm, vulnerabilityResolve, vulnerabilityDismiss, vulnerabilityRevertToDetected (through one shared concern)
• securityFindingConfirm, securityFindingResolve, securityFindingDismiss, securityFindingRevertToDetected
• vulnerabilitiesDismiss (bulk)

The fix: validate at the boundary

GraphQL-Ruby can validate an argument before the resolver runs. GitLab already uses this in other mutations, so the change is one line per argument. Here's the shared concern that covers four of the mutations:

argument :comment,
  GraphQL::Types::String,
  required: false,
  validates: { length: { maximum: ::Vulnerabilities::StateTransition::COMMENT_MAX_LENGTH } },
  description: "Comment why vulnerability was #{state_transition_name_past_tense} (maximum 50,000 characters)."

If the comment is too long, GraphQL returns a validation error and the resolver never runs. No vulnerability, no state transition, no half-written data. This also covers bulk dismiss, because the check runs before insert_all! is ever reached.

Each mutation got a request spec that proves two things: the error comes back, and nothing is written. Here's the bulk dismiss one:

context "when the comment exceeds the maximum length" do
  let(:comment) { 'a' * (::Vulnerabilities::StateTransition::COMMENT_MAX_LENGTH + 1) }

  it "returns a validation error and does not dismiss the vulnerabilities" do
    expect { post_graphql_mutation(mutation, current_user: current_user) }
      .not_to change { Vulnerabilities::StateTransition.count }

    expect(graphql_errors).to include(a_hash_including('message' => include('too long')))
    expect(vulnerability_1.reload).not_to be_dismissed
    expect(vulnerability_2.reload).not_to be_dismissed
  end
end

The not_to change { ... count } block matters most. Returning an error isn't enough; the spec has to prove that nothing was written.

Round 1: my own CI mistakes

The first pipeline failed two jobs:

• rubocop: I'd left extra blank lines at the end of four request specs.
• graphql-verify: I'd changed the securityFindingRevertToDetected description, but not the committed introspection schemas (public/-/graphql/introspection_result.json and its _no_deprecated sibling).

That second one is worth knowing about. In GitLab, a GraphQL description is part of the public schema. Changing it means regenerating the GraphQL API docs and the introspection JSON files, and CI checks that they match. I fixed both in 74bc19de, and the next pipeline passed.

Round 2: docs review

Brendan Lynch added Isaac Durham as a reviewer for the vulnerability management group. Isaac reviewed from the docs side and suggested adding the limit to the dismiss and revert_to_detected descriptions, to match the other comment arguments. Isaac also reminded me that I'd need to rerun bundle exec rake gitlab:graphql:docs:compile afterwards.

I applied both suggestions in 06b168dd, with regenerated docs and introspection JSON. The pipeline failed again, on rubocop:

ee/app/graphql/mutations/security/finding/revert_to_detected.rb:25:121: C: Layout/LineLength: Line is too long. [122/120]

The new description string was two characters too long for one line.

Round 3: backend review, four suggestions

Subashis then did a thorough review. It opened by summarizing why the change was safe:

The change is minimal and non breaking (it enforces the already-documented model limit at the GraphQL boundary)…

Then came four suggestions:

1. Magic number duplication. 50_000 appeared as a literal in 7 places (the model and 6 GraphQL arguments). Extract COMMENT_MAX_LENGTH on the model so the model and the API can't drift apart.
2. A failure-only spec. The new securityFindingDismiss request spec only covered the over-limit case. Add a happy path, or trim the unused fields.
3. Description style. confirm.rb and resolve.rb used "(maximum 50,000 characters)", while my updated dismiss.rb and revert_to_detected.rb used ". Maximum 50,000 characters." Pick one form.
4. Loose error matching. expect(graphql_errors.to_s).to include('comment is too long') turns the whole errors array into a string. Matching the message directly gives a more readable failure.

I applied all four in 2fd1e423. The constant lives on the model, and both the model validation and the API use it:

COMMENT_MAX_LENGTH = 50_000

# ...
validates :comment, length: { maximum: COMMENT_MAX_LENGTH }

Suggestion 3 is interesting because it went the other way from Round 2. Isaac's suggestion used the sentence form; Subashis suggested the parenthetical form, which the shared concern and two other mutations already used. I went with the parenthetical form everywhere, so the schema reads the same across all nine mutations. That meant regenerating the docs and introspection JSON once more, and it also fixed the Layout/LineLength offense from Round 2.

Then CI failed one more time:

Layout/LineEndStringConcatenationIndentation

When I split the long description into two concatenated strings, the continuation line wasn't indented the way RuboCop wanted. One more small commit (345bc6ff) fixed it, and the pipeline went green.

Round 4: the Danger warning, and the merge

After the constant extraction, Danger left a note on the model file:

Did you consider new validations can break existing records?

It's a reasonable thing for a bot to ask, because the diff touches a validates line. But it's not a new validation. I replied on the thread:

This isn't a new model validation: Vulnerabilities::StateTransition already had validates :comment, length: { maximum: 50_000 }. This MR only moves the literal into COMMENT_MAX_LENGTH so the GraphQL comment arguments can reuse the same limit. Existing records already satisfy it, so nothing can break.

Subashis confirmed it:

It's a false positive. The MR doesn't add a new model validation

Subashis also pointed out that the database check constraint enforces the limit on its own, and that the genuinely new validations in this MR are GraphQL argument validators, which "only run on incoming API input, never against stored records."

Isaac and Subashis both approved, and Subashis merged the MR. In total there were seven pipelines: three failed on my RuboCop and graphql-verify mistakes, and the rest passed.

What I took away

1. Validate at the boundary, before side effects. If a service can write data before an error is raised, rescuing the error is too late. Reject bad input where it enters the system.

2. Watch out for insert_all!. It skips model validations. If a code path uses it, model validations can't protect that path, and the check has to happen somewhere else.

3. In GitLab, GraphQL descriptions are schema. Changing a description means regenerating doc/api/graphql/reference and the introspection JSON. graphql-verify will catch you if you forget.

4. Run RuboCop on the exact files you touched before pushing. Three of my seven pipelines failed on RuboCop or generated files, all avoidable. Each extra pipeline costs reviewers time and CI minutes.

5. When suggestions conflict, choose consistency. I went with the form the shared concern already used, which kept all nine descriptions the same.

6. Answer bot warnings with facts. The Danger note was a fair question. A short, specific reply let the maintainer confirm it was a false positive quickly.

7. Let maintainers improve the scope. Lorenz added two mutations the issue had missed. A plan comment before writing code is what made that possible.

Links

• Issue: gitlab-org/gitlab#637728
• Merge request: gitlab-org/gitlab!269616
• The review comment that started it: gitlab-org/gitlab!253678

Thanks to Lorenz van Herwaarden for the issue and the extra scope, Subashis Chakraborty for a detailed and kind review, Isaac Durham for the docs review, and Brendan Lynch for routing it to the right people.