fix(oauth): filter unsupported scopes before parsing - #203
fix(oauth): filter unsupported scopes before parsing#203andrewsouthard wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Thanks — dropping unsupported scopes rather than failing the request is the right call for interop (it's what the reference provider does), so I'm keen to get this in. As it stands it's only half the change though: parseScope returns the filtered set but none of the callers use the return value, so the raw string is still stored and issued. I reproduced that end-to-end on this branch — details inline.
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Inline notes for the review above.
| { allowIncludes = false }: ParseScopeOptions = {}, | ||
| ): ScopesSet { | ||
| const set = ScopesSet.fromString(input ?? ""); | ||
| const filtered = |
There was a problem hiding this comment.
The filtering happens here, but nothing consumes it. Every caller ignores the return value and keeps the raw string:
par.ts~L186:params.scope = scope(raw), thenparseScope(scope)result droppedprovider.ts~L382 (authorize GET): sameprovider.ts~L524:scope = requestedScopegoes intoauthCodeData.scopeprovider.ts~L1122 (authorize POST): same- then
generateTokens({ scope: codeData.scope })and it rides through refresh
Reproduced on this branch with a full PAR → consent → token flow using atproto repo:com.example.thing?action=read madeup:thing include:bad: PAR 201, consent 200, and the token response is "scope": "atproto repo:com.example.thing?action=read madeup:thing include:bad" — junk and all.
That matters for three reasons: the token response's scope doesn't reflect what was granted (RFC 6749 §5.1); include:bad sneaks past the allowIncludes check below because it's filtered out before that loop runs; and — the one I actually care about — a scope we can't parse today gets stored verbatim and becomes a live permission the day @atproto/oauth-scopes is bumped to a version that understands it, without ever appearing on the consent screen. The reference implementation does parameters = { ...parameters, scope } to replace the stored value with the filtered one for exactly this reason.
Suggest: return (or expose) the filtered scope string and assign it back to params.scope / scope at those four sites before anything is stored.
| expect(response.status).toBe(400); | ||
| const json = (await response.json()) as { error: string }; | ||
| expect(json.error).toBe("invalid_scope"); | ||
| expect(response.status).toBe(201); |
There was a problem hiding this comment.
This would pass even if nothing was filtered. Once the callers are wired up, please carry this through to the token step and assert the response scope omits the dropped token — that's the test that pins the behaviour.
| it("rejects malformed granular scopes", () => { | ||
| expect(() => parseScope("atproto repo:not a real nsid")).toThrow( | ||
| ScopeParseError, | ||
| it("silently drops malformed scope tokens", () => { |
There was a problem hiding this comment.
These assert on a return value nothing in production consumes yet, so they don't really replace the deleted "rejects malformed / unknown" tests. Fine to keep as unit coverage of parseScope itself once the flow test above exists.
|
|
||
| export interface ParseScopeOptions { | ||
| /** | ||
| * When true, `include:` scopes are accepted (and structurally validated) |
There was a problem hiding this comment.
Stale now — this and the file header both still describe structural validation, which no longer happens here.
| } | ||
| if (!parser(scope)) { | ||
| throw new ScopeParseError(`Malformed scope: ${scope}`, scope); | ||
| if (scope.startsWith("include:") && !allowIncludes) { |
There was a problem hiding this comment.
Note include:<garbage> never reaches this check any more — isAtprotoOauthScope drops it before the loop. That's fine if the filtered string is what gets stored; with the current callers it's stored anyway.
|
I ran into this with my personal pds and had Claude whip up a fix. I do not vouch for that output except that I deployed it recklessly and my personal PDS has not exploded yet, haha. Feel free to scavenge if useful andrewsouthard#1 |
|
Thanks for the review and helpful pointers @ascorbic ! I've reworked the implementation based on your feedback. Happy to tweak it further if you see anything else I missed. Thanks! |
3 participants
Problem
Unsupported OAuth scopes throw instead of filtering them out.
Fix
Use
isAtprotoOauthScopeto filter scopes before parsing. This allows us to remove some parsing code added to handle invalid and malformed scopes.Testing
Fixes issue #198