Skip to content

fix: harden authorization on command mutation routes - #154

Merged
minghay merged 2 commits into
masterfrom
fix/harden-command-mutation-authorization
Sep 15, 2026
Merged

minghay merged 2 commits into
masterfrom
fix/harden-command-mutation-authorization

Conversation

@minghay

@minghay minghay commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Problem

The store re-derives authorization for command mutations instead of relying on the API's
decision, and two gaps have accumulated in that logic:

  1. DELETE /v1/commands/{namespace}/{name}/{version} accepts any token carrying user
    scope, and the handler skips its ownership check whenever the credential has no
    pipelineId. Since the store is directly reachable, a caller that never went through the
    API's permission check can reach the delete path.
  2. The ownership lookup in getCommandOwner requests the command record from the API at a
    /v1 path, but the API serves command records under /v4. The request therefore 404s,
    which the helper interprets as "no owner recorded" and allows. The ownership guard added in
    fix: enforce command ownership on store POST/DELETE #151 has consequently not been taking effect.

Change

  • Correct the ownership lookup to target the API path that actually serves command records, so
    the existing guard takes effect for build credentials.
  • Exclude guest scope from the delete route.
  • Additionally accept sdapi scope on the delete route. This is inert today and prepares the
    route for the API to send an explicitly scoped service token instead of forwarding the
    caller's token; user scope will be removed once that lands.

The skip for credentials without a pipelineId is deliberately left in place here: the API
currently forwards the caller's token for user-initiated deletes, so removing it before the API
change would break command deletion. That is handled as a follow-up.

Tests

  • Delete with guest scope is rejected.
  • Delete with user scope still succeeds (guards the current API-mediated flow).
  • A build credential deleting another pipeline's command is rejected, which the corrected
    lookup path now exercises.

Compatibility

Correcting the lookup means the ownership guard begins enforcing for build credentials where it
previously no-oped. A build that publishes or deletes a command owned by a different pipeline
will now receive a 403. That is the intended behavior of #151.

The store re-derives authorization for command mutations instead of
relying on the API's decision, and two gaps have accumulated in that
logic:

- DELETE /v1/commands/{namespace}/{name}/{version} accepted any token
  carrying `user` scope, including a `guest` token, and the handler
  skips its ownership check whenever the credential has no
  pipelineId. Since the store is directly reachable, a caller that
  never went through the API's permission check could reach the
  delete path.
- The ownership lookup requested the command record from the API at a
  /v1 path, but the API serves command records under /v4. The request
  therefore 404s, which is interpreted as "no owner recorded" and
  allowed. The ownership guard added in #151 has consequently not
  been taking effect for build credentials either.

This corrects the lookup path so the existing guard takes effect, and
excludes guest scope from the delete route. It also additionally
accepts `sdapi` scope on the route, which is inert today and prepares
it for the API to send an explicitly scoped service token instead of
forwarding the caller's token; `user` scope will be removed once that
lands.

The skip for credentials without a pipelineId is deliberately left in
place: the API currently forwards the caller's token for user-scope
deletes, so removing the skip before the API change would break
command deletion. That is a follow-up.
@minghay
minghay merged commit 5c2b56b into master Sep 15, 2026
1 check passed
@minghay
minghay deleted the fix/harden-command-mutation-authorization branch September 15, 2026 21:46
@sd-buildbot

Copy link
Copy Markdown

🎉 This PR is included in version 7.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants