From 09497873aaa201c8b77c2e35be50ad71f361d2cc Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Wed, 22 Jul 2026 08:10:14 +0200 Subject: [PATCH 1/3] fix: fail fast and explain 401s on group writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Group writes need the `workspaces:write` scope, which only ships with `tdc auth login --full-access`. A default login previously round-tripped to the API and surfaced a bare `Request failed with status 401`. Add `ensureScopeAllowed`, a per-method scope table checked before the request fires, so an under-scoped grant fails immediately with the command that fixes it. The guard fails open when the granted scope is unknown (`COMMS_API_TOKEN`, manually-saved tokens) — those may be session tokens, which bypass scope enforcement server-side. Also map Comms 401s onto an actionable `INVALID_TOKEN` error rather than letting the raw SDK message through, and document in the README and skill content that group writes require `--full-access`. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 2 + src/index.ts | 2 +- src/lib/api.test.ts | 39 ++++++++++++++++++- src/lib/api.ts | 20 +++++++--- src/lib/errors.ts | 11 ++++++ src/lib/permissions.test.ts | 78 ++++++++++++++++++++++++++++++++++++- src/lib/permissions.ts | 46 ++++++++++++++++++++++ src/lib/skills/content.ts | 4 +- 8 files changed, 192 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index b96c48e..854620e 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,8 @@ tdc auth login This opens Todoist OAuth in your browser. The default grant can read Comms data and create/update content and messages. It does not include delete, channel management, or user/workspace write scopes; use `--read-only` for read-only access or `--full-access` when needed. +All group management — `groups create`, `rename`, `delete`, `add-user`, `remove-user` — needs the `workspaces:write` scope, so it requires `tdc auth login --full-access`. Group reads (`groups`, `groups view`) work on a default login. + Once approved, the token is stored in your OS credential manager: - macOS: Keychain diff --git a/src/index.ts b/src/index.ts index 0133362..8ccdcbf 100644 --- a/src/index.ts +++ b/src/index.ts @@ -71,7 +71,7 @@ const commands: Record Promise<(p: Command) => void>]> = changelog: ['Show recent changelog entries', loadChangelogCommand], doctor: ['Diagnose common CLI setup and environment issues', loadDoctorCommand], groups: [ - 'Group operations (list, view, create, rename, delete, add-user, remove-user)', + 'Group operations (list, view, create, rename, delete, add-user, remove-user); writes need --full-access', loadGroupsCommand, ], config: ['Manage CLI configuration', loadConfigCommand], diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index 64b99f8..7425afa 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -7,12 +7,14 @@ const sdkMocks = vi.hoisted(() => ({ createClient: vi.fn(), deleteChannel: vi.fn(), uploadAttachment: vi.fn(), + addGroupUsers: vi.fn(), })) vi.mock('@doist/comms-sdk', () => { class CommsApi { channels = { deleteChannel: sdkMocks.deleteChannel } attachments = { upload: sdkMocks.uploadAttachment } + groups = { addUsers: sdkMocks.addGroupUsers } workspaceUsers = { getWorkspaceUsers: getWorkspaceUsersMock } constructor(token?: string, options?: unknown) { sdkMocks.createClient(token, options) @@ -51,8 +53,12 @@ vi.mock('./auth.js', () => ({ // reads (getWorkspaceUsers) stay off the write path. const permMocks = vi.hoisted(() => ({ ensureWriteAllowed: vi.fn().mockResolvedValue(undefined), + ensureScopeAllowed: vi.fn().mockResolvedValue(undefined), isMutatingMethod: vi.fn( - (path: string) => path === 'channels.deleteChannel' || path === 'attachments.upload', + (path: string) => + path === 'channels.deleteChannel' || + path === 'attachments.upload' || + path === 'groups.addUsers', ), })) vi.mock('./permissions.js', () => permMocks) @@ -125,7 +131,9 @@ describe('wrapResult — central 403 translation', () => { sdkMocks.createClient.mockReset() sdkMocks.deleteChannel.mockReset() sdkMocks.uploadAttachment.mockReset() + sdkMocks.addGroupUsers.mockReset() permMocks.ensureWriteAllowed.mockReset().mockResolvedValue(undefined) + permMocks.ensureScopeAllowed.mockReset().mockResolvedValue(undefined) }) it('uses the explicit base URL when creating the wrapped SDK client', () => { @@ -212,6 +220,35 @@ describe('wrapResult — central 403 translation', () => { expect(sdkMocks.uploadAttachment).not.toHaveBeenCalled() }) + it('routes group membership writes through the scope-guard', async () => { + permMocks.ensureScopeAllowed.mockRejectedValueOnce(new Error('INSUFFICIENT_SCOPE')) + const client = createWrappedCommsClient('test-token') + + await expect( + client.groups.addUsers({ id: 'G1', workspaceId: 69, userIds: [1] }), + ).rejects.toThrow('INSUFFICIENT_SCOPE') + // The guard runs before the request, so nothing hits the network. + expect(sdkMocks.addGroupUsers).not.toHaveBeenCalled() + expect(permMocks.ensureScopeAllowed).toHaveBeenCalledWith('groups.addUsers') + }) + + it('translates a 401 into INVALID_TOKEN with re-auth guidance', async () => { + sdkMocks.addGroupUsers.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 401', 401, { + error_string: 'Invalid token', + error_code: 200, + }), + ) + const client = createWrappedCommsClient('test-token') + + await expect( + client.groups.addUsers({ id: 'G1', workspaceId: 69, userIds: [1] }), + ).rejects.toMatchObject({ + code: 'INVALID_TOKEN', + message: 'Comms rejected the token: 401.', + }) + }) + it('passes non-403 errors through untranslated', async () => { const originalError = new CommsRequestError('Request failed with status 500', 500, {}) sdkMocks.deleteChannel.mockRejectedValueOnce(originalError) diff --git a/src/lib/api.ts b/src/lib/api.ts index 603045e..1dd9ce2 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -7,8 +7,8 @@ import { } from '@doist/comms-sdk' import { getApiTokenSnapshot } from './auth.js' import { getConfig, updateConfig } from './config.js' -import { CliError, isForbidden, isInsufficientScope } from './errors.js' -import { ensureWriteAllowed, isMutatingMethod } from './permissions.js' +import { CliError, isForbidden, isInsufficientScope, isInvalidToken } from './errors.js' +import { ensureScopeAllowed, ensureWriteAllowed, isMutatingMethod } from './permissions.js' import { getProgressTracker } from './progress.js' import { withSpinner } from './spinner.js' @@ -158,10 +158,12 @@ function createNestedSpinnerProxy(obj: T, basePath: string): T // For mutating methods, check permissions before calling the API if (shouldCheckPermissions) { - return ensureWriteAllowed().then(() => { - const result = originalMethod.apply(target, args) - return wrapResult(result, progressTracker, spinnerConfig) - }) + return ensureWriteAllowed() + .then(() => ensureScopeAllowed(fullPath)) + .then(() => { + const result = originalMethod.apply(target, args) + return wrapResult(result, progressTracker, spinnerConfig) + }) } const result = originalMethod.apply(target, args) @@ -208,6 +210,12 @@ function wrapResult( 'Contact your workspace admin, or re-authenticate with `tdc auth login` if your token looks wrong', ]) } + if (isInvalidToken(error)) { + throw new CliError('INVALID_TOKEN', 'Comms rejected the token: 401.', [ + 'Re-authenticate with `tdc auth login`, then check `tdc auth status`', + 'Group and workspace writes need `tdc auth login --full-access`', + ]) + } throw error }) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index f4dea5c..7aaea7c 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -106,6 +106,17 @@ export function isForbidden(error: unknown): boolean { return hasCommsStatusCode(error, 403) && !isInsufficientScope(error) } +/** + * Check whether an error is a Comms API 401. Comms returns this both for a + * genuinely bad token and — because `_raise_todoist_rest_error` maps an + * upstream Todoist `UNAUTHORIZED` onto `INVALID_TOKEN` — for a valid token that + * lacks the scope a proxied workspace/group write needs. The two are + * indistinguishable on the wire, so the hint covers both. + */ +export function isInvalidToken(error: unknown): boolean { + return hasCommsStatusCode(error, 401) +} + /** * Comms-flavoured CliError that preserves the historical positional * `(code, message, hints?, type?)` signature used across hundreds of call diff --git a/src/lib/permissions.test.ts b/src/lib/permissions.test.ts index d2a3bf8..eca3c84 100644 --- a/src/lib/permissions.test.ts +++ b/src/lib/permissions.test.ts @@ -5,7 +5,12 @@ vi.mock('./auth.js', () => ({ })) import { getAuthMetadata } from './auth.js' -import { ensureWriteAllowed, isMutatingMethod, READ_ONLY_ERROR_MESSAGE } from './permissions.js' +import { + ensureScopeAllowed, + ensureWriteAllowed, + isMutatingMethod, + READ_ONLY_ERROR_MESSAGE, +} from './permissions.js' const mockGetAuthMetadata = vi.mocked(getAuthMetadata) @@ -75,3 +80,74 @@ describe('permissions', () => { expect(isMutatingMethod('someNewApi.newMethod')).toBe(true) }) }) + +describe('ensureScopeAllowed', () => { + it('blocks group writes when the grant lacks workspaces:write', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-write', + authScope: 'user:read workspaces:read comms:content:write', + source: 'config', + }) + + await expect(ensureScopeAllowed('groups.addUsers')).rejects.toThrow('workspaces:write') + }) + + it('allows group writes when workspaces:write is granted', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-write', + authScope: 'user:read workspaces:read workspaces:write', + source: 'config', + }) + + await expect(ensureScopeAllowed('groups.addUsers')).resolves.toBeUndefined() + }) + + it('matches whole scopes, not substrings', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-write', + // `workspaces:write` is a prefix of nothing here, but a naive + // `includes` on the raw string would pass on `workspaces:write:x`. + authScope: 'workspaces:write:something-else', + source: 'config', + }) + + await expect(ensureScopeAllowed('groups.addUsers')).rejects.toThrow('workspaces:write') + }) + + it('fails open when the granted scope is unknown (env token)', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'unknown', + source: 'env', + }) + + await expect(ensureScopeAllowed('groups.addUsers')).resolves.toBeUndefined() + }) + + it('ignores methods with no declared scope requirement', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-write', + authScope: 'comms:content:write', + source: 'config', + }) + + await expect(ensureScopeAllowed('comments.createComment')).resolves.toBeUndefined() + }) + + it('covers every group write, not just membership changes', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-write', + authScope: 'comms:content:write', + source: 'config', + }) + + for (const method of [ + 'groups.createGroup', + 'groups.updateGroup', + 'groups.deleteGroup', + 'groups.addUsers', + 'groups.removeUsers', + ]) { + await expect(ensureScopeAllowed(method)).rejects.toThrow('workspaces:write') + } + }) +}) diff --git a/src/lib/permissions.ts b/src/lib/permissions.ts index 1d6c94d..47665e5 100644 --- a/src/lib/permissions.ts +++ b/src/lib/permissions.ts @@ -31,6 +31,24 @@ const KNOWN_SAFE_API_METHODS = new Set([ 'batch', ]) +/** + * OAuth scopes Comms requires for API methods whose scope is *not* covered by + * the default write grant. `workspaces:write` ships only with + * `tdc auth login --full-access`, so without this table a default login fails + * with an opaque server round-trip instead of an immediate, fixable error. + * + * Channel writes (`comms:channels:write` / `:delete`) are deliberately absent: + * they already surface a clean 403 "Insufficient scope" from Comms, which + * `wrapResult` turns into the same guidance. + */ +const API_METHOD_SCOPES: Record = { + 'groups.createGroup': 'workspaces:write', + 'groups.updateGroup': 'workspaces:write', + 'groups.deleteGroup': 'workspaces:write', + 'groups.addUsers': 'workspaces:write', + 'groups.removeUsers': 'workspaces:write', +} + export function isMutatingMethod(methodPath: string): boolean { return !KNOWN_SAFE_API_METHODS.has(methodPath) } @@ -43,3 +61,31 @@ export async function ensureWriteAllowed(): Promise { ]) } } + +/** + * Fail fast when the stored grant is missing a scope the method needs. + * + * Fails *open* whenever the granted scope is unknown — `COMMS_API_TOKEN` and + * manually-saved tokens carry no scope metadata, and may be session tokens, + * which bypass Comms' scope enforcement entirely. Blocking those would break + * working setups to guess at an error the server is better placed to raise. + */ +export async function ensureScopeAllowed(methodPath: string): Promise { + const requiredScope = API_METHOD_SCOPES[methodPath] + if (!requiredScope) return + + const metadata = await getAuthMetadata() + const grantedScope = metadata.authScope + if (!grantedScope) return + + if (!grantedScope.split(/\s+/).includes(requiredScope)) { + throw new CliError( + 'INSUFFICIENT_SCOPE', + `This action requires the \`${requiredScope}\` scope, which your token does not have.`, + [ + 'Re-run: tdc auth login --full-access', + 'Check the granted scopes with: tdc auth status', + ], + ) + } +} diff --git a/src/lib/skills/content.ts b/src/lib/skills/content.ts index f8b005a..a8f8012 100644 --- a/src/lib/skills/content.ts +++ b/src/lib/skills/content.ts @@ -50,7 +50,7 @@ tdc changelog # Show recent changelog entries tdc migrate urls # Translate old twist.com URLs to Comms URLs (needs a Twist token) \`\`\` -OAuth login uses Todoist OAuth for Comms access. The default grant can read Comms data and create/update content or messages. It does not include delete, channel management, or user/workspace write scopes; use \`tdc auth login --full-access\` only when needed. Stored auth uses the system credential manager when available. If secure storage is unavailable, \`tdc\` warns and falls back to \`~/.config/comms-cli/config.json\`. \`COMMS_API_TOKEN\` always takes priority over the stored token. +OAuth login uses Todoist OAuth for Comms access. The default grant can read Comms data and create/update content or messages. It does not include delete, channel management, or user/workspace write scopes; use \`tdc auth login --full-access\` only when needed (all \`tdc groups\` writes require it). Stored auth uses the system credential manager when available. If secure storage is unavailable, \`tdc\` warns and falls back to \`~/.config/comms-cli/config.json\`. \`COMMS_API_TOKEN\` always takes priority over the stored token. In read-only mode (\`tdc auth login --read-only\`), commands that modify Comms data (reply, archive, react, delete, etc.) are blocked by the CLI. Externally provided tokens (\`COMMS_API_TOKEN\` or \`tdc auth token\`) are treated as unknown scope and assumed write-capable. @@ -288,6 +288,8 @@ tdc groups remove-user user1 user2 # Remove users from a group tdc groups remove-user id:123,id:456 # Comma-separated ID refs \`\`\` +All group *writes* (\`groups create\`, \`rename\`, \`delete\`, \`add-user\`, \`remove-user\`) need the \`workspaces:write\` scope, which only \`tdc auth login --full-access\` grants. Group *reads* (\`groups\`, \`groups view\`) work on a default login. + If a channel is not found in \`tdc channels\`, widen with broader listings such as \`tdc channels --scope public\`, then \`tdc channels --scope public --state all\`. Check \`tdc channels --help\` for other available filters. \`tdc channel threads\` returns every thread in the channel; pagination filters (\`--limit\`, \`--cursor\`, \`--since\`, \`--until\`, \`--unread\`) are applied client-side after fetch. \`--archive-filter\` is applied server-side. Results are sorted newest-first by last activity. In \`--json\` / \`--ndjson\`, the response includes a \`nextCursor\` string (opaque) you can pass via \`--cursor\` to fetch the next page; NDJSON emits the cursor as a final \`{ "_meta": true, "nextCursor": "..." }\` line. From 488dd370cca3f969265a5a7a7fccbfdf11c8933a Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Wed, 22 Jul 2026 08:26:35 +0200 Subject: [PATCH 2/3] refactor: address review feedback on the group scope guard - Regenerate `skills/comms-cli/SKILL.md`; `check:skill-sync` compares it byte-for-byte against the built content and was failing. - Gate the scope hint on 401s to methods that declare a required scope. Every call routes through `wrapResult`, reads included, so an expired token on a read was being blamed on group/workspace scopes. - Add `ensureMutationAllowed`, so the write and scope checks share one `getAuthMetadata()` call. `getConfig()` is uncached, so the two guards were doing two disk reads per mutating call. - Extract `splitScopeString` into `scopes.ts` and reuse it, rather than hand-rolling a second parser that missed comma-delimited grants. It lives in its own module because `permissions` importing `auth-provider` would close an `auth-provider` -> `api` -> `permissions` cycle. - Cover `isInvalidToken` in `errors.test.ts` alongside the sibling predicates, and assert the full `hints` array on the 401 tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- skills/comms-cli/SKILL.md | 4 ++- src/lib/api.test.ts | 41 +++++++++++++++++++------- src/lib/api.ts | 23 +++++++++------ src/lib/auth-provider.ts | 9 +----- src/lib/errors.test.ts | 37 ++++++++++++++++++++++- src/lib/permissions.test.ts | 58 +++++++++++++++++++++++++++++++++++-- src/lib/permissions.ts | 34 +++++++++++++++++----- src/lib/scopes.ts | 28 ++++++++++++++++++ 8 files changed, 195 insertions(+), 39 deletions(-) create mode 100644 src/lib/scopes.ts diff --git a/skills/comms-cli/SKILL.md b/skills/comms-cli/SKILL.md index 3096c84..fd493fe 100644 --- a/skills/comms-cli/SKILL.md +++ b/skills/comms-cli/SKILL.md @@ -46,7 +46,7 @@ tdc changelog # Show recent changelog entries tdc migrate urls # Translate old twist.com URLs to Comms URLs (needs a Twist token) ``` -OAuth login uses Todoist OAuth for Comms access. The default grant can read Comms data and create/update content or messages. It does not include delete, channel management, or user/workspace write scopes; use `tdc auth login --full-access` only when needed. Stored auth uses the system credential manager when available. If secure storage is unavailable, `tdc` warns and falls back to `~/.config/comms-cli/config.json`. `COMMS_API_TOKEN` always takes priority over the stored token. +OAuth login uses Todoist OAuth for Comms access. The default grant can read Comms data and create/update content or messages. It does not include delete, channel management, or user/workspace write scopes; use `tdc auth login --full-access` only when needed (all `tdc groups` writes require it). Stored auth uses the system credential manager when available. If secure storage is unavailable, `tdc` warns and falls back to `~/.config/comms-cli/config.json`. `COMMS_API_TOKEN` always takes priority over the stored token. In read-only mode (`tdc auth login --read-only`), commands that modify Comms data (reply, archive, react, delete, etc.) are blocked by the CLI. Externally provided tokens (`COMMS_API_TOKEN` or `tdc auth token`) are treated as unknown scope and assumed write-capable. @@ -284,6 +284,8 @@ tdc groups remove-user user1 user2 # Remove users from a group tdc groups remove-user id:123,id:456 # Comma-separated ID refs ``` +All group *writes* (`groups create`, `rename`, `delete`, `add-user`, `remove-user`) need the `workspaces:write` scope, which only `tdc auth login --full-access` grants. Group *reads* (`groups`, `groups view`) work on a default login. + If a channel is not found in `tdc channels`, widen with broader listings such as `tdc channels --scope public`, then `tdc channels --scope public --state all`. Check `tdc channels --help` for other available filters. `tdc channel threads` returns every thread in the channel; pagination filters (`--limit`, `--cursor`, `--since`, `--until`, `--unread`) are applied client-side after fetch. `--archive-filter` is applied server-side. Results are sorted newest-first by last activity. In `--json` / `--ndjson`, the response includes a `nextCursor` string (opaque) you can pass via `--cursor` to fetch the next page; NDJSON emits the cursor as a final `{ "_meta": true, "nextCursor": "..." }` line. diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index 7425afa..9f59e7c 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -52,8 +52,10 @@ vi.mock('./auth.js', () => ({ // exercise `channels.deleteChannel` and `attachments.upload` as mutating; // reads (getWorkspaceUsers) stay off the write path. const permMocks = vi.hoisted(() => ({ - ensureWriteAllowed: vi.fn().mockResolvedValue(undefined), - ensureScopeAllowed: vi.fn().mockResolvedValue(undefined), + ensureMutationAllowed: vi.fn().mockResolvedValue(undefined), + getRequiredScope: vi.fn((path: string) => + path === 'groups.addUsers' ? 'workspaces:write' : undefined, + ), isMutatingMethod: vi.fn( (path: string) => path === 'channels.deleteChannel' || @@ -132,8 +134,7 @@ describe('wrapResult — central 403 translation', () => { sdkMocks.deleteChannel.mockReset() sdkMocks.uploadAttachment.mockReset() sdkMocks.addGroupUsers.mockReset() - permMocks.ensureWriteAllowed.mockReset().mockResolvedValue(undefined) - permMocks.ensureScopeAllowed.mockReset().mockResolvedValue(undefined) + permMocks.ensureMutationAllowed.mockReset().mockResolvedValue(undefined) }) it('uses the explicit base URL when creating the wrapped SDK client', () => { @@ -187,7 +188,7 @@ describe('wrapResult — central 403 translation', () => { }) it('translates an attachments.upload scope 403 into INSUFFICIENT_SCOPE (re-login prompt)', async () => { - permMocks.ensureWriteAllowed.mockResolvedValue(undefined) + permMocks.ensureMutationAllowed.mockResolvedValue(undefined) sdkMocks.uploadAttachment.mockRejectedValueOnce( new CommsRequestError('Request failed with status 403', 403, { error_string: 'Insufficient scope provided: attachments:write', @@ -206,11 +207,11 @@ describe('wrapResult — central 403 translation', () => { ], }) // Confirms upload runs through the mutating write-guard. - expect(permMocks.ensureWriteAllowed).toHaveBeenCalled() + expect(permMocks.ensureMutationAllowed).toHaveBeenCalled() }) it('routes attachments.upload through the write-guard, blocking it in read-only mode', async () => { - permMocks.ensureWriteAllowed.mockRejectedValueOnce(new Error('READ_ONLY')) + permMocks.ensureMutationAllowed.mockRejectedValueOnce(new Error('READ_ONLY')) const client = createWrappedCommsClient('test-token') await expect( @@ -220,8 +221,8 @@ describe('wrapResult — central 403 translation', () => { expect(sdkMocks.uploadAttachment).not.toHaveBeenCalled() }) - it('routes group membership writes through the scope-guard', async () => { - permMocks.ensureScopeAllowed.mockRejectedValueOnce(new Error('INSUFFICIENT_SCOPE')) + it('routes group membership writes through the mutation guard', async () => { + permMocks.ensureMutationAllowed.mockRejectedValueOnce(new Error('INSUFFICIENT_SCOPE')) const client = createWrappedCommsClient('test-token') await expect( @@ -229,10 +230,10 @@ describe('wrapResult — central 403 translation', () => { ).rejects.toThrow('INSUFFICIENT_SCOPE') // The guard runs before the request, so nothing hits the network. expect(sdkMocks.addGroupUsers).not.toHaveBeenCalled() - expect(permMocks.ensureScopeAllowed).toHaveBeenCalledWith('groups.addUsers') + expect(permMocks.ensureMutationAllowed).toHaveBeenCalledWith('groups.addUsers') }) - it('translates a 401 into INVALID_TOKEN with re-auth guidance', async () => { + it('translates a 401 into INVALID_TOKEN, adding the scope hint on scoped methods', async () => { sdkMocks.addGroupUsers.mockRejectedValueOnce( new CommsRequestError('Request failed with status 401', 401, { error_string: 'Invalid token', @@ -246,6 +247,24 @@ describe('wrapResult — central 403 translation', () => { ).rejects.toMatchObject({ code: 'INVALID_TOKEN', message: 'Comms rejected the token: 401.', + hints: [ + 'Re-authenticate with `tdc auth login`, then check `tdc auth status`', + 'This action needs `workspaces:write`: tdc auth login --full-access', + ], + }) + }) + + it('omits the scope hint on a 401 from a method that needs no extra scope', async () => { + // Every call routes through wrapResult, reads included — a 401 on an + // expired token must not blame group/workspace scopes. + sdkMocks.deleteChannel.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 401', 401, {}), + ) + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({ + code: 'INVALID_TOKEN', + hints: ['Re-authenticate with `tdc auth login`, then check `tdc auth status`'], }) }) diff --git a/src/lib/api.ts b/src/lib/api.ts index 1dd9ce2..a621f5d 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -8,7 +8,7 @@ import { import { getApiTokenSnapshot } from './auth.js' import { getConfig, updateConfig } from './config.js' import { CliError, isForbidden, isInsufficientScope, isInvalidToken } from './errors.js' -import { ensureScopeAllowed, ensureWriteAllowed, isMutatingMethod } from './permissions.js' +import { ensureMutationAllowed, getRequiredScope, isMutatingMethod } from './permissions.js' import { getProgressTracker } from './progress.js' import { withSpinner } from './spinner.js' @@ -158,16 +158,14 @@ function createNestedSpinnerProxy(obj: T, basePath: string): T // For mutating methods, check permissions before calling the API if (shouldCheckPermissions) { - return ensureWriteAllowed() - .then(() => ensureScopeAllowed(fullPath)) - .then(() => { - const result = originalMethod.apply(target, args) - return wrapResult(result, progressTracker, spinnerConfig) - }) + return ensureMutationAllowed(fullPath).then(() => { + const result = originalMethod.apply(target, args) + return wrapResult(result, progressTracker, spinnerConfig, fullPath) + }) } const result = originalMethod.apply(target, args) - return wrapResult(result, progressTracker, spinnerConfig) + return wrapResult(result, progressTracker, spinnerConfig, fullPath) } }, }) @@ -177,6 +175,7 @@ function wrapResult( result: unknown, progressTracker: ReturnType, spinnerConfig: (typeof API_SPINNER_MESSAGES)[string] | undefined, + methodPath?: string, ): unknown { // If the method returns a non-thenable, return as-is. if (!result || typeof (result as { then?: unknown }).then !== 'function') { @@ -211,9 +210,15 @@ function wrapResult( ]) } if (isInvalidToken(error)) { + // Re-authenticating is the fix for any 401. The scope hint only + // belongs on methods that actually need an extra scope — every + // call routes through here, reads included. + const requiredScope = methodPath ? getRequiredScope(methodPath) : undefined throw new CliError('INVALID_TOKEN', 'Comms rejected the token: 401.', [ 'Re-authenticate with `tdc auth login`, then check `tdc auth status`', - 'Group and workspace writes need `tdc auth login --full-access`', + ...(requiredScope + ? [`This action needs \`${requiredScope}\`: tdc auth login --full-access`] + : []), ]) } throw error diff --git a/src/lib/auth-provider.ts b/src/lib/auth-provider.ts index 6122ef1..f9417e9 100644 --- a/src/lib/auth-provider.ts +++ b/src/lib/auth-provider.ts @@ -25,6 +25,7 @@ import { } from './config.js' import { CliError } from './errors.js' import { parseRef } from './refs.js' +import { splitScopeString } from './scopes.js' import { createCommsUserRecordStore, getDefaultUserRecord } from './user-records.js' const DEFAULT_TODOIST_AUTH_BASE_URL = 'https://todoist.com' @@ -517,14 +518,6 @@ function normalizeScopeString(scope: string): string { return splitScopeString(scope).join(' ') } -function splitScopeString(scope: string): string[] { - return scope - .replaceAll(',', ' ') - .split(/\s+/) - .map((part) => part.trim()) - .filter(Boolean) -} - /** * Accepts `42`, `id:42`, and case-insensitive labels — `parseRef` normalises * the numeric forms. Broader than cli-core's default strict-equality matcher. diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index d383f68..285b74e 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -1,7 +1,7 @@ import { CommsRequestError } from '@doist/comms-sdk' import { describe, expect, it } from 'vitest' -import { isForbidden, isInsufficientScope } from './errors.js' +import { isForbidden, isInsufficientScope, isInvalidToken } from './errors.js' describe('isInsufficientScope', () => { it('returns true for a 403 with "Insufficient scope" error_string', () => { @@ -83,3 +83,38 @@ describe('isForbidden', () => { expect(isForbidden(error)).toBe(false) }) }) + +describe('isInvalidToken', () => { + it('returns true for a 401 regardless of body', () => { + expect( + isInvalidToken(new CommsRequestError('Request failed with status 401', 401, {})), + ).toBe(true) + expect( + isInvalidToken( + new CommsRequestError('Request failed with status 401', 401, { + error_code: 200, + error_string: 'Invalid token', + }), + ), + ).toBe(true) + }) + + it('returns false for non-401 status codes', () => { + expect( + isInvalidToken(new CommsRequestError('Request failed with status 403', 403, {})), + ).toBe(false) + expect( + isInvalidToken(new CommsRequestError('Request failed with status 404', 404, {})), + ).toBe(false) + expect( + isInvalidToken(new CommsRequestError('Request failed with status 500', 500, {})), + ).toBe(false) + }) + + it('returns false for plain errors and non-object values', () => { + expect(isInvalidToken(new Error('something'))).toBe(false) + expect(isInvalidToken(null)).toBe(false) + expect(isInvalidToken(undefined)).toBe(false) + expect(isInvalidToken('string')).toBe(false) + }) +}) diff --git a/src/lib/permissions.test.ts b/src/lib/permissions.test.ts index eca3c84..ac93fb1 100644 --- a/src/lib/permissions.test.ts +++ b/src/lib/permissions.test.ts @@ -6,6 +6,7 @@ vi.mock('./auth.js', () => ({ import { getAuthMetadata } from './auth.js' import { + ensureMutationAllowed, ensureScopeAllowed, ensureWriteAllowed, isMutatingMethod, @@ -105,8 +106,7 @@ describe('ensureScopeAllowed', () => { it('matches whole scopes, not substrings', async () => { mockGetAuthMetadata.mockResolvedValue({ authMode: 'read-write', - // `workspaces:write` is a prefix of nothing here, but a naive - // `includes` on the raw string would pass on `workspaces:write:x`. + // A naive `includes` on the raw string would pass on this. authScope: 'workspaces:write:something-else', source: 'config', }) @@ -114,6 +114,23 @@ describe('ensureScopeAllowed', () => { await expect(ensureScopeAllowed('groups.addUsers')).rejects.toThrow('workspaces:write') }) + it('accepts comma-delimited scope strings', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-write', + authScope: 'workspaces:write,comms:content:write', + source: 'config', + }) + + await expect(ensureScopeAllowed('groups.addUsers')).resolves.toBeUndefined() + }) + + it('skips the auth-metadata lookup for methods needing no extra scope', async () => { + mockGetAuthMetadata.mockClear() + + await expect(ensureScopeAllowed('comments.createComment')).resolves.toBeUndefined() + expect(mockGetAuthMetadata).not.toHaveBeenCalled() + }) + it('fails open when the granted scope is unknown (env token)', async () => { mockGetAuthMetadata.mockResolvedValue({ authMode: 'unknown', @@ -151,3 +168,40 @@ describe('ensureScopeAllowed', () => { } }) }) + +describe('ensureMutationAllowed', () => { + it('reads auth metadata once for both checks', async () => { + mockGetAuthMetadata.mockClear() + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-write', + authScope: 'workspaces:write', + source: 'config', + }) + + await expect(ensureMutationAllowed('groups.addUsers')).resolves.toBeUndefined() + // getAuthMetadata hits the config file uncached, so one read, not two. + expect(mockGetAuthMetadata).toHaveBeenCalledTimes(1) + }) + + it('rejects read-only mode before checking scopes', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-only', + authScope: 'workspaces:read', + source: 'config', + }) + + await expect(ensureMutationAllowed('groups.addUsers')).rejects.toThrow( + READ_ONLY_ERROR_MESSAGE, + ) + }) + + it('rejects an under-scoped grant', async () => { + mockGetAuthMetadata.mockResolvedValue({ + authMode: 'read-write', + authScope: 'comms:content:write', + source: 'config', + }) + + await expect(ensureMutationAllowed('groups.addUsers')).rejects.toThrow('workspaces:write') + }) +}) diff --git a/src/lib/permissions.ts b/src/lib/permissions.ts index 47665e5..22a441a 100644 --- a/src/lib/permissions.ts +++ b/src/lib/permissions.ts @@ -1,5 +1,6 @@ -import { getAuthMetadata } from './auth.js' +import { type AuthMetadata, getAuthMetadata } from './auth.js' import { CliError } from './errors.js' +import { hasScope } from './scopes.js' export const READ_ONLY_ERROR_MESSAGE = 'This CLI is authenticated in read-only mode. Re-run `tdc auth login` without --read-only to enable write operations.' @@ -53,8 +54,13 @@ export function isMutatingMethod(methodPath: string): boolean { return !KNOWN_SAFE_API_METHODS.has(methodPath) } -export async function ensureWriteAllowed(): Promise { - const metadata = await getAuthMetadata() +/** The scope `methodPath` needs beyond the default write grant, if any. */ +export function getRequiredScope(methodPath: string): string | undefined { + return API_METHOD_SCOPES[methodPath] +} + +export async function ensureWriteAllowed(preloaded?: AuthMetadata): Promise { + const metadata = preloaded ?? (await getAuthMetadata()) if (metadata.authMode === 'read-only') { throw new CliError('READ_ONLY', READ_ONLY_ERROR_MESSAGE, [ 'Re-run: tdc auth login (without --read-only)', @@ -70,15 +76,19 @@ export async function ensureWriteAllowed(): Promise { * which bypass Comms' scope enforcement entirely. Blocking those would break * working setups to guess at an error the server is better placed to raise. */ -export async function ensureScopeAllowed(methodPath: string): Promise { - const requiredScope = API_METHOD_SCOPES[methodPath] +export async function ensureScopeAllowed( + methodPath: string, + preloaded?: AuthMetadata, +): Promise { + const requiredScope = getRequiredScope(methodPath) + // Bail before touching auth metadata: most methods declare no extra scope. if (!requiredScope) return - const metadata = await getAuthMetadata() + const metadata = preloaded ?? (await getAuthMetadata()) const grantedScope = metadata.authScope if (!grantedScope) return - if (!grantedScope.split(/\s+/).includes(requiredScope)) { + if (!hasScope(grantedScope, requiredScope)) { throw new CliError( 'INSUFFICIENT_SCOPE', `This action requires the \`${requiredScope}\` scope, which your token does not have.`, @@ -89,3 +99,13 @@ export async function ensureScopeAllowed(methodPath: string): Promise { ) } } + +/** + * Single guard for a mutating call: one `getAuthMetadata()` read shared by both + * checks, since it hits the config file on every invocation and is uncached. + */ +export async function ensureMutationAllowed(methodPath: string): Promise { + const metadata = await getAuthMetadata() + await ensureWriteAllowed(metadata) + await ensureScopeAllowed(methodPath, metadata) +} diff --git a/src/lib/scopes.ts b/src/lib/scopes.ts new file mode 100644 index 0000000..7306dc7 --- /dev/null +++ b/src/lib/scopes.ts @@ -0,0 +1,28 @@ +/** + * OAuth scope-string parsing, shared by the auth provider (which records the + * server-granted scope) and the permission guards (which check it). + * + * Lives in its own module rather than in `auth-provider.ts` because + * `permissions.ts` needs it too, and `auth-provider` → `api` → `permissions` + * would close an import cycle. + */ + +/** + * Split a scope string into its individual scope codes. + * + * Scope strings are space-delimited per RFC 6749, but commas are tolerated + * because some issuers emit them; normalising here keeps every consumer's + * comparison honest. + */ +export function splitScopeString(scope: string): string[] { + return scope + .replaceAll(',', ' ') + .split(/\s+/) + .map((part) => part.trim()) + .filter(Boolean) +} + +/** Whether a granted scope string contains `scope` as a whole scope code. */ +export function hasScope(grantedScope: string, scope: string): boolean { + return splitScopeString(grantedScope).includes(scope) +} From b4b3c526975ec2f5becc14820f96c6b035cb3416 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Wed, 22 Jul 2026 08:38:44 +0200 Subject: [PATCH 3/3] fix: drop the scope hint from 401 errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An under-scoped grant is a 403 `Insufficient scope`, which has its own branch above — a 401 only ever means the token is bad or expired, so naming a required scope there is wrong rather than merely imprecise. The pre-flight guard already catches an under-scoped grant locally before any request is made. Removes the `methodPath` plumbing threaded through `wrapResult`, which existed only to gate that hint. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/api.test.ts | 14 +++----------- src/lib/api.ts | 17 ++++++----------- 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index 9f59e7c..c00a7dd 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -53,9 +53,6 @@ vi.mock('./auth.js', () => ({ // reads (getWorkspaceUsers) stay off the write path. const permMocks = vi.hoisted(() => ({ ensureMutationAllowed: vi.fn().mockResolvedValue(undefined), - getRequiredScope: vi.fn((path: string) => - path === 'groups.addUsers' ? 'workspaces:write' : undefined, - ), isMutatingMethod: vi.fn( (path: string) => path === 'channels.deleteChannel' || @@ -233,7 +230,7 @@ describe('wrapResult — central 403 translation', () => { expect(permMocks.ensureMutationAllowed).toHaveBeenCalledWith('groups.addUsers') }) - it('translates a 401 into INVALID_TOKEN, adding the scope hint on scoped methods', async () => { + it('translates a 401 into INVALID_TOKEN with re-auth guidance', async () => { sdkMocks.addGroupUsers.mockRejectedValueOnce( new CommsRequestError('Request failed with status 401', 401, { error_string: 'Invalid token', @@ -247,16 +244,11 @@ describe('wrapResult — central 403 translation', () => { ).rejects.toMatchObject({ code: 'INVALID_TOKEN', message: 'Comms rejected the token: 401.', - hints: [ - 'Re-authenticate with `tdc auth login`, then check `tdc auth status`', - 'This action needs `workspaces:write`: tdc auth login --full-access', - ], + hints: ['Re-authenticate with `tdc auth login`, then check `tdc auth status`'], }) }) - it('omits the scope hint on a 401 from a method that needs no extra scope', async () => { - // Every call routes through wrapResult, reads included — a 401 on an - // expired token must not blame group/workspace scopes. + it('gives the same 401 guidance on reads, which also route through wrapResult', async () => { sdkMocks.deleteChannel.mockRejectedValueOnce( new CommsRequestError('Request failed with status 401', 401, {}), ) diff --git a/src/lib/api.ts b/src/lib/api.ts index a621f5d..c66aa71 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -8,7 +8,7 @@ import { import { getApiTokenSnapshot } from './auth.js' import { getConfig, updateConfig } from './config.js' import { CliError, isForbidden, isInsufficientScope, isInvalidToken } from './errors.js' -import { ensureMutationAllowed, getRequiredScope, isMutatingMethod } from './permissions.js' +import { ensureMutationAllowed, isMutatingMethod } from './permissions.js' import { getProgressTracker } from './progress.js' import { withSpinner } from './spinner.js' @@ -160,12 +160,12 @@ function createNestedSpinnerProxy(obj: T, basePath: string): T if (shouldCheckPermissions) { return ensureMutationAllowed(fullPath).then(() => { const result = originalMethod.apply(target, args) - return wrapResult(result, progressTracker, spinnerConfig, fullPath) + return wrapResult(result, progressTracker, spinnerConfig) }) } const result = originalMethod.apply(target, args) - return wrapResult(result, progressTracker, spinnerConfig, fullPath) + return wrapResult(result, progressTracker, spinnerConfig) } }, }) @@ -175,7 +175,6 @@ function wrapResult( result: unknown, progressTracker: ReturnType, spinnerConfig: (typeof API_SPINNER_MESSAGES)[string] | undefined, - methodPath?: string, ): unknown { // If the method returns a non-thenable, return as-is. if (!result || typeof (result as { then?: unknown }).then !== 'function') { @@ -210,15 +209,11 @@ function wrapResult( ]) } if (isInvalidToken(error)) { - // Re-authenticating is the fix for any 401. The scope hint only - // belongs on methods that actually need an extra scope — every - // call routes through here, reads included. - const requiredScope = methodPath ? getRequiredScope(methodPath) : undefined + // A 401 means the token itself is bad or expired. An + // under-scoped grant is a 403 `Insufficient scope`, handled + // above — so re-authenticating is the whole fix here. throw new CliError('INVALID_TOKEN', 'Comms rejected the token: 401.', [ 'Re-authenticate with `tdc auth login`, then check `tdc auth status`', - ...(requiredScope - ? [`This action needs \`${requiredScope}\`: tdc auth login --full-access`] - : []), ]) } throw error