Skip to content

Commit 622d365

Browse files
devinivyclaude
andauthored
Fix OAuth service-proxying audience for combined did#serviceId scopes (bluesky-social#4992)
* feat(did): add AtprotoDidRefAbsolute, DidRefRelative, AtprotoScopeAud, AtprotoTokenAud Renames AtprotoAudience to AtprotoDidRefAbsolute (kept as deprecated alias), adds DidRefRelative + isDidRefRelative for #fragment-only references, and introduces use-site unions: AtprotoScopeAud (always combined, for OAuth scopes) and AtprotoTokenAud (bare DID or combined, for service-auth JWT audience claims). Lays the groundwork for Phase 1 of proposal 0014. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(did): cover new DID-reference predicates Tightens isAtprotoDidRefAbsolute to reject empty fragments (`did:plc:abc#`), aligning it with isDidRefRelative which already rejects bare `#`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(oauth-scopes): use AtprotoScopeAud / isAtprotoDidRefAbsolute Switches from the legacy AtprotoAudience names to the renamed canonical forms. Behavior unchanged; the deprecated alias from @atproto/did keeps external consumers compiling. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(lexicons): loosen getServiceAuth.aud to accept did or did#serviceId The 'format: did' constraint excluded the combined did#serviceId syntax that OAuth-scoped credentials grant via 'rpc:lxm?aud=did#service'. Loosens to a plain string with a maxLength cap; handler-side validation will enforce that the value is either a bare atproto DID or an AtprotoDidRefAbsolute. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(lexicons): tighten getServiceAuth.aud description and maxLength Drops the internal "Phase 1" reference from the user-facing description and bumps maxLength from 1024 to 2048 to accommodate the spec maximum DID length plus a service-id fragment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pds): accept did#serviceId aud in getServiceAuth handler Validates that the caller-supplied aud is a valid atproto DID or AtprotoDidRefAbsolute, then passes the original string straight through to permissions.assertRpc and createServiceJwt. Combined-aud OAuth scopes now match (resolves part of bluesky-social#4850); password-session callers continue to use bare DID. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pds): pass combined did#serviceId aud to pipethrough scope check parseProxyInfo and parseProxyHeader now return the serviceId alongside the did and url. The proxyHandler uses the combined did#serviceId form when calling permissions.assertRpc, so OAuth credentials with 'rpc:lxm?aud=did#service' scopes succeed (resolves the proxy half of bluesky-social#4850). The outbound service-auth JWT keeps bare-DID aud — Slice D adds the operator-visible flag to flip that. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(pds): rename scopeAud/tokenAud to clarify Phase 1 asymmetry Names the Phase 1 audience asymmetry structurally rather than relying on a comment to document it. The scope check uses the combined did#serviceId form; the outbound JWT keeps bare-DID aud. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(pds): assert parseProxyHeader returns serviceId Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(oauth-scopes): cover combined-aud assertRpc match (issue bluesky-social#4850) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pds): validate getServiceAuth aud for all caller types Moves the aud predicate check from the OAuth-only authorize callback into the shared handler body, so app-password and session callers also get syntactic validation — restoring the coverage that the lexicon's format: did constraint provided before Slice A loosened it to a plain string with maxLength. Also updates app-passwords tests to use valid atproto DIDs instead of did:example:test, which now correctly fails the predicate check. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Address review feedback for service-auth oauth fix - Annotate non-self-documenting checks in isAtprotoDidRefAbsolute and isDidRefRelative with terse case-name comments. - Spell out "Phase 1 of service auth updates" in the proxyHandler asymmetry comment. - Drop the issue-number reference from the assertRpc combined-aud describe block. - Mirror PR bluesky-social#4602's createReport pattern: use scopeAud/tokenAud naming to make the bare-DID JWT audience structurally clear. - Add a proxyHandler regression test that fails when scope-check aud reverts to bare DID. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(pds): use bare-DID aud in get/putPreferences proxy fallback When getPreferences/putPreferences proxy to a non-default appview, they were sending the combined did#serviceId form as the JWT audience. This contradicts the Phase 1 convention used elsewhere (proxyHandler, pipethrough(), createReport) where the scope check sees the combined form but the outbound service-auth JWT keeps bare-DID aud. Adds a small bareDidFromProxyTo() helper in pipethrough.ts and uses it at both call sites. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(pds): use real AppContext + override only authorization Replaces the proxy-oauth-aud test's stub-AppContext with a real TestNetworkNoAppView. Only AuthVerifier.authorization is overridden to drive the OAuth path off an x-test-scope header — every other ctx field stays real and real-typed. The override's signature is constrained by the real method type, so future shape changes (renames, output type restructures) break the test at compile time rather than silently diverging. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(pds): tighten proxy-oauth-aud cases Drops the lastAud capture (a 200 already implies combined-form aud reached the scope check) and replaces the bare-DID-aud negative case with a different-service-id negative case. Bare-DID aud isn't a valid rpc scope — isAudParam rejects it at parse time — so the prior negative case wasn't actually exercising audience matching. The new case (`upstream.did#atproto_other`) parses valid and proves the runtime aud's service id is being compared. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: add changesets for service-auth oauth fix Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(pds): cover getServiceAuth aud handling Adds five integration cases: bare-DID and combined did#serviceId aud both produce a token whose aud claim equals the input; malformed, non-atproto, and empty-fragment aud values are rejected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: pnpm style:fix Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(pds): revert scopeAud/tokenAud rename in handlers without both The naming pair was meant to disambiguate when scope-check and JWT audiences appear in the same scope. In get/putPreferences and createReport only one of the two is in scope at a time, so the rename adds noise without clarifying anything. proxyHandler keeps both names since they sit side-by-side there. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(did): drop OAuth-flavored types; remove deprecated AtprotoAudience Removes AtprotoScopeAud, AtprotoTokenAud, AtprotoAudience, and isAtprotoAudience from @atproto/did. The only consumer (oauth-scopes) now uses AtprotoDidRefAbsolute / isAtprotoDidRefAbsolute directly via its own AudParam alias. Keeps @atproto/did focused on DID-level primitives without OAuth or service-auth concepts. The AtprotoAudience/isAtprotoAudience deprecated aliases are dropped outright (breaking) — migration is a single import swap. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(did): split did-ref.ts into atproto-agnostic primitives did-ref.ts now hosts generic DidRefAbsolute / DidRefRelative primitives (and their predicates) parameterized on a DID method. The atproto-flavored AtprotoDidRefAbsolute moves to atproto.ts alongside the other Atproto* helpers, defined as DidRefAbsolute<AtprotoIdentityDidMethods>. Public API surface is unchanged (both still flow through the package barrel). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(did): use String.includes for double-hash check Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: changeset for oauth-scopes breaking re-export change Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(oauth-scopes): tighten scopeNeededFor casts back to concrete types The casts `as AudParam`/`as LxmParam` widened to include `'*'`, which is wrong inside scopeNeededFor — it's constructing the specific scope for a single call, never a wildcard. Restore the original AtprotoDidRefAbsolute / Nsid casts. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore: lead BREAKING-change changesets with **BREAKING:** marker Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 92ed125 commit 622d365

