Require write scope in oauth handling for write paths#711
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix an OAuth authorization bug where PKCE access tokens with scope=read could still be used to call write endpoints if the user had previously authorized the app for writes.
Changes:
- Adds
oauthScopeto request context when authenticating via PKCE access tokens. - Introduces
requireWriteScopemiddleware to enforcescope=writeon write routes (for PKCE tokens). - Applies
requireWriteScopeto many existing write endpoints in the v1 and v1/full routers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
api/server.go |
Adds requireWriteScope to multiple write routes to block PKCE scope=read tokens. |
api/auth_middleware.go |
Persists OAuth scope into request locals and introduces requireWriteScope middleware. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@rickyrombo I've opened a new pull request, #713, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@rickyrombo I've opened a new pull request, #714, to work on those changes. Once the pull request is ready, I'll request review from you. |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
- [x] Identify Developer Apps write endpoints missing `requireAuthMiddleware` + `requireWriteScope` - [x] Add `requireAuthMiddleware` and `requireWriteScope` to all Developer Apps write routes in `api/server.go`: - `POST /developer_apps` and `POST /developer-apps` - `PUT /developer_apps/:address` and `PUT /developer-apps/:address` - `DELETE /developer_apps/:address` and `DELETE /developer-apps/:address` - `POST /developer_apps/:address/access-keys/deactivate` and `POST /developer-apps/:address/access-keys/deactivate` - `POST /developer_apps/:address/access-keys` and `POST /developer-apps/:address/access-keys` - [x] Build verified successfully - [x] Code review passed with no issues <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>
`requireWriteScope` lacked test coverage for its three distinct code
paths: PKCE tokens with `scope=read` being rejected, `scope=write` being
allowed, and non-OAuth auth methods bypassing the scope check entirely.
## Changes
- **`api/auth_middleware_test.go`**: Adds `TestRequireWriteScope` with
three sub-tests covering each path. Since `requireWriteScope` only reads
`c.Locals("oauthScope")` and never touches the receiver, tests use a
bare `&ApiServer{}` — no DB required.
```go
testApp.Post("/write", func(c *fiber.Ctx) error {
if scope := c.Get("X-Test-Oauth-Scope"); scope != "" {
c.Locals("oauthScope", scope)
}
return c.Next()
}, app.requireWriteScope, ...)
// read scope → 403
// write scope → 200
// no scope (non-OAuth) → 200
```
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/AudiusProject/api/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>
rickyrombo
force-pushed
the
mjp-require-scope
branch
from
ecaa886 to
a9b5d90
Compare
Fixes bug where "read" tokens could be used on write endpoints as long as the user had authorized the app for writes previously