Skip to content

fix(oauth): filter unsupported scopes before parsing #203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andrewsouthard wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(oauth): filter unsupported scopes before parsing #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
373c4ad
fix(oauth): filter unsupported scopes via isAtprotoOauthScope before …
andrewsouthard Jul 12, 2026
c44cd1d
fixup fix(oauth): filter unsupported scopes via isAtprotoOauthScope b…
andrewsouthard Aug 23, 2026
264f45a
fix(oauth): support space scopes
andrewsouthard Sep 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/oauth-provider/src/par.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ export class PARHandler {
params.scope = scope;
const allowIncludes = !!this.permissionSetResolver;
try {
parseScope(scope, {
params.scope = parseScope(scope, {
allowIncludes,
allowSpaceScopes: this.allowSpaceScopes,
});
Expand Down
62 changes: 27 additions & 35 deletions packages/oauth-provider/src/provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,7 @@ import {
isTokenValid,
AUTH_CODE_TTL,
} from "./tokens.js";
import {
renderConsentUI,
renderErrorPage,
getConsentUiCsp,
} from "./ui.js";
import { renderConsentUI, renderErrorPage, getConsentUiCsp } from "./ui.js";
import type { PermissionSetBundle, SpaceScopeInfo } from "./ui.js";
import { IncludeScope } from "@atproto/oauth-scopes";
import { authenticateClient, ClientAuthError } from "./client-auth.js";
Expand Down Expand Up @@ -226,9 +222,7 @@ export class ATProtoOAuthProvider {
* consent — unlike permission sets, the scope's meaning is fully carried
* by the scope string itself.
*/
private async resolveSpaceMetadata(
scope: string,
): Promise<SpaceScopeInfo[]> {
private async resolveSpaceMetadata(scope: string): Promise<SpaceScopeInfo[]> {
if (!this.spacesEnabled) return [];
const spaces: SpaceScopeInfo[] = [];
for (const token of scope.split(" ")) {
Expand All @@ -243,22 +237,18 @@ export class ATProtoOAuthProvider {
this.permissionSetResolver?.resolveSpaceDeclaration
) {
try {
const decl =
await this.permissionSetResolver.resolveSpaceDeclaration(
perm.type as Parameters<
NonNullable<
PermissionSetResolver["resolveSpaceDeclaration"]
>
>[0],
);
const decl = await this.permissionSetResolver.resolveSpaceDeclaration(
perm.type as Parameters<
NonNullable<PermissionSetResolver["resolveSpaceDeclaration"]>
>[0],
);
if (decl?.name) {
info.name = decl.name;
} else {
info.error = "Space type declaration was not found";
}
} catch (e) {
info.error =
e instanceof Error ? e.message : "Resolution failed";
info.error = e instanceof Error ? e.message : "Resolution failed";
}
}
spaces.push(info);
Expand All @@ -284,9 +274,7 @@ export class ATProtoOAuthProvider {
? async (nsid) => {
const decl = await resolver.resolveSpaceDeclaration!(
nsid as Parameters<
NonNullable<
PermissionSetResolver["resolveSpaceDeclaration"]
>
NonNullable<PermissionSetResolver["resolveSpaceDeclaration"]>
>[0],
);
return decl?.collections ?? null;
Expand Down Expand Up @@ -467,11 +455,10 @@ export class ATProtoOAuthProvider {
// expanded later, at code-issuance time, so the consent UI can show
// bundle titles in their original include form.
const scope = params.scope ?? ATPROTO_SCOPE;
params.scope = scope;
const allowIncludes = !!this.permissionSetResolver;
const allowSpaceScopes = this.spacesEnabled;
try {
parseScope(scope, { allowIncludes, allowSpaceScopes });
params.scope = parseScope(scope, { allowIncludes, allowSpaceScopes });
} catch (e) {
if (e instanceof ScopeParseError) {
return await this.renderError("invalid_scope", e.message);
Expand Down Expand Up @@ -613,13 +600,15 @@ export class ATProtoOAuthProvider {
// stored scope contains only concrete granular permissions.
const requestedScope = params.scope ?? ATPROTO_SCOPE;
let scope = requestedScope;
if (
this.permissionSetResolver &&
requestedScope.includes("include:")
) {
if (this.permissionSetResolver && requestedScope.includes("include:")) {
try {
scope = await expandScope(requestedScope, this.permissionSetResolver);
parseScope(scope, { allowSpaceScopes: this.spacesEnabled });
const expandedScope = await expandScope(
requestedScope,
this.permissionSetResolver,
);
scope = parseScope(expandedScope, {
allowSpaceScopes: this.spacesEnabled,
});
} catch (e) {
if (e instanceof ScopeParseError) {
const errorUrl = new URL(redirectUri);
Expand Down Expand Up @@ -1061,9 +1050,7 @@ export class ATProtoOAuthProvider {
*/
async verifyAccessToken(
request: Request,
check?:
| string
| ((perms: ScopePermissionsTransition) => void),
check?: string | ((perms: ScopePermissionsTransition) => void),
): Promise<TokenData | null> {
// Extract token from Authorization header
const tokenInfo = extractAccessToken(request);
Expand Down Expand Up @@ -1214,13 +1201,18 @@ export class ATProtoOAuthProvider {
const allowIncludes = !!this.permissionSetResolver;
let scope = requestedScope;
try {
parseScope(requestedScope, {
scope = parseScope(requestedScope, {
allowIncludes,
allowSpaceScopes: this.spacesEnabled,
});
if (allowIncludes && requestedScope.includes("include:")) {
scope = await expandScope(requestedScope, this.permissionSetResolver);
parseScope(scope, { allowSpaceScopes: this.spacesEnabled });
const expandedScope = await expandScope(
requestedScope,
this.permissionSetResolver,
);
scope = parseScope(expandedScope, {
allowSpaceScopes: this.spacesEnabled,
});
}
} catch (e) {
if (e instanceof ScopeParseError) {
Expand Down
74 changes: 40 additions & 34 deletions packages/oauth-provider/src/scopes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
/**
* Scope parsing and matching, built on @atproto/oauth-scopes.
*
* Granular scopes (`repo:`, `rpc:`, `blob:`, `account:`, `identity:`) are
* parsed structurally. Permission-set includes (`include:NSID?aud=...`) are
* resolved at authorize-time via an injected {@link PermissionSetResolver}
* and expanded into concrete granular scopes inline before the auth code is
* stored — so resource-server checks never need network access.
* Unsupported or malformed scope tokens are filtered out (via
* `isAtprotoOauthScope`) rather than rejected. Permission-set includes
* (`include:NSID?aud=...`) are resolved at authorize-time via an injected
* {@link PermissionSetResolver} and expanded into concrete granular scopes
* inline before the auth code is stored — so resource-server checks never
* need network access.
*/

import type { Nsid as AtcuteNsid } from "@atcute/lexicons/syntax";
Expand All @@ -16,14 +17,20 @@ import {
IncludeScope,
RepoPermission,
RpcPermission,
isAtprotoOauthScope,
ScopeMissingError,
ScopePermissionsTransition,
ScopesSet,
} from "@atproto/oauth-scopes";
import * as oauthScopes from "@atproto/oauth-scopes";
import type { PermissionSetResolver } from "./permission-sets.js";

export { IncludeScope, ScopeMissingError, ScopePermissionsTransition, ScopesSet };
export {
IncludeScope,
ScopeMissingError,
ScopePermissionsTransition,
ScopesSet,
};

/**
* `SpacePermission` is only present in the `spaces-alpha` builds of
Expand Down Expand Up @@ -94,10 +101,10 @@ const STRUCTURAL_PARSERS: Record<

export interface ParseScopeOptions {
/**
* When true, `include:` scopes are accepted (and structurally validated)
* but not expanded — the returned ScopesSet may still contain them.
* Use this at authorize-time, then call {@link expandScope} to resolve
* the includes before storing.
* When true, `include:` scopes are accepted but not expanded — the
* returned scope string may still contain them. Use this at
* authorize-time, then call {@link expandScope} to resolve the includes
* before storing.
*
* When false (default), `include:` scopes throw a ScopeParseError. Use
* this on already-expanded scope strings (e.g. when re-validating a
Expand All @@ -113,14 +120,22 @@ export interface ParseScopeOptions {
}

/**
* Validate a space-separated scope string. Returns the parsed ScopesSet on
* success.
* Filter and validate a space-separated scope string, returning the cleaned
* scope string on success. Tokens not recognized by `isAtprotoOauthScope`
* are silently dropped; a missing "atproto" base scope or a disallowed
* `include:` scope throws a {@link ScopeParseError}.
*/
export function parseScope(
input: string | undefined | null,
{ allowIncludes = false, allowSpaceScopes = false }: ParseScopeOptions = {},
): ScopesSet {
const set = ScopesSet.fromString(input ?? "");
): string {
const filtered =

@ascorbic ascorbic Aug 22, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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), then parseScope(scope) result dropped
  • provider.ts ~L382 (authorize GET): same
  • provider.ts ~L524: scope = requestedScope goes into authCodeData.scope
  • provider.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.

(input ?? "")
.split(" ")
.filter(Boolean)
.filter(isAtprotoOauthScope)
.join(" ") || undefined;
const set = ScopesSet.fromString(filtered);

if (!set.has(ATPROTO_SCOPE)) {
throw new ScopeParseError(
Expand Down Expand Up @@ -149,7 +164,11 @@ export function parseScope(
const colon = scope.indexOf(":");
const question = scope.indexOf("?");
const end =
colon === -1 ? question : question === -1 ? colon : Math.min(colon, question);
colon === -1
? question
: question === -1
? colon
: Math.min(colon, question);
const resource = end === -1 ? scope : scope.slice(0, end);
if (resource === "space" && !allowSpaceScopes) {
throw new ScopeParseError(
Expand All @@ -158,9 +177,7 @@ export function parseScope(
);
}
const parser =
STRUCTURAL_PARSERS[
resource as (typeof GRANULAR_RESOURCES)[number]
];
STRUCTURAL_PARSERS[resource as (typeof GRANULAR_RESOURCES)[number]];
if (!parser) {
throw new ScopeParseError(`Unknown scope resource: ${scope}`, scope);
}
Expand All @@ -169,7 +186,7 @@ export function parseScope(
}
}

return set;
return Array.from(set).join(" ");
}

/**
Expand Down Expand Up @@ -261,9 +278,7 @@ export interface FinalizeSpaceScopesOptions {
* without default collections (reads unaffected, writes constrained to
* the explicitly requested collections).
*/
resolveSpaceCollections?: (
nsid: string,
) => Promise<readonly string[] | null>;
resolveSpaceCollections?: (nsid: string) => Promise<readonly string[] | null>;
}

/**
Expand Down Expand Up @@ -293,24 +308,15 @@ export async function finalizeSpaceScopes(
}

if (perm.isSelfAuthority) {
perm = perm.withResolvedAuthority(
userDid as `did:${string}:${string}`,
);
perm = perm.withResolvedAuthority(userDid as `did:${string}:${string}`);
}

if (
!perm.hasCollections &&
perm.type !== "*" &&
resolveSpaceCollections
) {
if (!perm.hasCollections && perm.type !== "*" && resolveSpaceCollections) {
try {
const collections = await resolveSpaceCollections(perm.type);
if (collections && collections.length > 0) {
perm = perm.withDefaultCollections(
collections as readonly (
| "*"
| `${string}.${string}.${string}`
)[],
collections as readonly ("*" | `${string}.${string}.${string}`)[],
);
}
} catch {
Expand Down
15 changes: 15 additions & 0 deletions packages/oauth-provider/test/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import { base64url } from "jose";
import { parseScope, type ParseScopeOptions, ScopesSet } from "../src/scopes";

// ============================================
// DPoP Test Helpers
Expand Down Expand Up @@ -249,3 +250,17 @@ export async function generateClientKeyPair(

return result;
}

/*
* Parse a scope string into a ScopesSet
* @param scopes The scope string
* @param options The parse options
* @returns The ScopesSet
*/
export function parsedScopesSet(
scopes: string,
options?: ParseScopeOptions,
): ScopesSet {
const parsed = parseScope(scopes, options);
return ScopesSet.fromString(parsed);
}
Loading