Skip to content

Check HTTP status codes in RawClient.Delete()#391

Merged
cutecatfann merged 3 commits intomainfrom
oauth-bug
Feb 10, 2026
Merged

Check HTTP status codes in RawClient.Delete()#391
cutecatfann merged 3 commits intomainfrom
oauth-bug

Conversation

@cutecatfann
Copy link
Copy Markdown
Contributor

@cutecatfann cutecatfann commented Feb 9, 2026

What I did

  • Adds HTTP status code checking to RawClient.Delete(), consistent with Get() and Post() which already check for non-2xx responses

RawClient.Delete() in pkg/desktop/raw_client.go discards the response body and only returns IO errors. Unlike Get() and Post(), it never inspects the HTTP status code. This means any error returned by the Docker Desktop backend (404, 500, etc.) is silently swallowed.

Testing

  • Ran docker mcp oauth revoke <app> against an improperly configured OAuth app, it surfaced error
docker mcp oauth revoke totally-not-a-real-app

Revoking OAuth access for totally-not-a-real-app...
failed to revoke OAuth access: HTTP 500: provider `totally-not-a-real-app`: not found
  • Ran docker mcp oauth revoke <app> against a working server and it succeeded
docker mcp oauth revoke github

Revoking OAuth access for github...
OAuth access revoked for github
  • Added unit tests with Claude Code, and ran via make test. All passed
	ok  	github.com/docker/mcp-gateway/pkg/db	2.589s
	=== RUN   TestGet_Success
	--- PASS: TestGet_Success (0.00s)
	=== RUN   TestGet_ErrorStatus
	--- PASS: TestGet_ErrorStatus (0.00s)
	=== RUN   TestGet_ErrorStatusPlainBody
	--- PASS: TestGet_ErrorStatusPlainBody (0.00s)
	=== RUN   TestPost_Success
	--- PASS: TestPost_Success (0.00s)
	=== RUN   TestPost_ErrorStatus
	--- PASS: TestPost_ErrorStatus (0.00s)
	=== RUN   TestPost_NilResult_Success
	--- PASS: TestPost_NilResult_Success (0.00s)
	=== RUN   TestPost_NilResult_ErrorStatus
	--- PASS: TestPost_NilResult_ErrorStatus (0.00s)
	=== RUN   TestDelete_Success
	--- PASS: TestDelete_Success (0.00s)
	=== RUN   TestDelete_204NoContent
	--- PASS: TestDelete_204NoContent (0.00s)
	=== RUN   TestDelete_ErrorStatusJSON
	--- PASS: TestDelete_ErrorStatusJSON (0.00s)
	=== RUN   TestDelete_ErrorStatusPlainBody
	--- PASS: TestDelete_ErrorStatusPlainBody (0.00s)
	PASS

(showing as 0.00 seconds since used a previously cached run of the test)

Related issue

(not mandatory) A picture of a cute animal, if possible in relation to what you did

@cutecatfann cutecatfann requested a review from a team as a code owner February 9, 2026 22:28
Copy link
Copy Markdown
Contributor

@bobbyhouse bobbyhouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Nice job going the extra mile to address issue at the gateway

suggestion (non-blocking): Consider adding unit tests. None exist currently. If you add tests it would set an expectation going forward.

question (non-blocking): Does the same bug exist for Post()? Do you know if there are any downstream problems we'll have if the same behavior exists there as well?

bobbyhouse
bobbyhouse previously approved these changes Feb 10, 2026
@cutecatfann
Copy link
Copy Markdown
Contributor Author

Only one caller passes nil for result, AvoidResourceSaverMode at raw_client.go:48. That call is fire-and-forget so the swallowed error doesn't matter there. The auth.go and policy calls all pass a &result, so they hit the status code check path. No downstream problem from the Post() bug in practice, but it's still inconsistent and should be fixed for correctness. I'll push up a quick rev with a couple of tests as well for best practice 😄

@cutecatfann cutecatfann merged commit 6401238 into main Feb 10, 2026
8 checks passed
@cutecatfann cutecatfann deleted the oauth-bug branch February 10, 2026 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants