Answer external status check create and delete role refusals with 403

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab:jmrp-status-check-role-refusals into master

What does this MR do and why?

Managing a project's external status checks takes the Maintainer role (create_external_status_check, read_external_status_check, update_external_status_check and delete_external_status_check in config/authz/roles/maintainer.yml). A Developer who calls the REST API to change one is refused by the service, but two of the three write routes lose that refusal on the way to the response:

  • DELETE /projects/:id/external_status_checks/:check_id answers 204 No Content and the check is still there. ExternalStatusChecks::DestroyService refuses the caller, but the route calls it inside destroy_conditionally!, which sets the status to 204 and the body to empty before it yields and ignores what the block returns. A client is told the check was deleted.
  • POST /projects/:id/external_status_checks answers 500 Internal Server Error with {"message":["Not allowed"]}. ExternalStatusChecks::CreateService#access_denied_error builds its refusal with a reason and no http_status, so response.http_status is nil, and render_api_error! hands that nil to Grape, which answers it with 500. A client sees a fault of the instance and may retry or report an outage.

This MR makes both routes answer 403 Forbidden:

  • ee/lib/api/status_checks.rb: the create and delete routes check the role with authorize! before they call the service, on the same ability and subject the service checks (:create_external_status_check on the project, :delete_external_status_check on the check), so a caller without it is answered 403 with {"message":"403 Forbidden"}. That is how the merge request routes of the same API class already refuse: status_check_responses, the merge request's status check list and the retry all go through find_merge_request_with_access, which calls authorize!.
  • The delete route also renders the service's error from inside the destroy_conditionally! block, which replaces the 204, the same way the update route renders its service's error. A destroy that fails (422 Unprocessable Entity, with the model's errors) is now reported too, where it was also answered 204.

I chose 403 because the caller is authenticated and is refused for a permission it lacks. The REST API status codes describe 401 Unauthorized as "The user isn't authenticated" and 403 Forbidden as "The request isn't allowed", and a client that reads a 401 the way that page describes it concludes that its token is the problem.

The REST API style guide counts a change to any status code other than 500 as a breaking change, and a change from 500 to a supported status code as a bug fix. The create route is the second case. The delete route's 204 is not a 500, so here is why I still think changing it is a fix: the same status code table describes 204 No Content as the server having successfully fulfilled the request, and in both delete cases this MR changes (a refused caller and a destroy that fails) the request was not fulfilled and nothing was deleted. A client that trusted that answer was already wrong. If you would rather handle the delete route differently, I can split it into its own MR so that the create fix does not wait for it.

The update and list routes of this API class answer 401 Unauthorized to the same caller today, and this MR does not change them. A Developer's PUT /projects/:id/external_status_checks/:check_id is answered 401 with {"message":["Not allowed"]}, because ExternalStatusChecks::UpdateService gives its refusal http_status: :unauthorized and the route renders it. A Developer's GET /projects/:id/external_status_checks is answered 401 with {"message":"401 Unauthorized"} by the route's own unauthorized! when the caller cannot read_external_status_check. After this MR the class answers the same missing role with 403 on create and delete and with 401 on update and list. I left those two alone because each already answers a status a client may depend on, which the same style guide counts as a breaking change to move, while the two routes this MR fixes answered wrongly outright. Aligning them is the same small change (authorize! before the update service, and authorize! in place of unauthorized! on the list), and I can make it in this MR or in a follow-up, whichever you prefer. The before block's license check (check_feature_enabled!) also answers 401, to any caller who can see the project, when the project lacks the external_status_checks feature. That one is about a license rather than a role, and I left it as it is too.

I left the services unchanged. ExternalStatusChecks::CreateService's refusal still carries no http_status, but the route no longer reaches it, because authorize! asks the same question first. Checking in the route also keeps this fix away from the deprecated http_status attribute: the Gitlab/ServiceResponse cop asks for reason instead (#356036 (closed)), and the three services are already on its to-do list.

The GraphQL branch rule mutations that call these services (BranchRuleExternalStatusCheckCreate, BranchRuleExternalStatusCheckUpdate and BranchRuleExternalStatusCheckDestroy) need no change and do not answer with an HTTP status: they authorize on the branch rule before the service runs (authorize and authorized_find!), and turn a refusal from the service into GraphQL's resource-not-available error by reading response.cause. This MR changes nothing they run.

I left doc/api/status_checks.md unchanged. It documents neither the role these routes need nor their error responses, and I can add the role as a prerequisite here or in a follow-up if you prefer. I also did not add failure declarations to the two routes' desc blocks: the file is still on the API/DescriptionFailureResponse to-do list, and declaring them changes the generated OpenAPI document. I can do that in this MR too if you would like the new 403 to appear there.

Where the code is, on master at fa825b69
  • ee/lib/api/status_checks.rb:16: check_feature_enabled!, which answers 401 when the project lacks the licensed feature.
  • ee/lib/api/status_checks.rb:43-55: the create route, which renders response.http_status.
  • ee/lib/api/status_checks.rb:67: the list route's unauthorized! for a caller without read_external_status_check.
  • ee/lib/api/status_checks.rb:99-111: the update route, whose service answers a missing role with 401.
  • ee/lib/api/status_checks.rb:122-132: the delete route, which discarded the service's result.
  • ee/lib/api/status_checks.rb:172, 204 and 235: the merge request routes, which refuse through find_merge_request_with_access.
  • lib/api/helpers.rb:53-66: destroy_conditionally!, which sets 204 and an empty body before it yields.
  • lib/api/helpers.rb:378-382, 426-428 and 592-594: find_merge_request_with_access, authorize! and forbidden!.
  • ee/app/services/external_status_checks/create_service.rb:32-38: access_denied_error, with no http_status.
  • ee/app/services/external_status_checks/destroy_service.rb:27-33 and ee/app/services/external_status_checks/update_service.rb:35-41: the refusals with http_status: :unauthorized.
  • app/services/service_response.rb:13: ServiceResponse.error, whose http_status defaults to nil.
  • ee/spec/requests/api/status_checks_spec.rb: the existing "when feature is disabled, unlicensed or user has permission" tables drive only the project owner and a user who is not a member, whom user_project answers 404 before the service runs, so no example reached the service's refusal.

References

I searched the issues and merge requests of this project for the routes, the services and the Not allowed message and found nothing about this.

The routes declare feature_category :security_policy_management and their API page names the Security Policies group, so I would suggest Alan Paruszewski (who wrote the most recent fixes to these routes, such as !233327 (merged)), Sashi Kumar Kumaresan or Marcos Rocha, backend engineers in that group, for the review. #613741 notes that ownership of external status checks is not settled, and the Source Code group last reworked the role checks these services make, in !239178 (merged), in case that group should look as well.

Screenshots or screen recordings

A request from a user with the Developer role on the project, on a licensed instance:

Before After
POST /projects/:id/external_status_checks: 500 Internal Server Error, {"message":["Not allowed"]}, nothing created 403 Forbidden, {"message":"403 Forbidden"}, nothing created
DELETE /projects/:id/external_status_checks/:check_id: 204 No Content, the check is not deleted 403 Forbidden, {"message":"403 Forbidden"}, the check is not deleted
PUT /projects/:id/external_status_checks/:check_id: 401 Unauthorized, {"message":["Not allowed"]} Unchanged
GET /projects/:id/external_status_checks: 401 Unauthorized, {"message":"401 Unauthorized"} Unchanged

How to set up and validate locally

  1. On an instance with an Ultimate license, take a project and two users: one with the Maintainer role or above, and one with the Developer role, each with a personal access token that has the api scope.
  2. As the Maintainer, create a status check: 
    curl --request POST --header "PRIVATE-TOKEN: <maintainer_token>" \
  --data "name=QA&external_url=https://example.com/check" \
  "http://gdk.test:3000/api/v4/projects/<project_id>/external_status_checks"
  3. As the Developer, create one: 
    curl --include --request POST --header "PRIVATE-TOKEN: <developer_token>" \
  --data "name=QA2&external_url=https://example.com/check2" \
  "http://gdk.test:3000/api/v4/projects/<project_id>/external_status_checks"
    On master this answers 500 with {"message":["Not allowed"]}. On this branch it answers 403 with {"message":"403 Forbidden"}.
  4. As the Developer, delete the Maintainer's check: 
    curl --include --request DELETE --header "PRIVATE-TOKEN: <developer_token>" \
  "http://gdk.test:3000/api/v4/projects/<project_id>/external_status_checks/<check_id>"
    On master this answers 204, and GET /projects/<project_id>/external_status_checks as the Maintainer still lists the check. On this branch it answers 403 with {"message":"403 Forbidden"}, and the check is still listed.
  5. Run the specs: 
    bin/rspec ee/spec/requests/api/status_checks_spec.rb
Specs and checks I ran
  • New request examples in ee/spec/requests/api/status_checks_spec.rb: a Developer's create and delete, each expecting 403, {"message":"403 Forbidden"} and nothing changed, and an owner's delete whose destroy fails, expecting 422 with the model's error and the check kept.
  • Against master's ee/lib/api/status_checks.rb with the new examples, all three fail: the Developer's delete gets 204, the failed destroy gets 204, and the Developer's create gets 500 with ["Not allowed"].
  • With the change, ee/spec/requests/api/status_checks_spec.rb: 107 examples, 0 failures, 14 pending. The 14 pending examples all come from the existing authorizing granular token permissions shared examples, which this MR does not touch: seven when namespace enforces granular tokens cases skipped with "namespace has no top-level group", and seven mirrors the legacy non-member outcome cases skipped with "public-access bypass only applies to public resources".
  • The service specs of ExternalStatusChecks::CreateService, DestroyService and UpdateService and of BranchRules::ExternalStatusChecks::CreateService, DestroyService and UpdateService, which this MR does not change: 78 examples, 0 failures.
  • I could not run ee/spec/requests/api/graphql/mutations/branch_rules/external_status_checks/create_spec.rb and destroy_spec.rb locally: every example that runs (30 of 32, the other two are pending) fails before the mutation runs, loading sass for the stylesheet path the GraphQL controller's gon helper builds, and the create spec fails the same way on master. Every file those mutations run is unchanged from master in this MR, so I am relying on the pipeline for them.
  • RuboCop on the two changed files: no offenses. scripts/lint/commit_linter.rb on the commit: no problems.

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.

Where this comes from

I maintain gitlab-mcp-server, an MCP server that exposes the GitLab API to AI assistants. While making its status check tools tell a caller which role a refusal asks for, I followed each route's refusal to the response and found that the delete never sends it and the create sends it as a 500. Until this lands, its delete tool cannot tell a refused delete from a real one, and its create tool has to read a 500 carrying Not allowed as the role refusal it is. The project keeps a record of what it finds in its dependencies in upstream-bugs.md, with one entry for the delete and one for the create. The same record has an entry for the routes that answer a permission refusal with 401, the update and list routes above among them, which is why I would rather these two answer 403 than join that list.

Merge request reports