diff --git a/packages/cli/src/commands/assets/actions.ts b/packages/cli/src/commands/assets/actions.ts index 4aa05f894..23b765174 100644 --- a/packages/cli/src/commands/assets/actions.ts +++ b/packages/cli/src/commands/assets/actions.ts @@ -512,7 +512,7 @@ export const fetchSharedAssetFolders = async ({ } return { asset_folders: all.filter((folder) => keep.has(folder.id)) }; } catch (maybeError) { - handleAPIError("pull_shared_asset_folders", toError(maybeError)); + handleAPIError("list_shared_asset_folders", toError(maybeError)); } }; diff --git a/packages/cli/src/commands/assets/pull/index.test.ts b/packages/cli/src/commands/assets/pull/index.test.ts index ce78e2ee1..b4ba2b6b1 100644 --- a/packages/cli/src/commands/assets/pull/index.test.ts +++ b/packages/cli/src/commands/assets/pull/index.test.ts @@ -143,6 +143,26 @@ const preconditions = { ), ); }, + forbidsLibraryDiscovery() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json( + { error: "This endpoint does not support this token type" }, + { status: 403 }, + ), + ), + ); + }, + forbidsLibraryDiscoveryForAnotherReason() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json( + { error: "This token is restricted to specific spaces" }, + { status: 403 }, + ), + ), + ); + }, hasLocalStoriesReferencing(assetIds: number[]) { vol.fromJSON({ ".storyblok/stories/12345/home_uuid.json": JSON.stringify({ @@ -589,6 +609,72 @@ describe("assets pull command", () => { expect(sharedSpy).not.toHaveBeenCalled(); }); + it("should pull space assets and warn when referenced-library discovery is forbidden", async () => { + const spaceAsset = makeMockAsset({ id: 42 }); + preconditions.hasLocalStoriesReferencing([42, 90]); + preconditions.canFetchRemoteFolders([]); + preconditions.canFetchRemoteAssetPages([[spaceAsset]]); + preconditions.canDownloadAssets([spaceAsset]); + preconditions.sharedAssetResolves({ + id: 90, + filename: "https://a.storyblok.com/g/1/x.png", + asset_folder_id: 7, + }); + preconditions.forbidsLibraryDiscovery(); + + await assetsCommand.parseAsync(["node", "test", "pull", "--space", "12345"]); + + expect(assetFileExists(spaceAsset)).toBeTruthy(); + expect(console.warn).toHaveBeenCalledWith( + expect.stringContaining("Shared libraries are unavailable"), + ); + expect(process.exitCode).toBe(0); + }); + + it("should fail rather than degrade when referenced-library discovery fails for a non-token-type reason", async () => { + const spaceAsset = makeMockAsset({ id: 42 }); + preconditions.hasLocalStoriesReferencing([42, 90]); + preconditions.canFetchRemoteFolders([]); + preconditions.canFetchRemoteAssetPages([[spaceAsset]]); + preconditions.canDownloadAssets([spaceAsset]); + preconditions.sharedAssetResolves({ + id: 90, + filename: "https://a.storyblok.com/g/1/x.png", + asset_folder_id: 7, + }); + preconditions.forbidsLibraryDiscoveryForAnotherReason(); + + await assetsCommand.parseAsync(["node", "test", "pull", "--space", "12345"]); + + expect(console.warn).not.toHaveBeenCalledWith( + expect.stringContaining("Shared libraries are unavailable"), + ); + expect(process.exitCode).toBe(1); + }); + + it("should fail when libraries were explicitly requested but are forbidden", async () => { + preconditions.forbidsLibraryDiscovery(); + + await assetsCommand.parseAsync([ + "node", + "test", + "pull", + "--space", + "12345", + "--target", + "shared", + ]); + + expect( + (console.error as ReturnType).mock.calls + .flat() + .some( + (arg) => typeof arg === "string" && arg.includes("This command is not available with"), + ), + ).toBe(true); + expect(process.exitCode).toBe(1); + }); + it("rejects an invalid --target value", async () => { const pull = assetsCommand.commands.find((command) => command.name() === "pull")!; pull.exitOverride(); diff --git a/packages/cli/src/commands/assets/pull/index.ts b/packages/cli/src/commands/assets/pull/index.ts index e832d630f..25ed6d0ca 100644 --- a/packages/cli/src/commands/assets/pull/index.ts +++ b/packages/cli/src/commands/assets/pull/index.ts @@ -10,7 +10,9 @@ import { resolveCommandPath } from "../../../utils/filesystem"; import { getLogger } from "../../../lib/logger/logger"; import { getReporter } from "../../../lib/reporter/reporter"; import { requireAuthentication } from "../../../utils/auth"; +import { APIError } from "../../../utils/error/api-error"; import { CommandError } from "../../../utils/error/command-error"; +import { isUnsupportedTokenTypeServerError } from "../../../utils/error/credential-hint"; import { handleError, logOnlyError, toError } from "../../../utils/error/error"; import { downloadAssetStream, @@ -350,16 +352,37 @@ pullCmd.action(async (options, command) => { const sharedAssets = resolved.filter((asset): asset is Asset => Boolean(asset?.id)); if (sharedAssets.length > 0) { - const resolveRoot = await buildLibraryRootResolver(space); - const byLibrary = new Map(); - for (const asset of sharedAssets) { - const libraryId = resolveRoot(asset.asset_folder_id ?? 0); - const bucket = byLibrary.get(libraryId) ?? []; - bucket.push(asset); - byLibrary.set(libraryId, bucket); + // `with-referenced` is implicit: the user asked to pull assets, not libraries. A + // credential that cannot reach library discovery degrades to the space-only pull + // with a warning rather than failing the whole command. An explicit `--target + // shared`/`--target all` still fails via `listReadableLibraries`. + let resolveRoot: ((assetFolderId: number) => number) | undefined; + try { + resolveRoot = await buildLibraryRootResolver(space); + } catch (error) { + if ( + error instanceof APIError && + error.code === 403 && + isUnsupportedTokenTypeServerError(error.serverError) + ) { + getUI().warn( + "Shared libraries are unavailable with this login; skipping referenced library assets.", + ); + } else { + throw error; + } } - for (const [libraryId, assets] of byLibrary) { - await pullReferencedAssets(libraryId, assets); + if (resolveRoot) { + const byLibrary = new Map(); + for (const asset of sharedAssets) { + const libraryId = resolveRoot(asset.asset_folder_id ?? 0); + const bucket = byLibrary.get(libraryId) ?? []; + bucket.push(asset); + byLibrary.set(libraryId, bucket); + } + for (const [libraryId, assets] of byLibrary) { + await pullReferencedAssets(libraryId, assets); + } } } } diff --git a/packages/cli/src/commands/assets/push/index.test.ts b/packages/cli/src/commands/assets/push/index.test.ts index d8dd7ec09..9b18230b5 100644 --- a/packages/cli/src/commands/assets/push/index.test.ts +++ b/packages/cli/src/commands/assets/push/index.test.ts @@ -457,6 +457,16 @@ const preconditions = { [filePath]: content, }); }, + forbidsLibraryDiscovery({ space = DEFAULT_SPACE }: { space?: string } = {}) { + server.use( + http.get(`https://mapi.storyblok.com/v1/spaces/${space}/shared_asset_folders`, () => + HttpResponse.json( + { error: "This endpoint does not support this token type" }, + { status: 403 }, + ), + ), + ); + }, hasLibraries( libraries: { id: number; name: string; accessLevel: "read" | "write" }[], { space = DEFAULT_SPACE }: { space?: string } = {}, @@ -2570,5 +2580,43 @@ describe("assets push command", () => { stderrCalls.some((msg: unknown) => typeof msg === "string" && msg.includes("ENOENT")), ).toBe(false); }); + + it("should push space assets and warn when library discovery is forbidden", async () => { + preconditions.forbidsLibraryDiscovery(); + const asset = makeMockAsset(); + preconditions.canLoadFolders([]); + preconditions.canLoadAssets([asset]); + preconditions.canUpsertRemoteAssets([asset]); + + await assetsCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + expect(console.warn).toHaveBeenCalledWith( + expect.stringContaining("Shared libraries are unavailable"), + ); + expect(process.exitCode).toBe(0); + }); + + it("should fail when libraries were explicitly requested but are forbidden", async () => { + preconditions.forbidsLibraryDiscovery(); + + await assetsCommand.parseAsync([ + "node", + "test", + "push", + "--space", + DEFAULT_SPACE, + "--target", + "shared", + ]); + + expect( + (console.error as ReturnType).mock.calls + .flat() + .some( + (arg) => typeof arg === "string" && arg.includes("This command is not available with"), + ), + ).toBe(true); + expect(process.exitCode).toBe(1); + }); }); }); diff --git a/packages/cli/src/commands/assets/push/index.ts b/packages/cli/src/commands/assets/push/index.ts index 06474d446..6a336154b 100644 --- a/packages/cli/src/commands/assets/push/index.ts +++ b/packages/cli/src/commands/assets/push/index.ts @@ -62,6 +62,7 @@ import { makeWriteStoryAPITransport } from "../../stories/streams"; import { assertLibraryWritable, listWritableLibraries, + listWritableLibrariesOrDegrade, resolveScopeBaseDir, type Scope, } from "../scope"; @@ -211,14 +212,26 @@ pushCmd.action(async (assetInput, options, command) => { scopes.push({ kind: "space", spaceId: fromSpace }); } if (target === "shared" || target === "auto") { - // `listWritableLibraries` already filters to writable libraries, so - // these scopes need no further access check. - const libraries = await listWritableLibraries(targetSpace); - scopes.push( - ...libraries.map( - (library) => ({ kind: "library", libraryId: library.id }) satisfies Scope, - ), - ); + // `auto` is implicit: the user asked to push assets, not libraries. A credential that + // cannot reach library discovery degrades to the space scope with a warning rather + // than failing the whole command. An explicit `--target shared` still fails. + // `listWritableLibraries`/`listWritableLibrariesOrDegrade` already filter to writable + // libraries, so these scopes need no further access check. + const libraries = + target === "auto" + ? await listWritableLibrariesOrDegrade(targetSpace) + : await listWritableLibraries(targetSpace); + if (libraries === undefined) { + getUI().warn( + "Shared libraries are unavailable with this login; continuing with space assets only.", + ); + } else { + scopes.push( + ...libraries.map( + (library) => ({ kind: "library", libraryId: library.id }) satisfies Scope, + ), + ); + } } } } catch (maybeError) { diff --git a/packages/cli/src/commands/assets/scope.test.ts b/packages/cli/src/commands/assets/scope.test.ts index 16f6408f1..5734cf223 100644 --- a/packages/cli/src/commands/assets/scope.test.ts +++ b/packages/cli/src/commands/assets/scope.test.ts @@ -1,5 +1,66 @@ -import { describe, expect, it } from "vitest"; -import { resolveScopeBaseDir, type Scope } from "./scope"; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest"; +import { http, HttpResponse } from "msw"; +import { setupServer } from "msw/node"; +import { getMapiClient } from "../../api"; +import { listLibrariesOrDegrade, resolveScopeBaseDir, type Scope } from "./scope"; + +const server = setupServer(); +beforeAll(() => server.listen({ onUnhandledRequest: "error" })); +beforeEach(() => { + getMapiClient({ personalAccessToken: "valid-token", region: "eu" }); +}); +afterEach(() => server.resetHandlers()); +afterAll(() => server.close()); + +const preconditions = { + forbidsLibraryDiscovery() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json( + { error: "This endpoint does not support this token type" }, + { status: 403 }, + ), + ), + ); + }, + hasNoLibraries() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json({ shared_asset_folders: [] }), + ), + ); + }, + forbidsLibraryDiscoveryForAnotherReason() { + server.use( + http.get("https://mapi.storyblok.com/v1/spaces/12345/shared_asset_folders", () => + HttpResponse.json( + { error: "This token is restricted to specific spaces" }, + { status: 403 }, + ), + ), + ); + }, +}; + +describe("listLibrariesOrDegrade", () => { + it("should return undefined when the credential cannot reach library discovery", async () => { + preconditions.forbidsLibraryDiscovery(); + + await expect(listLibrariesOrDegrade("12345")).resolves.toBeUndefined(); + }); + + it("should return an empty list when the space simply has no libraries", async () => { + preconditions.hasNoLibraries(); + + await expect(listLibrariesOrDegrade("12345")).resolves.toEqual([]); + }); + + it("should still fail loudly for a different 403 from the same endpoint", async () => { + preconditions.forbidsLibraryDiscoveryForAnotherReason(); + + await expect(listLibrariesOrDegrade("12345")).rejects.toThrow(); + }); +}); describe("resolveScopeBaseDir", () => { it("returns the space subtree for a space scope", () => { diff --git a/packages/cli/src/commands/assets/scope.ts b/packages/cli/src/commands/assets/scope.ts index d95d494ac..53e4d9c23 100644 --- a/packages/cli/src/commands/assets/scope.ts +++ b/packages/cli/src/commands/assets/scope.ts @@ -1,7 +1,8 @@ import { join } from "pathe"; import { directories } from "../../constants"; import { getMapiClient } from "../../api"; -import { handleAPIError } from "../../utils/error/api-error"; +import { APIError, handleAPIError } from "../../utils/error/api-error"; +import { isUnsupportedTokenTypeServerError } from "../../utils/error/credential-hint"; import { toError } from "../../utils/error/error"; import { resolveCommandPath } from "../../utils/filesystem"; import type { SharedAssetFolder } from "./types"; @@ -55,7 +56,7 @@ export async function listLibraries(spaceId: string): Promise { ?.access_level ?? "read", })); } catch (maybeError) { - handleAPIError("pull_shared_asset_folders", toError(maybeError)); + handleAPIError("list_shared_asset_folders", toError(maybeError)); } } @@ -65,6 +66,37 @@ export const listReadableLibraries = (spaceId: string): Promise => export const listWritableLibraries = async (spaceId: string): Promise => (await listLibraries(spaceId)).filter((library) => library.accessLevel === "write"); +/** + * Library discovery that tolerates a credential which cannot reach the endpoint at all. + * Returns `undefined` when the lookup was forbidden, distinct from `[]` for a space with + * no libraries, so callers can warn only in the former case. + * + * The shared-asset controllers carry no scope annotations, so an OAuth grant is refused + * regardless of consent (storyrails credential.rb). Implicit callers degrade to the space + * scope rather than failing a command the user did not aim at libraries. + */ +export async function listLibrariesOrDegrade(spaceId: string): Promise { + try { + return await listLibraries(spaceId); + } catch (error) { + if ( + error instanceof APIError && + error.code === 403 && + isUnsupportedTokenTypeServerError(error.serverError) + ) { + return undefined; + } + throw error; + } +} + +export const listWritableLibrariesOrDegrade = async ( + spaceId: string, +): Promise => { + const libraries = await listLibrariesOrDegrade(spaceId); + return libraries?.filter((library) => library.accessLevel === "write"); +}; + /** * Builds a resolver mapping any shared `asset_folder_id` to its top-level * library root id (walking `parent_id` up). Fetches the space's shared folders @@ -73,12 +105,18 @@ export const listWritableLibraries = async (spaceId: string): Promise export async function buildLibraryRootResolver( spaceId: string, ): Promise<(assetFolderId: number) => number> { - const { data } = await getMapiClient().sharedAssetFolders.list({ - path: { space_id: Number(spaceId) }, - throwOnError: true, - }); + let folders: SharedAssetFolder[]; + try { + const { data } = await getMapiClient().sharedAssetFolders.list({ + path: { space_id: Number(spaceId) }, + throwOnError: true, + }); + folders = data?.shared_asset_folders ?? []; + } catch (maybeError) { + handleAPIError("list_shared_asset_folders", toError(maybeError)); + } const parentById = new Map(); - for (const folder of data?.shared_asset_folders ?? []) { + for (const folder of folders) { parentById.set(folder.id, folder.parent_id ?? null); } return (assetFolderId: number): number => { diff --git a/packages/cli/src/commands/login/README.md b/packages/cli/src/commands/login/README.md index 9433717d6..06d001dbf 100644 --- a/packages/cli/src/commands/login/README.md +++ b/packages/cli/src/commands/login/README.md @@ -66,6 +66,9 @@ own OAuth app instead, for example while developing against a self-hosted instan - If you're already logged in, you'll need to logout first to switch accounts - For CI environments, it's recommended to use the `--token` option - The CLI supports two-factor authentication (2FA) when using email login +- If a command reports a missing permission, the message names the scope and the fix. For an OAuth + login, re-run `storyblok login` and grant it at the consent screen. For a personal access token, + create a new token with that scope. > If you sign in with SSO (e.g., Google, GitHub, Azure AD), you must use a Personal Access Token. > Generate one in your account settings: diff --git a/packages/cli/src/commands/login/actions.test.ts b/packages/cli/src/commands/login/actions.test.ts index 30f4ad120..6c59ba318 100644 --- a/packages/cli/src/commands/login/actions.test.ts +++ b/packages/cli/src/commands/login/actions.test.ts @@ -49,8 +49,7 @@ describe("login actions", () => { it("should throw an masked error for invalid token", async () => { await expect(loginWithToken("invalid-token", "eu")).rejects.toThrow( - `The token provided ${chalk.bold("inva*********")} is invalid. - Please make sure you are using the correct token and try again.`, + `The token provided ${chalk.bold("inva*********")} is invalid. Please make sure you are using the correct token and try again.`, ); }); diff --git a/packages/cli/src/commands/login/oauth.test.ts b/packages/cli/src/commands/login/oauth.test.ts index 1d891842a..28100438d 100644 --- a/packages/cli/src/commands/login/oauth.test.ts +++ b/packages/cli/src/commands/login/oauth.test.ts @@ -14,7 +14,10 @@ vi.mock("node:fs/promises"); // Avoid opening a real browser and a real socket in tests. vi.mock("open", () => ({ default: vi.fn(async () => undefined) })); vi.mock("../oauth/server", () => ({ - waitForCallback: vi.fn(async () => ({ code: "auth-code", state: "ignored" })), + startCallbackServer: vi.fn(async () => ({ + callback: Promise.resolve({ code: "auth-code", state: "ignored" }), + close: vi.fn(), + })), })); // Force the state check to pass by returning the same state generatePkce/generateState produced. vi.mock("../oauth/pkce", async (importOriginal) => { diff --git a/packages/cli/src/commands/logout/oauth.test.ts b/packages/cli/src/commands/logout/oauth.test.ts index 1041f7378..997cc372e 100644 --- a/packages/cli/src/commands/logout/oauth.test.ts +++ b/packages/cli/src/commands/logout/oauth.test.ts @@ -6,6 +6,7 @@ import { setupServer } from "msw/node"; import "../../index"; import { logoutCommand } from "./index"; import { getOAuthEntry } from "../oauth/store"; +import { getUI } from "../../lib/ui"; vi.mock("node:fs"); vi.mock("node:fs/promises"); @@ -99,4 +100,16 @@ describe("logout with an oauth session", () => { expect(revokeRequests).toHaveLength(0); expect(await getOAuthEntry("eu")).toEqual({}); }); + + it("should not advise a login flag while logging out", async () => { + delete process.env.STORYBLOK_OAUTH_CLIENT_ID; + delete process.env.STORYBLOK_OAUTH_CLIENT_SECRET; + const warnSpy = vi.spyOn(getUI(), "warn").mockImplementation(() => {}); + + await logoutCommand.parseAsync(["node", "test"]); + + const warned = warnSpy.mock.calls.flat().join("\n"); + expect(warned).toContain("Could not revoke the OAuth session server-side"); + expect(warned).not.toContain("--oauth"); + }); }); diff --git a/packages/cli/src/commands/oauth/client.test.ts b/packages/cli/src/commands/oauth/client.test.ts index 2ee2f8db7..8200f6b90 100644 --- a/packages/cli/src/commands/oauth/client.test.ts +++ b/packages/cli/src/commands/oauth/client.test.ts @@ -20,4 +20,16 @@ describe("resolveOAuthClient", () => { expect(OAUTH_CLIENT_ID.startsWith(OAUTH_CLIENT_PLACEHOLDER_PREFIX)).toBe(true); expect(() => resolveOAuthClient()).toThrow(/ships without OAuth client credentials/); }); + + it("should throw a cause-only message that names no command flag", () => { + expect(() => resolveOAuthClient()).toThrow( + "This build of the CLI ships without OAuth client credentials.", + ); + try { + resolveOAuthClient(); + } catch (error) { + expect((error as Error).message).not.toContain("--oauth"); + expect((error as Error).message).not.toContain("\n"); + } + }); }); diff --git a/packages/cli/src/commands/oauth/client.ts b/packages/cli/src/commands/oauth/client.ts index 1edc6f75a..2cad63446 100644 --- a/packages/cli/src/commands/oauth/client.ts +++ b/packages/cli/src/commands/oauth/client.ts @@ -12,11 +12,9 @@ export const resolveOAuthClient = (): OAuthClientCredentials => { } if (OAUTH_CLIENT_ID.startsWith(OAUTH_CLIENT_PLACEHOLDER_PREFIX)) { - throw new CommandError( - `This build of the CLI ships without OAuth client credentials, so \`--oauth\` cannot be used yet.\n` + - `Log in with a Personal Access Token (\`storyblok login --token \`), or set the ` + - `STORYBLOK_OAUTH_CLIENT_ID and STORYBLOK_OAUTH_CLIENT_SECRET environment variables to use your own OAuth app.`, - ); + // Cause only. Each caller appends the consequence and remedy, because only the caller + // knows whether the user was logging in, refreshing a session, or revoking one. + throw new CommandError("This build of the CLI ships without OAuth client credentials."); } return { client_id: OAUTH_CLIENT_ID, client_secret: OAUTH_CLIENT_SECRET }; diff --git a/packages/cli/src/commands/oauth/login-flow.test.ts b/packages/cli/src/commands/oauth/login-flow.test.ts index ec3ec0ee9..a17126dee 100644 --- a/packages/cli/src/commands/oauth/login-flow.test.ts +++ b/packages/cli/src/commands/oauth/login-flow.test.ts @@ -15,7 +15,10 @@ vi.mock("./pkce", () => ({ generateState: () => "state-abc", })); vi.mock("./server", () => ({ - waitForCallback: vi.fn(async () => ({ code: "auth-code", state: "state-abc" })), + startCallbackServer: vi.fn(async () => ({ + callback: Promise.resolve({ code: "auth-code", state: "state-abc" }), + close: vi.fn(), + })), })); vi.mock("./token-endpoint", () => ({ exchangeToken: vi.fn(async () => ({ access_token: "at", refresh_token: "rt", expires_in: 900 })), @@ -24,6 +27,7 @@ vi.mock("./grant", () => ({ introspectGrant: vi.fn() })); const { introspectGrant } = await import("./grant"); const { resolveOAuthClient } = await import("./client"); +const { startCallbackServer } = await import("./server"); describe("buildAuthorizeUrl", () => { it("should build an /oauth/init URL with PKCE and space-safe params", () => { @@ -94,6 +98,21 @@ describe("performOAuthLogin", () => { expect(params.get("scope")).toBe(OAUTH_LOGIN_SCOPES.join(" ")); }); + it("should not open a browser when the callback port cannot be bound", async () => { + vi.mocked(startCallbackServer).mockRejectedValueOnce( + new Error("Port 4900 is already in use by nc (PID 1)"), + ); + const openBrowser = vi.fn(async () => {}); + + await expect(performOAuthLogin({ region: "eu", openBrowser })).rejects.toThrow( + "already in use", + ); + + // Sending a user through consent whose redirect can never be received is worse than + // failing before the tab opens. + expect(openBrowser).not.toHaveBeenCalled(); + }); + it("should not persist tokens when introspection fails", async () => { vi.mocked(introspectGrant).mockRejectedValueOnce(new Error("introspection failed")); @@ -105,3 +124,32 @@ describe("performOAuthLogin", () => { expect(await getOAuthActiveRegion()).toBeUndefined(); }); }); + +// This suite's top-level `vi.mock("./client", ...)` stubs resolveOAuthClient for every other +// test above so they don't trip the placeholder-client guard. Here we unmock it for real, so +// performOAuthLogin runs against the actual resolveOAuthClient() and its thrown cause, letting +// this test verify the login-specific remedy the catch block in login-flow.ts appends. +describe("performOAuthLogin when no oauth client credentials are available", () => { + afterEach(() => { + delete process.env.STORYBLOK_OAUTH_CLIENT_ID; + delete process.env.STORYBLOK_OAUTH_CLIENT_SECRET; + vi.doMock("./client", () => ({ + resolveOAuthClient: vi.fn(() => ({ client_id: "cid", client_secret: "sec" })), + })); + vi.resetModules(); + }); + + it("should surface the login remedy naming a token login and the client env vars", async () => { + delete process.env.STORYBLOK_OAUTH_CLIENT_ID; + delete process.env.STORYBLOK_OAUTH_CLIENT_SECRET; + vi.doUnmock("./client"); + vi.resetModules(); + const { performOAuthLogin: performOAuthLoginWithRealClient } = await import("./login-flow"); + + await expect( + performOAuthLoginWithRealClient({ region: "eu", openBrowser: async () => {} }), + ).rejects.toThrow( + "or set STORYBLOK_OAUTH_CLIENT_ID and STORYBLOK_OAUTH_CLIENT_SECRET to use your own OAuth app", + ); + }); +}); diff --git a/packages/cli/src/commands/oauth/login-flow.ts b/packages/cli/src/commands/oauth/login-flow.ts index 82772b0bb..d7ce06dfb 100644 --- a/packages/cli/src/commands/oauth/login-flow.ts +++ b/packages/cli/src/commands/oauth/login-flow.ts @@ -13,9 +13,9 @@ import { import { introspectGrant } from "./grant"; import { generatePkce, generateState } from "./pkce"; import { computeExpiresAt } from "./refresh"; -import { waitForCallback } from "./server"; +import { startCallbackServer } from "./server"; import { setOAuthActiveRegion, updateOAuthEntry } from "./store"; -import type { OAuthGrantSpace, OAuthTokens } from "./store"; +import type { OAuthClientCredentials, OAuthGrantSpace, OAuthTokens } from "./store"; import { exchangeToken } from "./token-endpoint"; export interface OAuthLoginResult { @@ -51,13 +51,23 @@ export const performOAuthLogin = async (options: { const openBrowser = options.openBrowser ?? ((url) => open(url)); const ui = getUI(); - const client = resolveOAuthClient(); + let client: OAuthClientCredentials; + try { + client = resolveOAuthClient(); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new CommandError( + `${message} Log in with a Personal Access Token (\`storyblok login --token \`), or set STORYBLOK_OAUTH_CLIENT_ID and STORYBLOK_OAUTH_CLIENT_SECRET to use your own OAuth app.`, + ); + } const scopes = OAUTH_LOGIN_SCOPES; const { verifier, challenge } = generatePkce(); const state = generateState(); - // Start listening before opening the browser so no callback is missed. - const callbackPromise = waitForCallback(OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH); + // Bind the callback port before opening the browser: no callback can be missed, and a port + // conflict fails here rather than after sending the user through a consent screen whose + // redirect the CLI could never have received. + const listener = await startCallbackServer(OAUTH_CALLBACK_PORT, OAUTH_CALLBACK_PATH); const authorizeUrl = buildAuthorizeUrl({ region, @@ -69,9 +79,14 @@ export const performOAuthLogin = async (options: { ui.info( `Opening your browser to authorize the Storyblok CLI.\nIf it does not open, visit:\n${authorizeUrl}`, ); - await openBrowser(authorizeUrl); + try { + await openBrowser(authorizeUrl); + } catch (error) { + listener.close(); + throw error; + } - const { code, state: returnedState } = await callbackPromise; + const { code, state: returnedState } = await listener.callback; if (returnedState !== state) { throw new CommandError( "OAuth state mismatch; aborting for your safety. Please try `storyblok login` again.", diff --git a/packages/cli/src/commands/oauth/pkce.test.ts b/packages/cli/src/commands/oauth/pkce.test.ts index 586142a73..fc34cb7f5 100644 --- a/packages/cli/src/commands/oauth/pkce.test.ts +++ b/packages/cli/src/commands/oauth/pkce.test.ts @@ -7,13 +7,13 @@ describe("generatePkce", () => { expect(verifier.length).toBeGreaterThanOrEqual(43); expect(verifier.length).toBeLessThanOrEqual(128); expect(verifier).toMatch(/^[\w\-.~]+$/); - expect(challenge).toMatch(/^[\w\-]+$/); + expect(challenge).toMatch(/^[\w-]+$/); expect(challenge).not.toBe(verifier); }); }); describe("generateState", () => { it("should produce a non-empty url-safe string", () => { - expect(generateState()).toMatch(/^[\w\-]+$/); + expect(generateState()).toMatch(/^[\w-]+$/); }); }); diff --git a/packages/cli/src/commands/oauth/refresh.test.ts b/packages/cli/src/commands/oauth/refresh.test.ts index f3c9c455a..c1f27ffa8 100644 --- a/packages/cli/src/commands/oauth/refresh.test.ts +++ b/packages/cli/src/commands/oauth/refresh.test.ts @@ -123,4 +123,16 @@ describe("refreshOAuthTokens", () => { }); await expect(refreshOAuthTokens("eu")).rejects.toThrow(); }); + + it("should surface the refresh remedy when no oauth client credentials are available", async () => { + // Override this suite's beforeEach, which sets these so refreshes normally resolve their + // credentials through the env-var override; deleting them here reaches the placeholder + // guard in resolveOAuthClient() and exercises the catch block's refresh-specific remedy. + delete process.env.STORYBLOK_OAUTH_CLIENT_ID; + delete process.env.STORYBLOK_OAUTH_CLIENT_SECRET; + + await expect(refreshOAuthTokens("eu")).rejects.toThrow( + "Your OAuth session cannot be refreshed:", + ); + }); }); diff --git a/packages/cli/src/commands/oauth/refresh.ts b/packages/cli/src/commands/oauth/refresh.ts index b7b48ae45..9181baf6a 100644 --- a/packages/cli/src/commands/oauth/refresh.ts +++ b/packages/cli/src/commands/oauth/refresh.ts @@ -2,7 +2,7 @@ import type { RegionCode } from "../../constants"; import { CommandError } from "../../utils"; import { resolveOAuthClient } from "./client"; import { getOAuthEntry, updateOAuthEntry } from "./store"; -import type { OAuthTokens } from "./store"; +import type { OAuthClientCredentials, OAuthTokens } from "./store"; import { exchangeToken } from "./token-endpoint"; export const computeExpiresAt = (expiresInSeconds: number, nowMs: number = Date.now()): string => { @@ -20,7 +20,15 @@ const doRefresh = async (region: RegionCode): Promise => { throw new CommandError("No OAuth refresh token stored. Run `storyblok login` to authenticate."); } - const client = resolveOAuthClient(); + let client: OAuthClientCredentials; + try { + client = resolveOAuthClient(); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new CommandError( + `Your OAuth session cannot be refreshed: ${message} Log in with a Personal Access Token (\`storyblok login --token \`).`, + ); + } let response; try { diff --git a/packages/cli/src/commands/oauth/server.test.ts b/packages/cli/src/commands/oauth/server.test.ts index 9ee176726..8b7830e7e 100644 --- a/packages/cli/src/commands/oauth/server.test.ts +++ b/packages/cli/src/commands/oauth/server.test.ts @@ -1,37 +1,40 @@ import { createServer } from "node:http"; import { describe, expect, it } from "vitest"; -import { waitForCallback } from "./server"; +import { startCallbackServer } from "./server"; const PATH = "/oauth/callback"; const callback = (port: number, query: string) => fetch(`http://127.0.0.1:${port}${PATH}${query}`); -describe("waitForCallback", () => { +describe("startCallbackServer", () => { it("should resolve with code and state and serve a 200 success page", async () => { - const pending = waitForCallback(4917, PATH); + const listener = await startCallbackServer(4917, PATH); const response = await callback(4917, "?code=auth-code&state=state-abc"); expect(response.status).toBe(200); - await expect(pending).resolves.toEqual({ code: "auth-code", state: "state-abc" }); + await expect(listener.callback).resolves.toEqual({ code: "auth-code", state: "state-abc" }); }); it("should serve a non-200 page and reject when the callback carries an error", async () => { + const listener = await startCallbackServer(4918, PATH); // Attach the rejection assertion before triggering the callback so the // rejection is handled the moment it settles. - const rejected = expect(waitForCallback(4918, PATH)).rejects.toThrow(/access_denied/); + const rejected = expect(listener.callback).rejects.toThrow(/access_denied/); const response = await callback(4918, "?error=access_denied&error_description=denied"); expect(response.status).toBe(400); await rejected; }); - it("should explain which process blocks the callback port when it is already in use", async () => { + it("should reject before listening when the callback port is already in use", async () => { const blocker = createServer(); await new Promise((resolve) => blocker.listen(4920, "127.0.0.1", resolve)); try { - // The holder lookup shells out to lsof/netstat, which may be unavailable in CI, so the - // assertion covers the always-present parts: the port, the cause, and the way out. - await expect(waitForCallback(4920, PATH)).rejects.toThrow( + // Rejecting the start promise (rather than the callback promise) is what lets the login + // flow report the conflict before it opens a browser tab. The holder lookup shells out to + // lsof/netstat, which may be unavailable in CI, so the assertion covers the always-present + // parts: the port, the cause, and the way out. + await expect(startCallbackServer(4920, PATH)).rejects.toThrow( /Port 4920 is already in use .*run `storyblok login --oauth` again/s, ); } finally { @@ -40,10 +43,20 @@ describe("waitForCallback", () => { }); it("should serve a non-200 page and reject when code or state is missing", async () => { - const rejected = expect(waitForCallback(4919, PATH)).rejects.toThrow(/code and state/); + const listener = await startCallbackServer(4919, PATH); + const rejected = expect(listener.callback).rejects.toThrow(/code and state/); const response = await callback(4919, "?code=only-code"); expect(response.status).toBe(400); await rejected; }); + + it("should stop listening once closed so the port is free again", async () => { + const listener = await startCallbackServer(4921, PATH); + listener.close(); + + // A second bind on the same port only succeeds if the first server really let it go. + const reopened = await startCallbackServer(4921, PATH); + reopened.close(); + }); }); diff --git a/packages/cli/src/commands/oauth/server.ts b/packages/cli/src/commands/oauth/server.ts index 45cd7b8cb..a1300f4d3 100644 --- a/packages/cli/src/commands/oauth/server.ts +++ b/packages/cli/src/commands/oauth/server.ts @@ -16,13 +16,33 @@ const ERROR_PAGE = page( "Authorization failed. You can close this tab and return to the terminal.", ); -export const waitForCallback = ( +export interface CallbackListener { + /** Settles when the browser hits the redirect URI, or the wait times out. */ + callback: Promise<{ code: string; state: string }>; + /** Stops listening. Safe to call after `callback` has already settled. */ + close: () => void; +} + +/** + * Binds the loopback callback server and resolves only once it is actually listening, so a + * caller can confirm the port is free *before* sending the user to the consent screen. A bind + * failure (typically `EADDRINUSE`) rejects this promise rather than the callback promise — + * otherwise a doomed run would still open a browser tab it can never collect a code from. + */ +export const startCallbackServer = ( port: number, path: string, timeoutMs = 300_000, -): Promise<{ code: string; state: string }> => { - return new Promise((resolve, reject) => { +): Promise => { + return new Promise((ready, failToStart) => { + let listening = false; let timer: NodeJS.Timeout; + let settleCallback: (result: { code: string; state: string }) => void; + let failCallback: (error: Error) => void; + const callback = new Promise<{ code: string; state: string }>((resolve, reject) => { + settleCallback = resolve; + failCallback = reject; + }); const server = createServer((req, res) => { const url = new URL(req.url ?? "/", `http://localhost:${port}`); @@ -38,7 +58,7 @@ export const waitForCallback = ( const fail = (error: CommandError): void => { res.writeHead(400, { "Content-Type": "text/html" }); res.end(ERROR_PAGE); - reject(error); + failCallback(error); }; const error = url.searchParams.get("error"); @@ -59,17 +79,12 @@ export const waitForCallback = ( res.writeHead(200, { "Content-Type": "text/html" }); res.end(SUCCESS_PAGE); - resolve({ code, state }); + settleCallback({ code, state }); }); - timer = setTimeout(() => { - server.close(); - reject(new CommandError("Timed out waiting for the browser authorization callback.")); - }, timeoutMs); - timer.unref?.(); - server.on("error", (err) => { clearTimeout(timer); + const reject = listening ? failCallback : failToStart; if ((err as NodeJS.ErrnoException).code === "EADDRINUSE") { describePortConflict(port).then( (message) => reject(new CommandError(message)), @@ -79,6 +94,23 @@ export const waitForCallback = ( } reject(err); }); + + server.on("listening", () => { + listening = true; + timer = setTimeout(() => { + server.close(); + failCallback(new CommandError("Timed out waiting for the browser authorization callback.")); + }, timeoutMs); + timer.unref?.(); + ready({ + callback, + close: () => { + clearTimeout(timer); + server.close(); + }, + }); + }); + // Bind to loopback only so the authorization code is never accepted from other hosts. server.listen(port, "127.0.0.1"); }); diff --git a/packages/cli/src/commands/oauth/space-guard.test.ts b/packages/cli/src/commands/oauth/space-guard.test.ts index 5552927db..5f09bcc21 100644 --- a/packages/cli/src/commands/oauth/space-guard.test.ts +++ b/packages/cli/src/commands/oauth/space-guard.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { formatSpaceNotAllowedMessage } from "../../utils"; import { assertSpaceAllowed } from "./space-guard"; describe("assertSpaceAllowed", () => { @@ -6,8 +7,10 @@ describe("assertSpaceAllowed", () => { expect(() => assertSpaceAllowed(123, [{ id: 123 }])).not.toThrow(); }); - it("should throw when the space is outside the grant", () => { - expect(() => assertSpaceAllowed(999, [{ id: 123 }])).toThrow(/not covered by your OAuth login/); + it("should reject a space outside the grant using the shared wording", () => { + expect(() => assertSpaceAllowed(999, [{ id: 1 }, { id: 2 }])).toThrow( + formatSpaceNotAllowedMessage(999, [1, 2]), + ); }); it("should pass when the grant has no space restriction", () => { diff --git a/packages/cli/src/commands/oauth/space-guard.ts b/packages/cli/src/commands/oauth/space-guard.ts index c59225da0..224aea3b2 100644 --- a/packages/cli/src/commands/oauth/space-guard.ts +++ b/packages/cli/src/commands/oauth/space-guard.ts @@ -1,4 +1,4 @@ -import { CommandError } from "../../utils"; +import { CommandError, formatSpaceNotAllowedMessage } from "../../utils"; // A grant with an empty/absent space list is not space-restricted (storyrails token_scopeable.rb). export const assertSpaceAllowed = ( @@ -13,10 +13,11 @@ export const assertSpaceAllowed = ( } const target = Number(space); if (!grantedSpaces.some((granted) => granted.id === target)) { - const allowed = grantedSpaces.map((granted) => granted.id).join(", "); throw new CommandError( - `Space ${space} is not covered by your OAuth login (authorized spaces: ${allowed}).\n` + - `Re-run \`storyblok login\` and select this space at the consent screen.`, + formatSpaceNotAllowedMessage( + space, + grantedSpaces.map((granted) => granted.id), + ), ); } }; diff --git a/packages/cli/src/commands/stories/push/README.md b/packages/cli/src/commands/stories/push/README.md index 418f28eeb..0adeb2653 100644 --- a/packages/cli/src/commands/stories/push/README.md +++ b/packages/cli/src/commands/stories/push/README.md @@ -31,3 +31,6 @@ storyblok stories push --space YOUR_SPACE_ID - Component schemas are required to map story references correctly. Always run `storyblok components pull` to fetch the latest schemas before running `storyblok stories push`. +- The command exits with code 1 if any story fails to push, even if most stories succeed. Check the + exit code in CI pipelines and scripts that run `storyblok stories push`, since a partial failure + no longer exits 0. diff --git a/packages/cli/src/commands/stories/push/failure-report.test.ts b/packages/cli/src/commands/stories/push/failure-report.test.ts new file mode 100644 index 000000000..9c39a4fdf --- /dev/null +++ b/packages/cli/src/commands/stories/push/failure-report.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from "vitest"; +import { FailureCollector } from "./failure-report"; +import { APIError, resetCredentialContext, setCredentialContext } from "../../../utils"; +import { FetchError } from "../../../utils/fetch"; + +const insufficientScopeError = (): APIError => { + setCredentialContext({ kind: "oauth" }); + const apiError = new APIError( + "insufficient_scope", + "update_story", + new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }), + ); + resetCredentialContext(); + return apiError; +}; + +describe("FailureCollector fatal tracking", () => { + it("should report no fatal failure for ordinary errors", () => { + const failures = new FailureCollector(); + failures.record({ filename: "a.json" }, new Error("boom")); + + expect(failures.hasFatal).toBe(false); + }); + + it("should report a fatal failure once a credential error is recorded", () => { + const failures = new FailureCollector(); + failures.record({ filename: "a.json" }, insufficientScopeError()); + + expect(failures.hasFatal).toBe(true); + }); +}); diff --git a/packages/cli/src/commands/stories/push/failure-report.ts b/packages/cli/src/commands/stories/push/failure-report.ts index 529511cc4..f77823590 100644 --- a/packages/cli/src/commands/stories/push/failure-report.ts +++ b/packages/cli/src/commands/stories/push/failure-report.ts @@ -112,6 +112,7 @@ function renderFailureReport(ui: UI, records: FailedStoryRecord[], verbose: bool */ export class FailureCollector { private records = new Map(); + private fatal = false; private keyFor(story: StoryIdentity): string { return story.full_slug ?? story.uuid ?? story.filename ?? `__unknown_${this.records.size}`; @@ -123,6 +124,9 @@ export class FailureCollector { * the caller should skip counter updates to avoid double-billing). */ record(story: StoryIdentity, error: Error): boolean { + if (error instanceof APIError && error.fatal) { + this.fatal = true; + } const key = this.keyFor(story); if (this.records.has(key)) { return false; @@ -142,6 +146,14 @@ export class FailureCollector { return this.records.size === 0; } + /** + * True when a recorded failure is credential-level. Such a failure is identical for + * every remaining item, so the caller should stop rather than repeat it per story. + */ + get hasFatal(): boolean { + return this.fatal; + } + get size(): number { return this.records.size; } diff --git a/packages/cli/src/commands/stories/push/index.test.ts b/packages/cli/src/commands/stories/push/index.test.ts index 46fb2819a..bffaba92f 100644 --- a/packages/cli/src/commands/stories/push/index.test.ts +++ b/packages/cli/src/commands/stories/push/index.test.ts @@ -153,6 +153,28 @@ const preconditions = { ); } }, + /** + * Every update fails with the same credential-level 403. Returns a counter + * object tracking how many PUT requests the handler actually received, so + * a test can prove the pipeline stopped issuing requests early rather than + * attempting every story. + */ + failsToUpdateStoriesWithInsufficientScope(stories: MockStory[], space = DEFAULT_SPACE) { + const requestCount = { current: 0 }; + const ids = new Set(stories.map((s) => String(s.id))); + server.use( + http.put(`https://mapi.storyblok.com/v1/spaces/${space}/stories/:id`, ({ params }) => { + if (ids.has(String(params.id))) { + requestCount.current += 1; + } + return HttpResponse.json( + { error: "Insufficient scope: stories:write is required" }, + { status: 403 }, + ); + }), + ); + return requestCount; + }, canListStories(stories: MockStory[], space = DEFAULT_SPACE) { // The push command issues targeted list calls filtered by `by_slugs` or // `by_ids`. Honor both filters so the handler matches the real MAPI shape; @@ -190,8 +212,12 @@ describe("stories push command", () => { server.resetHandlers(); getProgram().setOptionValueWithSource("path", undefined, "default"); resetReporter(); + process.exitCode = undefined; + }); + afterAll(() => { + server.close(); + process.exitCode = undefined; }); - afterAll(() => server.close()); describe("first-time push", () => { it("should push stories with mapped references", async () => { @@ -404,7 +430,7 @@ describe("stories push command", () => { expect.stringContaining("Push results: 5 stories pushed, 0 stories failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 5/5 succeeded, 0 failed."), + expect.stringContaining("Creating stories: 5/5 succeeded, 0 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 5/5 succeeded, 0 failed."), @@ -855,8 +881,10 @@ describe("stories push command", () => { expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Push results: 1 story pushed, 0 stories failed"), ); + // The story already existed remotely, so creation was skipped, not + // succeeded. Only the update phase actually wrote anything. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 0 failed, 1 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 1/1 succeeded, 0 failed."), @@ -1373,12 +1401,12 @@ describe("stories push command", () => { // UI — deferred, grouped summary (no inline console.error during streaming) expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); expect(console.error).toHaveBeenCalledWith(expect.stringContaining(`story-a (uuid: `)); - // UI + // UI — creation itself failed (manifest append), so nothing was written. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/0 succeeded, 0 failed."), @@ -1408,6 +1436,8 @@ describe("stories push command", () => { ); expect(actions.createStory).not.toHaveBeenCalled(); expect(actions.updateStory).not.toHaveBeenCalled(); + // Bailing on a precondition is a failed push; CI must not read it as a clean run. + expect(process.exitCode).toBe(2); }); it("should abort with a hard error when a story references a missing component schema", async () => { @@ -1433,6 +1463,7 @@ describe("stories push command", () => { ); const report = getReport(); expect(report?.status).toBe("FAILURE"); + expect(process.exitCode).toBe(2); const logFile = getLogFileContents(LOG_PREFIX); expect(logFile).toContain("Schema validation failed"); }); @@ -1667,7 +1698,7 @@ describe("stories push command", () => { const report = getReport(); expect(report?.status).toBe("FAILURE"); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/0 succeeded, 0 failed."), @@ -1818,12 +1849,12 @@ describe("stories push command", () => { // UI — deferred, grouped summary (no inline console.error during streaming) expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); expect(console.error).toHaveBeenCalledWith(expect.stringContaining("story-a.json")); - // UI + // UI — the local file failed to load, so no story was ever pushed. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/0 succeeded, 0 failed."), @@ -1860,12 +1891,12 @@ describe("stories push command", () => { expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Failed to create placeholder story"), ); - // UI + // UI — placeholder creation failed, so nothing reached the remote. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed."), + expect.stringContaining("Creating stories: 0/1 succeeded, 1 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/0 succeeded, 0 failed."), @@ -1912,11 +1943,13 @@ describe("stories push command", () => { expect.stringContaining("Invalid bloks field: expected an array"), ); expect(console.error).toHaveBeenCalledWith(expect.stringContaining(`story-a (uuid: `)); + // The placeholder was created, but mapping references failed before + // the update phase ever ran, so the story's content never got written. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed."), + expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 0/1 succeeded, 1 failed."), @@ -1975,11 +2008,13 @@ describe("stories push command", () => { expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed to update story")); expect(console.error).toHaveBeenCalledWith(expect.stringContaining(`story-a (uuid: `)); + // The placeholder was created and references were mapped, but the + // update call itself failed, so the story's real content never wrote. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed."), + expect.stringContaining("Creating stories: 1/1 succeeded, 0 failed, 0 skipped."), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Processing stories: 1/1 succeeded, 0 failed."), @@ -2023,9 +2058,10 @@ describe("stories push command", () => { // story disappears from this phase's counts. updateResults: { total: 0, succeeded: 0, failed: 0 }, }); - // Top summary must reflect the distinct failure (1), not 0. + // Top summary must reflect the distinct failure (1), not 0, and nothing + // was actually written, so pushed stays 0. expect(console.error).toHaveBeenCalledWith( - expect.stringContaining("Push results: 1 story pushed, 1 story failed"), + expect.stringContaining("Push results: 0 stories pushed, 1 story failed"), ); expect(console.error).toHaveBeenCalledWith( expect.stringContaining("Updating stories: 0/0 succeeded, 0 failed."), @@ -2033,5 +2069,125 @@ describe("stories push command", () => { // The grouped report still lists the story exactly once. expect(console.error).toHaveBeenCalledWith(expect.stringContaining("Failed stories (1):")); }); + + it("should not report stories as pushed when every update fails", async () => { + // Regression for a live run measured with `stories:write` missing: + // the headline said "10 stories pushed" while nothing was written. + // Uses a 500 (not 403) so this stays independent of the credential + // short-circuit path covered above. + const storyA = makeMockStory({ slug: "story-a" }); + const storyB = makeMockStory({ slug: "story-b" }); + const localStories = [storyA, storyB]; + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + // Matching uuids in the target space mean creation is skipped for + // both stories; only the update phase determines whether anything + // was actually written. + preconditions.canListStories(localStories); + preconditions.failsToUpdateStories(localStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining("Push results: 0 stories pushed, 2 stories failed"), + ); + expect(console.error).not.toHaveBeenCalledWith(expect.stringContaining("2 stories pushed")); + }); + + it("should count skipped creations separately from succeeded ones", async () => { + const storyA = makeMockStory({ slug: "story-a" }); + const storyB = makeMockStory({ slug: "story-b" }); + const localStories = [storyA, storyB]; + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + preconditions.canListStories(localStories); + preconditions.failsToUpdateStories(localStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + // Every story already existed remotely, so all creations were + // skipped, not succeeded — "0 succeeded" must not fold skipped in. + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining("Creating stories: 0/2 succeeded, 0 failed, 2 skipped."), + ); + }); + + it("should stop after the first credential failure instead of repeating it per story", async () => { + const storyA = makeMockStory({ slug: "story-a" }); + const storyB = makeMockStory({ slug: "story-b" }); + const storyC = makeMockStory({ slug: "story-c" }); + const localStories = [storyA, storyB, storyC]; + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + const remoteStories = preconditions.canCreateStories(localStories); + preconditions.failsToUpdateStoriesWithInsufficientScope(remoteStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + // The shared harness mocks a PAT session, so the rendered message is the + // PAT-flavored variant ("Your personal access token is missing the + // \"stories:write\" scope…"), not the OAuth one. Assert on the scope + // name, which both variants contain, so this stays about the count + // rather than the wording. + const errorCalls = (console.error as unknown as ReturnType).mock.calls; + const rendered = errorCalls.map((call) => call[0]).join("\n"); + expect(rendered.match(/stories:write/g)).toHaveLength(1); + // An aborted run is not a silent success. + expect(process.exitCode).toBe(1); + }); + + it("should stop issuing update requests once a credential failure is detected, instead of attempting every story", async () => { + // Comfortably larger than the pipeline's concurrency limit (12 in-flight + // requests by default) so that, if the run kept going, the mock would + // see every story attempted. Proves the pipeline actually halts rather + // than merely going quiet about repeats. + const localStories = Array.from({ length: 40 }, (_, i) => + makeMockStory({ slug: `story-${i}` }), + ); + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + const remoteStories = preconditions.canCreateStories(localStories); + const requestCount = preconditions.failsToUpdateStoriesWithInsufficientScope(remoteStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + // Far below the 40 local stories: the pipeline stopped dispatching new + // update requests once the first credential failure was recorded. + expect(requestCount.current).toBeLessThan(localStories.length / 2); + expect(process.exitCode).toBe(1); + }); + + it("should name the stories an aborted run never attempted", async () => { + const localStories = Array.from({ length: 5 }, (_, i) => + makeMockStory({ slug: `story-${i}` }), + ); + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + const remoteStories = preconditions.canCreateStories(localStories); + preconditions.failsToUpdateStoriesWithInsufficientScope(remoteStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + // Without the "not attempted" tail this reads as "0/5 succeeded, 1 failed", leaving + // four stories silently unaccounted for and the abort looking like data loss. + const errorCalls = (console.error as unknown as ReturnType).mock.calls; + const rendered = errorCalls.map((call) => call[0]).join("\n"); + expect(rendered).toContain("Updating stories: 0/5 succeeded, 1 failed, 4 not attempted."); + }); + + it("should not add a not-attempted tail when every story is accounted for", async () => { + const localStories = [makeMockStory({ slug: "story-a" })]; + preconditions.canLoadStories(localStories); + preconditions.canLoadComponents([makeMockComponent({ name: "page" })]); + const remoteStories = preconditions.canCreateStories(localStories); + preconditions.canUpdateStories(remoteStories); + + await storiesCommand.parseAsync(["node", "test", "push", "--space", DEFAULT_SPACE]); + + expect(console.error).toHaveBeenCalledWith( + expect.stringContaining("Updating stories: 1/1 succeeded, 0 failed."), + ); + expect(console.error).not.toHaveBeenCalledWith(expect.stringContaining("not attempted")); + }); }); }); diff --git a/packages/cli/src/commands/stories/push/index.ts b/packages/cli/src/commands/stories/push/index.ts index 3bb17d871..62cc1b94b 100644 --- a/packages/cli/src/commands/stories/push/index.ts +++ b/packages/cli/src/commands/stories/push/index.ts @@ -32,6 +32,16 @@ import { prefetchTargetStoriesByKeys } from "../actions"; import { collectSchemaIssues, formatSchemaIssues, hasSchemaIssues } from "../validate-story"; import { FailureCollector } from "./failure-report"; +/** + * A fatal credential failure stops a phase mid-run, leaving stories the CLI deliberately + * never tried. Spelling those out keeps every phase line adding up to its total instead of + * leaving a silent shortfall between "succeeded" and the denominator. + */ +function formatNotAttempted(total: number, ...accounted: number[]): string { + const remaining = total - accounted.reduce((sum, count) => sum + count, 0); + return remaining > 0 ? `, ${remaining} not attempted` : ""; +} + const pushCmd = storiesCommand .command("push") .option("-s, --space ", "space ID") @@ -99,10 +109,14 @@ pushCmd.action(async (options, command) => { resolveCommandPath(directories.components, fromSpace, basePath), ); if (Object.keys(schemas).length === 0) { - const message = - "No components found. Please run `storyblok components pull` to fetch the latest components."; - ui.error(message); - logger.error(message); + // `handleError` rather than a bare `ui.error`: bailing on a precondition is still a + // failed push, and CI must see a non-zero exit for it. + handleError( + new CommandError( + "No components found. Please run `storyblok components pull` to fetch the latest components.", + ), + verbose, + ); return; } @@ -116,9 +130,8 @@ pushCmd.action(async (options, command) => { schemas, }); if (hasSchemaIssues(schemaIssues)) { - const message = formatSchemaIssues(schemaIssues); - ui.error(message); - logger.error(message); + // See the components precondition above: this aborts the push, so it must exit non-zero. + handleError(new CommandError(formatSchemaIssues(schemaIssues)), verbose); // Surface the failure in the run summary so the report status is // FAILURE rather than a trivial zero-counts SUCCESS. const total = Math.max(schemaIssues.total, 1); @@ -177,6 +190,8 @@ pushCmd.action(async (options, command) => { scanProgress.increment(); }, onError(error, filename) { + // No `hasFatal` guard here: a local filesystem/parse error can never + // be an `APIError`, so it can never be fatal — nothing to short-circuit. if (failures.record({ filename }, error)) { summary.creationResults.failed += 1; } @@ -273,6 +288,12 @@ pushCmd.action(async (options, command) => { creationProgress.increment(); }, onStoryError(error, entry) { + // A credential-level failure is identical for every remaining story, so + // once one is recorded, treat any already-queued concurrent entry as a + // no-op instead of repeating the same failure per story. + if (failures.hasFatal) { + return; + } if (failures.record(entry, error)) { summary.creationResults.failed += 1; summary.processResults.total -= 1; @@ -284,6 +305,12 @@ pushCmd.action(async (options, command) => { logOnlyError(error, { storyId: entry.uuid }); }, }); + // A credential failure inside this level was recorded; the same + // failure would repeat for every remaining level, so stop creating + // placeholders rather than issuing more doomed requests. + if (failures.hasFatal) { + break; + } } if (summary.creationResults.failed > 0) { @@ -304,6 +331,7 @@ pushCmd.action(async (options, command) => { // Read local stories from `.json` files. readLocalStoriesStream({ directoryPath: storiesDirectoryPath, + shouldStop: () => failures.hasFatal, fileFilter({ filename }) { // Only load files that were successfully created and mapped. const uuid = uuidByFilename.get(filename); @@ -316,6 +344,11 @@ pushCmd.action(async (options, command) => { updateProgress.setTotal(total); }, onStoryError(error, filename) { + // See the creation-phase `onStoryError` above: a credential-level + // failure is identical for every remaining story, so stop repeating it. + if (failures.hasFatal) { + return; + } if (failures.record({ filename }, error)) { summary.processResults.failed += 1; } else { @@ -333,6 +366,7 @@ pushCmd.action(async (options, command) => { mapReferencesStream({ schemas, maps, + shouldStop: () => failures.hasFatal, onIncrement() { processProgress.increment(); }, @@ -344,8 +378,15 @@ pushCmd.action(async (options, command) => { onStoryError(error, localStory) { // Always keep the audit trail — even when we suppress the // user-facing summary entry for stories that already failed - // creation, the file log should retain the full per-phase error. + // creation (or that are skipped below because a credential + // failure was already recorded), the file log should retain the + // full per-phase error. logOnlyError(error, { storyId: localStory.uuid }); + // See the creation-phase `onStoryError` above: a credential-level + // failure is identical for every remaining story, so stop repeating it. + if (failures.hasFatal) { + return; + } if (failures.record(localStory, error)) { summary.processResults.failed += 1; } else { @@ -358,6 +399,7 @@ pushCmd.action(async (options, command) => { }), // Update remote stories with correct references. writeStoryStream({ + shouldStop: () => failures.hasFatal, transports: { writeStory: options.dryRun ? async (story: Story) => story @@ -378,6 +420,11 @@ pushCmd.action(async (options, command) => { summary.updateResults.succeeded += 1; }, onStoryError(error, localStory) { + // See the creation-phase `onStoryError` above: a credential-level + // failure is identical for every remaining story, so stop repeating it. + if (failures.hasFatal) { + return; + } logOnlyError(error, { storyId: localStory.uuid }); if (failures.record(localStory, error)) { summary.updateResults.failed += 1; @@ -398,13 +445,23 @@ pushCmd.action(async (options, command) => { ui.br(); const failedCount = failures.size; + // A story's content is only actually written to the remote during the + // update phase (creation only reserves a placeholder id, and a + // newly-created story still goes through update to receive its real + // content). So `updateResults.succeeded` alone is "stories actually + // pushed" — summing it with `creationResults.succeeded` would double + // count every newly-created story that also updated successfully. + // `skipped` creations still proceed to update and are counted there, so + // they are neither a success nor a failure of their own and get no + // separate weight in this headline. + const pushedCount = summary.updateResults.succeeded; ui.info( - `Push results: ${summary.creationResults.total} ${summary.creationResults.total === 1 ? "story" : "stories"} pushed, ${failedCount} ${failedCount === 1 ? "story" : "stories"} failed`, + `Push results: ${pushedCount} ${pushedCount === 1 ? "story" : "stories"} pushed, ${failedCount} ${failedCount === 1 ? "story" : "stories"} failed`, ); ui.list([ - `Creating stories: ${summary.creationResults.succeeded + summary.creationResults.skipped}/${summary.creationResults.total} succeeded, ${summary.creationResults.failed} failed.`, - `Processing stories: ${summary.processResults.succeeded}/${summary.processResults.total} succeeded, ${summary.processResults.failed} failed.`, - `Updating stories: ${summary.updateResults.succeeded}/${summary.updateResults.total} succeeded, ${summary.updateResults.failed} failed.`, + `Creating stories: ${summary.creationResults.succeeded}/${summary.creationResults.total} succeeded, ${summary.creationResults.failed} failed, ${summary.creationResults.skipped} skipped${formatNotAttempted(summary.creationResults.total, summary.creationResults.succeeded, summary.creationResults.failed, summary.creationResults.skipped)}.`, + `Processing stories: ${summary.processResults.succeeded}/${summary.processResults.total} succeeded, ${summary.processResults.failed} failed${formatNotAttempted(summary.processResults.total, summary.processResults.succeeded, summary.processResults.failed)}.`, + `Updating stories: ${summary.updateResults.succeeded}/${summary.updateResults.total} succeeded, ${summary.updateResults.failed} failed${formatNotAttempted(summary.updateResults.total, summary.updateResults.succeeded, summary.updateResults.failed)}.`, ]); if (pendingWarnings.length > 0 || !failures.isEmpty) { @@ -428,5 +485,14 @@ pushCmd.action(async (options, command) => { reporter.addMeta("failedStories", failures.toReporterMeta()); } reporter.finalize(); + + // Per-story failures (including a credential-level one) go through + // `logOnlyError`, not `handleError`, so they never touch `process.exitCode` + // on their own — without this, a push that failed every story would still + // exit 0. Guarded so we never downgrade an exit code a thrown error + // already set via `handleError` in the `catch` block above. + if (process.exitCode === undefined) { + process.exitCode = failures.isEmpty ? 0 : 1; + } } }); diff --git a/packages/cli/src/commands/stories/streams.ts b/packages/cli/src/commands/stories/streams.ts index 174a2173f..0613f03e5 100644 --- a/packages/cli/src/commands/stories/streams.ts +++ b/packages/cli/src/commands/stories/streams.ts @@ -141,6 +141,7 @@ export const readLocalStoriesStream = ({ onIncrement, onStorySuccess, onStoryError, + shouldStop, }: { directoryPath: string; /** @@ -155,6 +156,13 @@ export const readLocalStoriesStream = ({ onIncrement?: () => void; onStorySuccess?: (story: Story) => void; onStoryError?: (error: Error, filename: string) => void; + /** + * Checked before each file is read. Once it returns `true`, the generator + * stops yielding — a clean end-of-stream, not an error — so a caller-level + * fatal failure (e.g. a credential error) halts the rest of the pipeline + * instead of reading and forwarding every remaining file. + */ + shouldStop?: () => boolean; }) => { const listGenerator = async function* localStoryIterator() { const files = (await readDirectory(directoryPath)).filter( @@ -163,6 +171,9 @@ export const readLocalStoriesStream = ({ setTotalStories?.(files.length); for (const file of files) { + if (shouldStop?.()) { + break; + } try { const filePath = join(directoryPath, file); const fileContent = await readFile(filePath, "utf-8"); @@ -186,16 +197,27 @@ export const mapReferencesStream = ({ onIncrement, onStorySuccess, onStoryError, + shouldStop, }: { schemas: ComponentSchemas; maps: RefMaps; onIncrement?: () => void; onStorySuccess?: (localStory: Story) => void; onStoryError?: (error: Error, story: Story) => void; + /** + * Checked before each chunk is mapped. Once it returns `true`, the chunk is + * dropped without pushing it downstream, so a caller-level fatal failure + * stops feeding `writeStoryStream` further work. + */ + shouldStop?: () => boolean; }) => { return new Transform({ objectMode: true, transform(localStory: Story, _encoding, callback) { + if (shouldStop?.()) { + callback(); + return; + } try { const mappedStory = storyRefMapper(localStory, { schemas, maps }); onStorySuccess?.(mappedStory); @@ -527,6 +549,7 @@ export const writeStoryStream = ({ onIncrement, onStorySuccess, onStoryError, + shouldStop, }: { transports: { writeStory: WriteStoryTransport; @@ -535,12 +558,25 @@ export const writeStoryStream = ({ onIncrement?: () => void; onStorySuccess?: (mappedLocalStory: Story, remoteStory: Story) => void; onStoryError?: (error: Error, story: Story) => void; + /** + * Checked before each write is dispatched. Once it returns `true`, the + * story is dropped without calling `writeStory` (no API call, no + * increment) — a caller-level fatal failure stops issuing further + * requests instead of repeating the same failure per story. Stories + * already dispatched to the transport before the flag flipped still run + * to completion; only not-yet-started work is skipped. + */ + shouldStop?: () => boolean; }) => { const processing = new Set>(); return new Writable({ objectMode: true, async write(mappedLocalStory: Story, _encoding, callback) { + if (shouldStop?.()) { + callback(); + return; + } await getPipelineSlot().acquire(); const task = (async () => { diff --git a/packages/cli/src/commands/user/actions.test.ts b/packages/cli/src/commands/user/actions.test.ts index 379594fda..0cc85041f 100644 --- a/packages/cli/src/commands/user/actions.test.ts +++ b/packages/cli/src/commands/user/actions.test.ts @@ -2,6 +2,7 @@ import { getUser } from "./actions"; import { http, HttpResponse } from "msw"; import { setupServer } from "msw/node"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import { resetCredentialContext, setCredentialContext } from "../../utils"; const handlers = [ http.get("https://mapi.storyblok.com/v1/users/me", async ({ request }) => { @@ -19,7 +20,10 @@ const server = setupServer(...handlers); beforeAll(() => server.listen({ onUnhandledRequest: "error" })); -afterEach(() => server.resetHandlers()); +afterEach(() => { + server.resetHandlers(); + resetCredentialContext(); +}); afterAll(() => server.close()); describe("user actions", () => { @@ -37,8 +41,7 @@ describe("user actions", () => { it("should throw an masked error for invalid token", async () => { await expect(getUser("invalid-token", "eu")).rejects.toThrow( - `The token provided inva********* is invalid. - Please make sure you are using the correct token and try again.`, + "The token provided inva********* is invalid. Please make sure you are using the correct token and try again.", ); }); @@ -51,3 +54,26 @@ describe("user actions", () => { await expect(getUser("any-token", "eu")).rejects.toThrow("The server returned an error"); }); }); + +describe("getUser credential errors", () => { + it("should use the centralized message for an OAuth session", async () => { + setCredentialContext({ kind: "oauth" }); + + await expect(getUser({ oauthToken: "sb_oat_dead" }, "eu")).rejects.toThrow( + "Your OAuth login is no longer valid", + ); + }); + + it("should use the centralized message for a PAT session", async () => { + setCredentialContext({ kind: "pat" }); + + await expect(getUser({ personalAccessToken: "sb_pat_dead" }, "eu")).rejects.toThrow( + "Your personal access token was rejected", + ); + }); + + it("should keep the masked-token message while validating a token at login", async () => { + // No session yet: the context is still unknown, so the matcher stays inactive. + await expect(getUser("sb_pat_invalid", "eu")).rejects.toThrow("is invalid"); + }); +}); diff --git a/packages/cli/src/commands/user/actions.ts b/packages/cli/src/commands/user/actions.ts index 96e90cb22..36dca45d0 100644 --- a/packages/cli/src/commands/user/actions.ts +++ b/packages/cli/src/commands/user/actions.ts @@ -1,6 +1,12 @@ import chalk from "chalk"; import type { ApiCredential } from "../../utils"; -import { getResponseStatus, handleAPIError, maskToken, toError } from "../../utils"; +import { + getCredentialContext, + getResponseStatus, + handleAPIError, + maskToken, + toError, +} from "../../utils"; import { createMapiClient } from "../../api"; import type { RegionCode } from "../../constants"; @@ -14,7 +20,6 @@ export type { User } from "../../types"; export const getUser = async (credential: string | ApiCredential, region: RegionCode) => { const config: ApiCredential = typeof credential === "string" ? { personalAccessToken: credential } : credential; - const isOauth = "oauthToken" in config; const token = "personalAccessToken" in config ? config.personalAccessToken : config.oauthToken; try { const client = createMapiClient({ @@ -30,13 +35,12 @@ export const getUser = async (credential: string | ApiCredential, region: Region } catch (maybeError) { const error = toError(maybeError); const status = getResponseStatus(maybeError); + // Only when no session is established, which is the `login --token` validation path. + // With a session present, the centralized credential rewrite owns 401 messaging, and a + // customMessage here would suppress it. const customMessage = - status === 401 - ? isOauth - ? `Your OAuth session has expired or been revoked. - Please run \`storyblok login --oauth\` to authenticate again.` - : `The token provided ${chalk.bold(maskToken(token))} is invalid. - Please make sure you are using the correct token and try again.` + status === 401 && getCredentialContext().kind === "unknown" + ? `The token provided ${chalk.bold(maskToken(token))} is invalid. Please make sure you are using the correct token and try again.` : undefined; handleAPIError("get_user", error, customMessage); } diff --git a/packages/cli/src/program.credential-context.test.ts b/packages/cli/src/program.credential-context.test.ts new file mode 100644 index 000000000..6f38cf02a --- /dev/null +++ b/packages/cli/src/program.credential-context.test.ts @@ -0,0 +1,33 @@ +// Integration coverage for credential-context wiring in the shared preAction hook. +// The matcher cannot be verified in isolation: only a real command run proves the +// context is populated before the action executes. +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { getCredentialContext, resetCredentialContext } from "./utils/error/credential-context"; + +vi.mock("./api", () => ({ + getMapiClient: vi.fn(), +})); + +describe("program preAction credential context", () => { + beforeEach(() => { + resetCredentialContext(); + vi.clearAllMocks(); + }); + + it("should expose the PAT session to the error layer", async () => { + // Deliberately no vi.resetModules() here: resetting the module registry would + // give program.ts a fresh instance of ./utils/error/credential-context, distinct + // from the one statically imported above, so setCredentialContext and + // getCredentialContext would read and write different singletons. + const { getProgram } = await import("./program"); + const program = getProgram(); + program + .command("context-probe-pat") + .option("-s, --space ", "space ID") + .action(() => {}); + + await program.parseAsync(["node", "test", "context-probe-pat", "--space", "4242"]); + + expect(getCredentialContext()).toMatchObject({ kind: "pat", space: "4242" }); + }); +}); diff --git a/packages/cli/src/program.ts b/packages/cli/src/program.ts index 5d50cab6b..c1f82cded 100644 --- a/packages/cli/src/program.ts +++ b/packages/cli/src/program.ts @@ -15,6 +15,7 @@ import { getMapiClient } from "./api"; import { isExpiringSoon } from "./commands/oauth/expiry"; import { refreshOAuthTokens } from "./commands/oauth/refresh"; import { assertSpaceAllowed } from "./commands/oauth/space-guard"; +import { setCredentialContext } from "./utils/error/credential-context"; import { applyConfigToCommander, getCommandAncestry, @@ -105,6 +106,14 @@ export function getProgram(): Command { }); } + // Tell the error layer which credential is in play so 401/403 responses can name + // the right remedy. Set for every credential kind, including none at all. + setCredentialContext({ + kind: state.authType ?? "unknown", + spaces: state.oauthSpaces, + space: targetCommand.optsWithGlobals().space, + }); + // Guard OAuth sessions against operating on spaces outside their consent grant. // A thrown CommandError here propagates out of the preAction hook, rejecting // `program.parseAsync()` in index.ts, which handles it once at the top level. diff --git a/packages/cli/src/utils/error/api-error.test.ts b/packages/cli/src/utils/error/api-error.test.ts index 504984699..280d9727a 100644 --- a/packages/cli/src/utils/error/api-error.test.ts +++ b/packages/cli/src/utils/error/api-error.test.ts @@ -1,6 +1,7 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it } from "vitest"; import { ClientError } from "@storyblok/management-api-client"; -import { APIError, handleAPIError } from "./api-error"; +import { API_ACTIONS, APIError, handleAPIError } from "./api-error"; +import { resetCredentialContext, setCredentialContext } from "./credential-context"; import { FetchError } from "../fetch"; // ClientError tests verify that mapi-client errors (which have a .response property) @@ -416,3 +417,109 @@ describe("aPIError server message extraction", () => { } }); }); + +describe("APIError credential rewrites", () => { + afterEach(() => { + resetCredentialContext(); + }); + + it("should classify a plain 403 as forbidden rather than generic", () => { + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: {}, + }); + + try { + handleAPIError("push_component", error); + } catch (e) { + expect((e as APIError).errorId).toBe("forbidden"); + expect((e as APIError).code).toBe(403); + } + }); + + it("should rewrite an insufficient-scope 403 into an actionable message", () => { + setCredentialContext({ kind: "oauth" }); + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }); + + try { + handleAPIError("update_story", error); + } catch (e) { + const apiError = e as APIError; + expect(apiError.errorId).toBe("insufficient_scope"); + expect(apiError.fatal).toBe(true); + expect(apiError.message).toBe( + 'Your OAuth login is missing the "stories:write" permission. Re-run `storyblok login` and grant it at the consent screen.', + ); + } + }); + + it("should keep the failed action as the first message stack entry", () => { + setCredentialContext({ kind: "oauth" }); + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }); + + try { + handleAPIError("update_story", error); + } catch (e) { + const apiError = e as APIError; + expect(apiError.messageStack[0]).toBe(API_ACTIONS.update_story); + expect(apiError.messageStack.at(-1)).toBe(apiError.message); + } + }); + + it("should leave the raw server string alone when the credential kind is unknown", () => { + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }); + + try { + handleAPIError("update_story", error); + } catch (e) { + const apiError = e as APIError; + expect(apiError.message).toBe("Insufficient scope: stories:write is required"); + expect(apiError.fatal).toBe(false); + } + }); + + it("should let a customMessage suppress the credential rewrite entirely", () => { + setCredentialContext({ kind: "oauth" }); + const error = new FetchError("Forbidden", { + status: 403, + statusText: "Forbidden", + data: { error: "Insufficient scope: stories:write is required" }, + }); + + try { + handleAPIError("update_story", error, "Custom override message"); + } catch (e) { + const apiError = e as APIError; + expect(apiError.message).toBe("Custom override message"); + expect(apiError.fatal).toBe(false); + } + }); + + it("should not mark unrelated errors as fatal", () => { + setCredentialContext({ kind: "oauth" }); + const error = new FetchError("Unprocessable", { + status: 422, + statusText: "Unprocessable Entity", + data: { slug: ["has already been taken"] }, + }); + + try { + handleAPIError("create_story", error); + } catch (e) { + expect((e as APIError).fatal).toBe(false); + } + }); +}); diff --git a/packages/cli/src/utils/error/api-error.ts b/packages/cli/src/utils/error/api-error.ts index 79bf6e4e3..b5bd74004 100644 --- a/packages/cli/src/utils/error/api-error.ts +++ b/packages/cli/src/utils/error/api-error.ts @@ -1,3 +1,5 @@ +import { getCredentialContext } from "./credential-context"; +import { matchCredentialError } from "./credential-hint"; import { FetchError } from "../fetch"; export const API_ACTIONS = { @@ -40,7 +42,9 @@ export const API_ACTIONS = { transfer_asset: "Failed to transfer asset", pull_shared_assets: "Failed to pull library assets", pull_shared_asset: "Failed to pull library asset", - pull_shared_asset_folders: "Failed to pull library folders", + // Folder discovery runs on both the pull and the push path, so this stays verb-neutral: + // a push must not report a failure to "pull" anything. + list_shared_asset_folders: "Failed to list library folders", pull_shared_asset_folder: "Failed to pull library folder", pull_shared_internal_tags: "Failed to pull library tags", push_shared_asset_create: "Failed to create library asset", @@ -69,6 +73,8 @@ export const API_ERRORS = { not_found: "The requested resource was not found", unprocessable_entity: "The request was well-formed but was unable to be followed due to semantic errors", + forbidden: "The user is not allowed to perform this action", + insufficient_scope: "The credential is missing a required permission", } as const; function getErrorId(status: number): keyof typeof API_ERRORS { @@ -79,6 +85,8 @@ function getErrorId(status: number): keyof typeof API_ERRORS { return "not_found"; case 422: return "unprocessable_entity"; + case 403: + return "forbidden"; default: return status >= 500 ? "server_error" : "generic"; } @@ -180,6 +188,15 @@ export class APIError extends Error { messageStack: string[]; error: FetchError | undefined; response: FetchError["response"] | undefined; + /** True when the failure is credential-level, so bulk loops should stop instead of retrying. */ + fatal: boolean; + /** + * The raw `data.error`/`data.message` string extracted from the response, before any + * rewrite. Undefined when a `customMessage` suppressed extraction, or none was present. + * Callers that need to distinguish specific server signatures (e.g. the unsupported-token-type + * 403) beyond the generic `errorId`/`fatal` classification should match on this. + */ + serverError: string | undefined; constructor( errorId: keyof typeof API_ERRORS, action: keyof typeof API_ACTIONS, @@ -194,6 +211,8 @@ export class APIError extends Error { this.messageStack = []; this.error = error; this.response = error?.response; + this.fatal = false; + this.serverError = undefined; if (!customMessage) { this.messageStack.push(API_ACTIONS[action]); @@ -206,6 +225,7 @@ export class APIError extends Error { const serverMessage = customMessage ? undefined : extractServerString(responseData ?? {}, this.code, statusText); + this.serverError = serverMessage; const stackLengthBefore422 = this.messageStack.length; @@ -243,6 +263,20 @@ export class APIError extends Error { this.cause = this.message; } } + + // A credential-level 401/403 gets a rewritten, actionable message. This runs last so it + // wins over the raw server string, and replaces only the final stack entry so the + // `API_ACTIONS[action]` context line above it survives. + const hint = customMessage + ? undefined + : matchCredentialError(this.code, serverMessage, getCredentialContext()); + if (hint) { + this.errorId = hint.errorId; + this.message = hint.message; + this.cause = hint.message; + this.fatal = hint.fatal; + this.messageStack[this.messageStack.length - 1] = hint.message; + } } getInfo() { diff --git a/packages/cli/src/utils/error/credential-context.test.ts b/packages/cli/src/utils/error/credential-context.test.ts new file mode 100644 index 000000000..23b2096b2 --- /dev/null +++ b/packages/cli/src/utils/error/credential-context.test.ts @@ -0,0 +1,40 @@ +import { beforeEach, describe, expect, it } from "vitest"; +import { + getCredentialContext, + resetCredentialContext, + setCredentialContext, +} from "./credential-context"; + +describe("credential context", () => { + beforeEach(() => { + resetCredentialContext(); + }); + + it("should default to an unknown credential kind", () => { + expect(getCredentialContext()).toEqual({ kind: "unknown" }); + }); + + it("should return the context that was set", () => { + setCredentialContext({ kind: "oauth", spaces: [{ id: 1, region: "eu" }], space: "1" }); + + expect(getCredentialContext()).toEqual({ + kind: "oauth", + spaces: [{ id: 1, region: "eu" }], + space: "1", + }); + }); + + it("should replace the previous context rather than merging into it", () => { + setCredentialContext({ kind: "oauth", spaces: [{ id: 1, region: "eu" }] }); + setCredentialContext({ kind: "pat" }); + + expect(getCredentialContext()).toEqual({ kind: "pat" }); + }); + + it("should reset back to unknown", () => { + setCredentialContext({ kind: "pat" }); + resetCredentialContext(); + + expect(getCredentialContext()).toEqual({ kind: "unknown" }); + }); +}); diff --git a/packages/cli/src/utils/error/credential-context.ts b/packages/cli/src/utils/error/credential-context.ts new file mode 100644 index 000000000..8222218dd --- /dev/null +++ b/packages/cli/src/utils/error/credential-context.ts @@ -0,0 +1,34 @@ +export type CredentialKind = "oauth" | "pat" | "unknown"; + +/** + * The active credential, pushed in by `program.ts`'s preAction hook so the error + * layer can tailor remedies without importing `session.ts` (which would create an + * import cycle through the `utils` barrel). + * + * `kind` stays `"unknown"` until a session is initialized. That is deliberate: + * during `storyblok login` there is no session yet, and the login actions must keep + * their own messages. + */ +export type CredentialContext = { + kind: CredentialKind; + /** Spaces the grant is restricted to, when known. Empty or absent means unrestricted. */ + spaces?: { id: number; region: string }[]; + /** The space the current command targets, when one was resolved. */ + space?: string | number; +}; + +const UNKNOWN_CONTEXT: CredentialContext = { kind: "unknown" }; + +let currentContext: CredentialContext = UNKNOWN_CONTEXT; + +export function setCredentialContext(context: CredentialContext): void { + currentContext = context; +} + +export function getCredentialContext(): CredentialContext { + return currentContext; +} + +export function resetCredentialContext(): void { + currentContext = UNKNOWN_CONTEXT; +} diff --git a/packages/cli/src/utils/error/credential-hint.test.ts b/packages/cli/src/utils/error/credential-hint.test.ts new file mode 100644 index 000000000..f449717a6 --- /dev/null +++ b/packages/cli/src/utils/error/credential-hint.test.ts @@ -0,0 +1,160 @@ +import { describe, expect, it } from "vitest"; +import type { CredentialContext } from "./credential-context"; +import { formatSpaceNotAllowedMessage, matchCredentialError } from "./credential-hint"; + +const oauth: CredentialContext = { kind: "oauth" }; +const pat: CredentialContext = { kind: "pat" }; +const unknown: CredentialContext = { kind: "unknown" }; + +describe("matchCredentialError", () => { + it("should name the missing permission and the consent screen for an OAuth session", () => { + const hint = matchCredentialError(403, "Insufficient scope: stories:write is required", oauth); + + expect(hint).toEqual({ + errorId: "insufficient_scope", + fatal: true, + message: + 'Your OAuth login is missing the "stories:write" permission. Re-run `storyblok login` and grant it at the consent screen.', + }); + }); + + it("should name the missing scope and token creation for a PAT session", () => { + const hint = matchCredentialError(403, "Insufficient scope: assets:write is required", pat); + + expect(hint?.errorId).toBe("insufficient_scope"); + expect(hint?.message).toBe( + 'Your personal access token is missing the "assets:write" scope. Create a new token with that scope under My account, Personal access tokens.', + ); + }); + + it("should tell OAuth users to use a personal access token for unsupported endpoints", () => { + const hint = matchCredentialError(403, "This endpoint does not support this token type", oauth); + + expect(hint?.errorId).toBe("forbidden"); + expect(hint?.message).toBe( + "This command is not available with an OAuth login yet. Re-run `storyblok login --token ` with a personal access token instead.", + ); + }); + + it("should tell PAT users to sign in with email and password for unsupported endpoints", () => { + const hint = matchCredentialError(403, "This endpoint does not support this token type", pat); + + expect(hint?.message).toBe( + "This command is not available with a personal access token. Run `storyblok login` and sign in with your email and password.", + ); + }); + + it("should list the authorized space ids when the credential is space restricted", () => { + const hint = matchCredentialError(403, "This token is restricted to specific spaces", { + kind: "oauth", + spaces: [ + { id: 12345, region: "eu" }, + { id: 67890, region: "eu" }, + ], + }); + + expect(hint?.message).toBe( + "Your OAuth login is limited to specific spaces. Pass --space with one of: 12345, 67890.", + ); + }); + + it("should fall back to a generic --space hint when the space ids are unknown", () => { + const hint = matchCredentialError(403, "This token is restricted to specific spaces", oauth); + + expect(hint?.message).toBe( + "Your OAuth login is limited to specific spaces. Pass --space .", + ); + }); + + it("should reuse the pre-flight wording when the space is outside the grant", () => { + const hint = matchCredentialError(403, "This token does not have access to this space", { + kind: "oauth", + space: 999, + spaces: [ + { id: 1, region: "eu" }, + { id: 2, region: "eu" }, + ], + }); + + expect(hint?.message).toBe(formatSpaceNotAllowedMessage(999, [1, 2])); + }); + + it("should name the personal access token and a remedy it can act on for a space-restricted PAT", () => { + const hint = matchCredentialError(403, "This token does not have access to this space", { + kind: "pat", + space: 222, + spaces: [], + }); + + expect(hint?.message).toBe( + "Space 222 is not covered by your personal access token. Create a new token that covers this space under My account, Personal access tokens.", + ); + }); + + it("should omit the empty parenthetical for an OAuth grant with no known space list", () => { + const hint = matchCredentialError(403, "This token does not have access to this space", { + kind: "oauth", + space: 222, + spaces: [], + }); + + expect(hint?.message).toBe( + "Space 222 is not covered by your OAuth login. Re-run `storyblok login` and select this space at the consent screen.", + ); + expect(hint?.message).not.toContain("(authorized spaces: )"); + }); + + it("should treat a 401 as a dead session regardless of the body", () => { + expect(matchCredentialError(401, "Unauthorized", oauth)?.message).toBe( + "Your OAuth login is no longer valid, it may have been revoked or expired. Run `storyblok login` to sign in again.", + ); + expect(matchCredentialError(401, undefined, pat)?.message).toBe( + "Your personal access token was rejected. It may have been revoked, create a new one and run `storyblok login --token `.", + ); + }); + + it("should mark every credential failure as fatal", () => { + const cases = [ + matchCredentialError(403, "Insufficient scope: stories:write is required", oauth), + matchCredentialError(403, "This endpoint does not support this token type", oauth), + matchCredentialError(403, "This token is restricted to specific spaces", oauth), + matchCredentialError(403, "This token does not have access to this space", oauth), + matchCredentialError(401, "Unauthorized", oauth), + ]; + + expect(cases.every((hint) => hint?.fatal === true)).toBe(true); + }); + + it("should ignore every status when the credential kind is unknown", () => { + expect( + matchCredentialError(403, "Insufficient scope: stories:write is required", unknown), + ).toBeUndefined(); + expect(matchCredentialError(401, "Unauthorized", unknown)).toBeUndefined(); + }); + + it("should not claim unrelated forbidden responses", () => { + expect( + matchCredentialError(403, "Asset conversion is not available on this plan", oauth), + ).toBeUndefined(); + expect(matchCredentialError(403, undefined, oauth)).toBeUndefined(); + }); + + it("should ignore statuses that are not 401 or 403", () => { + expect(matchCredentialError(422, "slug has already been taken", oauth)).toBeUndefined(); + expect(matchCredentialError(404, "Not Found", oauth)).toBeUndefined(); + expect(matchCredentialError(500, "Internal Server Error", oauth)).toBeUndefined(); + }); + + it("should emit single-line messages only", () => { + const messages = [ + matchCredentialError(403, "Insufficient scope: stories:write is required", oauth)?.message, + matchCredentialError(403, "This endpoint does not support this token type", pat)?.message, + matchCredentialError(401, "Unauthorized", oauth)?.message, + formatSpaceNotAllowedMessage(9, [1]), + ]; + + for (const message of messages) { + expect(message).not.toContain("\n"); + } + }); +}); diff --git a/packages/cli/src/utils/error/credential-hint.ts b/packages/cli/src/utils/error/credential-hint.ts new file mode 100644 index 000000000..a37ffd373 --- /dev/null +++ b/packages/cli/src/utils/error/credential-hint.ts @@ -0,0 +1,126 @@ +import type { CredentialContext } from "./credential-context"; + +export type CredentialHint = { + errorId: "insufficient_scope" | "forbidden" | "unauthorized"; + message: string; + /** Credential failures are identical for every item, so bulk loops stop on the first one. */ + fatal: true; +}; + +// Signatures rendered by storyrails: scope_enforceable.rb and application_controller.rb. +const INSUFFICIENT_SCOPE = /^Insufficient scope: (\S+) is required$/; +const UNSUPPORTED_TOKEN_TYPE = "This endpoint does not support this token type"; +const SPACE_RESTRICTED = "This token is restricted to specific spaces"; +const SPACE_NOT_ALLOWED = "This token does not have access to this space"; + +function formatAuthorizedSpacesParenthetical(grantedSpaceIds: number[]): string { + return grantedSpaceIds.length > 0 ? ` (authorized spaces: ${grantedSpaceIds.join(", ")})` : ""; +} + +/** + * Shared by the `oauth/space-guard.ts` pre-flight check and the post-flight 403 branch, + * so a user sees identical wording whichever one catches the problem first. OAuth only: + * the backend's `enforce_pat_space_restriction` check runs for any scoped credential, but + * the pre-flight guard this mirrors only ever runs for an OAuth grant (see `program.ts`). + */ +export function formatSpaceNotAllowedMessage( + space: string | number, + grantedSpaceIds: number[], +): string { + return ( + `Space ${space} is not covered by your OAuth login${formatAuthorizedSpacesParenthetical(grantedSpaceIds)}. ` + + `Re-run \`storyblok login\` and select this space at the consent screen.` + ); +} + +function formatPatSpaceNotAllowedMessage( + space: string | number, + grantedSpaceIds: number[], +): string { + return ( + `Space ${space} is not covered by your personal access token${formatAuthorizedSpacesParenthetical(grantedSpaceIds)}. ` + + `Create a new token that covers this space under My account, Personal access tokens.` + ); +} + +/** + * Identifies the credential-does-not-support-this-endpoint signature, distinct from other + * `forbidden`-classified cases (space restriction) that must still fail loudly rather than + * degrade gracefully. See `assets/scope.ts#listLibrariesOrDegrade`. + */ +export function isUnsupportedTokenTypeServerError(serverError: string | undefined): boolean { + return serverError === UNSUPPORTED_TOKEN_TYPE; +} + +function credentialLabel(kind: CredentialContext["kind"]): string { + return kind === "oauth" ? "Your OAuth login" : "Your personal access token"; +} + +export function matchCredentialError( + status: number, + serverError: string | undefined, + context: CredentialContext, +): CredentialHint | undefined { + if (context.kind === "unknown") { + return undefined; + } + const isOAuth = context.kind === "oauth"; + + if (status === 401) { + return { + errorId: "unauthorized", + fatal: true, + message: isOAuth + ? "Your OAuth login is no longer valid, it may have been revoked or expired. Run `storyblok login` to sign in again." + : "Your personal access token was rejected. It may have been revoked, create a new one and run `storyblok login --token `.", + }; + } + + if (status !== 403 || !serverError) { + return undefined; + } + + const missingScope = serverError.match(INSUFFICIENT_SCOPE)?.[1]; + if (missingScope) { + return { + errorId: "insufficient_scope", + fatal: true, + message: isOAuth + ? `Your OAuth login is missing the "${missingScope}" permission. Re-run \`storyblok login\` and grant it at the consent screen.` + : `Your personal access token is missing the "${missingScope}" scope. Create a new token with that scope under My account, Personal access tokens.`, + }; + } + + if (serverError === UNSUPPORTED_TOKEN_TYPE) { + return { + errorId: "forbidden", + fatal: true, + message: isOAuth + ? "This command is not available with an OAuth login yet. Re-run `storyblok login --token ` with a personal access token instead." + : "This command is not available with a personal access token. Run `storyblok login` and sign in with your email and password.", + }; + } + + if (serverError === SPACE_RESTRICTED) { + const ids = context.spaces?.map((space) => space.id) ?? []; + const target = ids.length > 0 ? `one of: ${ids.join(", ")}` : ""; + return { + errorId: "forbidden", + fatal: true, + message: `${credentialLabel(context.kind)} is limited to specific spaces. Pass --space ${ids.length > 0 ? "with " : ""}${target}.`, + }; + } + + if (serverError === SPACE_NOT_ALLOWED) { + const ids = context.spaces?.map((space) => space.id) ?? []; + return { + errorId: "forbidden", + fatal: true, + message: isOAuth + ? formatSpaceNotAllowedMessage(context.space ?? "unknown", ids) + : formatPatSpaceNotAllowedMessage(context.space ?? "unknown", ids), + }; + } + + return undefined; +} diff --git a/packages/cli/src/utils/error/index.ts b/packages/cli/src/utils/error/index.ts index e9c33637e..3227feddb 100644 --- a/packages/cli/src/utils/error/index.ts +++ b/packages/cli/src/utils/error/index.ts @@ -1,3 +1,5 @@ +export * from "./credential-context"; +export * from "./credential-hint"; export * from "./api-error"; export * from "./command-error"; export * from "./error"; diff --git a/packages/cli/test/GUIDE.md b/packages/cli/test/GUIDE.md index 004bf2d3f..298a92403 100644 --- a/packages/cli/test/GUIDE.md +++ b/packages/cli/test/GUIDE.md @@ -26,6 +26,14 @@ files. - IMPORTANT: When running `assets push --update-stories` or `stories push`, make sure you run `components pull` first! +- `stories push` stops at the first credential-level failure (for example a missing scope) instead + of repeating the same error for every remaining story, and it now exits 1 whenever any story + failed, where it previously always exited 0. Watch for this when scripting CI checks around the + exit code. +- The `stories push` summary headline ("N stories pushed") counts only stories whose content was + actually written, not stories attempted. A run that creates a placeholder but fails to write its + content reports 0 pushed; skipped creations get their own "skipped" label in the per-phase rows + instead of counting as succeeded. ### Scenario seeds @@ -68,14 +76,43 @@ it, and `logout` revokes the grant server-side. `PATH` swallows the launch. The authorize URL is also printed to stderr, so you can paste it into a browser yourself to finish consent. - **Occupy port 4900 with `nc -l 127.0.0.1 4900`, not `nc -l 4900`.** The wildcard bind does not - conflict with the CLI's loopback bind under `SO_REUSEADDR`, so the CLI starts normally and hangs - for the full 5 minute callback timeout instead of reporting the conflict. + conflict with the CLI's loopback bind under `SO_REUSEADDR`, so the CLI never notices it. Against a + loopback bind the CLI fails before it opens a browser, naming the blocking process and its PID. Worth checking manually: port 4900 occupied, denied consent, out-of-grant space (restrict `permitted_space_ids` via `PUT /v1/oauth_clients/`), and refresh (set `expires_at` to the past, then run any command). Manage apps through `/v1/oauth_clients` with a Personal Access Token, where `client_id` is `oauth_identifier` and `client_secret` is `oauth_secret`. +### Headless consent + +Consent needs a logged-in `app.storyblok.com` session, but that session can be minted from a +Personal Access Token instead of a browser, which makes the whole flow scriptable: + +1. `POST /oauth/init` with `Authorization: ` sets `session[:user_id]` and returns a + `_storyrails_session` cookie. Keep it in a cookie jar. +2. `GET /oauth/authorize?` with that cookie renders the consent page. Take the query string + from the authorize URL the CLI prints to stderr, and swap `/oauth/init` for `/oauth/authorize` + (`/oauth/init` only bounces to the SPA, which primes the session step 1 already did). +3. Parse the page for `authenticity_token`, the hidden OAuth params, the `scope[]` values, and the + `space_ids[]` checkbox values. `POST /oauth/authorize` with those plus `approve=true`, or + `deny=true` to test a refused grant. +4. Follow the `Location` header, which points at `http://localhost:4900/oauth/callback`, to hand the + code to the waiting CLI. + +Drop scopes or space IDs from that POST to produce a genuinely narrow grant, which is the only way +to exercise the scoped-credential errors against the real API: + +- **Space restriction.** Post a single `space_ids[]`. Any other space then trips the pre-flight + guard in `program.ts`, and the API answers + `403 {"error":"This token does not have access to this space"}`. +- **Insufficient scope.** Dropping `stories:write` alone is not enough: `covers_scope?` in + `token_scopeable.rb` treats `publish` as implying `write`, and `write` as implying `read`. Drop + `stories:write` **and** `stories:publish` to get + `403 {"error":"Insufficient scope: stories:write is required"}`. + +Combine this with the browser stub above to run the entire login end to end with no browser at all. + ## Shared asset libraries A shared asset library is a top-level shared asset folder owned by the organization, with per-space @@ -83,6 +120,13 @@ read or write access. `assets pull` and `assets push` reach libraries through `- `--library`. Library assets live under `.storyblok/assets/shared//`, parallel to the space subtree at `.storyblok/assets//`, each with its own `manifest.jsonl`. +Shared libraries are unreachable with an OAuth login. `shared_asset_folders_controller` and +`shared_internal_tags_controller` carry no `require_token_scopes`, so storyrails default-denies any +scoped credential, and `credential.rb` never grants an OAuth grant `user_permission?`. Consent +cannot change this. `assets push` and `assets pull` therefore warn and continue with the space +scope; `--target shared`, `--target all`, and `--library` still fail. Use a personal access token to +test libraries. + ```bash # Pull only the readable libraries (writes .storyblok/assets/shared//). ./dist/index.mjs assets pull --space $STORYBLOK_SPACE_ID --target shared