22 files changed

Lines changed: 526 additions & 46 deletions

.changeset/crisp-bridges-dream.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@atproto/pds': patch
3+
---
4+
5+
Fix OAuth service-proxying audience check to use combined `did#serviceId` form, matching the shape of granted `rpc:` scopes.

.changeset/plain-rivers-glow.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@atproto/oauth-scopes': minor
3+
---
4+
5+
**BREAKING:** Remove the `AtprotoAudience` and `isAtprotoAudience` re-exports. They are superseded by `AtprotoDidRefAbsolute` and `isAtprotoDidRefAbsolute`, also re-exported from this package.

.changeset/quiet-bridges-hum.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@atproto/did': minor
3+
---
4+
5+
**BREAKING:** Remove `AtprotoAudience` and `isAtprotoAudience`. They are superseded by `AtprotoDidRefAbsolute` and `isAtprotoDidRefAbsolute`. Adds the related `DidRefAbsolute` and `DidRefRelative` types and predicates.

.changeset/quiet-rivers-dance.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@atproto/pds': patch
3+
---
4+
5+
`getServiceAuth` now accepts the combined `did#serviceId` form for its `aud` parameter, in addition to the bare DID form.

lexicons/com/atproto/server/getServiceAuth.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111
"properties": {
1212
"aud": {
1313
"type": "string",
14-
"format": "did",
15-
"description": "The DID of the service that the token will be used to authenticate with"
14+
"maxLength": 2048,
15+
"description": "The DID or `did#serviceId` reference of the service that the token will be used to authenticate with."
1616
},
1717
"exp": {
1818
"type": "integer",

packages/did/src/atproto.ts

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
import { z } from 'zod'
22
import { DidDocument, DidService } from './did-document.js'
33
import { DidError, InvalidDidError } from './did-error.js'
4+
import { DidRefAbsolute, isDidRefAbsolute } from './did-ref.js'
45
import { Did } from './did.js'
5-
import { canParse, isFragment } from './lib/uri.js'
6+
import { canParse } from './lib/uri.js'
67
import {
78
DID_PLC_PREFIX,
89
DID_WEB_PREFIX,
@@ -115,17 +116,6 @@ function isDidWebWithHttpsPort(did: Did<'web'>): boolean {
115116
return hasPort
116117
}
117118

118-
export type AtprotoAudience = `${AtprotoDid}#${string}`
119-
export const isAtprotoAudience = (value: unknown): value is AtprotoAudience => {
120-
if (typeof value !== 'string') return false
121-
const hashIndex = value.indexOf('#')
122-
if (hashIndex === -1) return false
123-
if (value.indexOf('#', hashIndex + 1) !== -1) return false
124-
return (
125-
isFragment(value, hashIndex + 1) && isAtprotoDid(value.slice(0, hashIndex))
126-
)
127-
}
128-
129119
export type AtprotoData<
130120
M extends AtprotoIdentityDidMethods = AtprotoIdentityDidMethods,
131121
> = {
@@ -231,3 +221,15 @@ export function isAtprotoVerificationMethod<
231221
matchesIdentifier(this.id, 'atproto', method.id)
232222
)
233223
}
224+
225+
/**
226+
* An atproto-constrained absolute DID reference: `${AtprotoDid}#${fragment}`.
227+
*/
228+
export type AtprotoDidRefAbsolute = DidRefAbsolute<AtprotoIdentityDidMethods>
229+
230+
export function isAtprotoDidRefAbsolute(
231+
value: unknown,
232+
): value is AtprotoDidRefAbsolute {
233+
if (!isDidRefAbsolute(value)) return false
234+
return isAtprotoDid(value.slice(0, value.indexOf('#')))
235+
}

packages/did/src/did-ref.ts

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import { Did, isDid } from './did.js'
2+
import { isFragment } from './lib/uri.js'
3+
4+
/**
5+
* An absolute DID reference: `${Did}#${fragment}`.
6+
*
7+
* @see {@link https://www.w3.org/TR/did-core/#did-url-syntax}
8+
*/
9+
export type DidRefAbsolute<M extends string = string> = `${Did<M>}#${string}`
10+
11+
export const isDidRefAbsolute = (value: unknown): value is DidRefAbsolute => {
12+
if (typeof value !== 'string') return false
13+
const hashIndex = value.indexOf('#')
14+
if (hashIndex === -1) return false // no '#'
15+
if (hashIndex === value.length - 1) return false // empty fragment
16+
if (value.includes('#', hashIndex + 1)) return false // more than one '#'
17+
return isFragment(value, hashIndex + 1) && isDid(value.slice(0, hashIndex))
18+
}
19+
20+
/**
21+
* A relative DID reference (a `#fragment` resolved against the surrounding
22+
* DID document's `id`). The optional `id` parameter narrows the fragment.
23+
*/
24+
export type DidRefRelative<I extends string = string> = `#${I}`
25+
26+
export function isDidRefRelative<I extends string = string>(
27+
value: unknown,
28+
id?: I,
29+
): value is DidRefRelative<I> {
30+
if (typeof value !== 'string') return false
31+
if (value.charCodeAt(0) !== 35 /* '#' */) return false // doesn't start with '#'
32+
if (value.length < 2) return false // empty fragment
33+
if (value.includes('#', 1)) return false // more than one '#'
34+
if (!isFragment(value, 1)) return false
35+
if (id !== undefined && value !== `#${id}`) return false // id mismatch
36+
return true
37+
}

packages/did/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
export * from './atproto.js'
22
export * from './did-document.js'
33
export * from './did-error.js'
4+
export * from './did-ref.js'
45
export * from './did.js'
56
export * from './methods.js'
67
export * from './utils.js'

packages/did/tests/did-ref.test.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
import { isAtprotoDidRefAbsolute } from '../src/atproto.js'
2+
import { isDidRefRelative } from '../src/did-ref.js'
3+
4+
describe('isAtprotoDidRefAbsolute', () => {
5+
it('accepts well-formed absolute references', () => {
6+
expect(
7+
isAtprotoDidRefAbsolute('did:plc:l3rouwludahu3ui3bt66mfvj#atproto'),
8+
).toBe(true)
9+
expect(isAtprotoDidRefAbsolute('did:web:example.com#service_id')).toBe(true)
10+
expect(isAtprotoDidRefAbsolute('did:web:example.com#atproto_label')).toBe(
11+
true,
12+
)
13+
})
14+
15+
it('rejects bare DIDs', () => {
16+
expect(isAtprotoDidRefAbsolute('did:plc:l3rouwludahu3ui3bt66mfvj')).toBe(
17+
false,
18+
)
19+
expect(isAtprotoDidRefAbsolute('did:web:example.com')).toBe(false)
20+
})
21+
22+
it('rejects relative references', () => {
23+
expect(isAtprotoDidRefAbsolute('#atproto')).toBe(false)
24+
})
25+
26+
it('rejects malformed input', () => {
27+
expect(isAtprotoDidRefAbsolute('')).toBe(false)
28+
expect(isAtprotoDidRefAbsolute('did:plc:l3rouwludahu3ui3bt66mfvj#')).toBe(
29+
false,
30+
)
31+
expect(
32+
isAtprotoDidRefAbsolute('did:plc:l3rouwludahu3ui3bt66mfvj##foo'),
33+
).toBe(false)
34+
expect(
35+
isAtprotoDidRefAbsolute('did:plc:l3rouwludahu3ui3bt66mfvj#a#b'),
36+
).toBe(false)
37+
expect(isAtprotoDidRefAbsolute('did:foo:bar#baz')).toBe(false)
38+
expect(isAtprotoDidRefAbsolute(null)).toBe(false)
39+
expect(isAtprotoDidRefAbsolute(123)).toBe(false)
40+
})
41+
})
42+
43+
describe('isDidRefRelative', () => {
44+
it('accepts well-formed relative references', () => {
45+
expect(isDidRefRelative('#atproto')).toBe(true)
46+
expect(isDidRefRelative('#atproto_label')).toBe(true)
47+
expect(isDidRefRelative('#a')).toBe(true)
48+
})
49+
50+
it('narrows on a specific id when supplied', () => {
51+
expect(isDidRefRelative('#atproto', 'atproto')).toBe(true)
52+
expect(isDidRefRelative('#atproto_label', 'atproto')).toBe(false)
53+
})
54+
55+
it('rejects absolute references and bare strings', () => {
56+
expect(isDidRefRelative('did:plc:l3rouwludahu3ui3bt66mfvj#atproto')).toBe(
57+
false,
58+
)
59+
expect(isDidRefRelative('atproto')).toBe(false)
60+
})
61+
62+
it('rejects malformed input', () => {
63+
expect(isDidRefRelative('')).toBe(false)
64+
expect(isDidRefRelative('#')).toBe(false)
65+
expect(isDidRefRelative('#a#b')).toBe(false)
66+
expect(isDidRefRelative(null)).toBe(false)
67+
expect(isDidRefRelative(123)).toBe(false)
68+
})
69+
})

packages/oauth/oauth-scopes/src/scope-permissions.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -264,4 +264,40 @@ describe('ScopePermissions', () => {
264264
).toBe(false)
265265
})
266266
})
267+
268+
describe('assertRpc combined-aud', () => {
269+
it('allows did#serviceId aud when scope grants the same combined form', () => {
270+
const set = new ScopePermissions(
271+
'rpc:app.bsky.feed.getFeed?aud=did:web:example.com%23bsky_appview',
272+
)
273+
expect(() =>
274+
set.assertRpc({
275+
aud: 'did:web:example.com#bsky_appview',
276+
lxm: 'app.bsky.feed.getFeed',
277+
}),
278+
).not.toThrow()
279+
})
280+
281+
it('rejects bare-DID aud when scope grants a combined form', () => {
282+
const set = new ScopePermissions(
283+
'rpc:app.bsky.feed.getFeed?aud=did:web:example.com%23bsky_appview',
284+
)
285+
expect(() =>
286+
set.assertRpc({
287+
aud: 'did:web:example.com',
288+
lxm: 'app.bsky.feed.getFeed',
289+
}),
290+
).toThrow()
291+
})
292+
293+
it('allows wildcard aud against a combined-form match', () => {
294+
const set = new ScopePermissions('rpc:app.bsky.feed.getFeed?aud=*')
295+
expect(() =>
296+
set.assertRpc({
297+
aud: 'did:web:example.com#bsky_appview',
298+
lxm: 'app.bsky.feed.getFeed',
299+
}),
300+
).not.toThrow()
301+
})
302+
})
267303
})

0 commit comments

Comments
 (0)