Repository navigation
Handle Trust Battery challenges in theme commands - #8562
graygilmore wants to merge 5 commits into
Conversation
7d6db8a to
945684a
Compare
945684a to
de3151f
Compare
iaineh
changed the title
iaineh
left a comment
iaineh
left a comment
There was a problem hiding this comment.
Notes on the larger decisions in this PR, for reviewers.
| ): Promise<TResult> { | ||
| try { | ||
| const response = await adminRequestDoc(options) | ||
| const response = await withTrustChallengeRetry(() => adminRequestDoc(options)) |
There was a problem hiding this comment.
This is the only api.ts change in this PR. #8897 moves the six theme mutations to requestThemeAdminDoc, so theme reads and mutations all go through this one wrapper.
Today Core challenges only the mutations. It runs the decision for files/file reads only when the query passes sessionId (breadth_resolvers/online_store_theme/files.rb), and getThemeFileChecksums / getThemeFileBodies send none. So fetchChecksums, the theme dev poll, and theme pull are not challenged. If Core starts to challenge them, they get the same handling here. That read case is not tested end to end.
Edited: an earlier version of this comment said that Core challenges reads. That was wrong.
| if (isCI() || !isTTY() || commandEventOutputMode() === 'json') { | ||
| recordEvent('theme-api:trust-challenge:non-interactive') | ||
| throw new AbortError('Shopify needs to verify your identity before it can continue.', undefined, [ | ||
| [ | ||
| 'Open', | ||
| {link: {url: challengeUrl}}, | ||
| "in a browser where you're logged in to Shopify with the same account you use in Shopify CLI, complete the verification, then run the command again.", | ||
| ], | ||
| ]) | ||
| } |
There was a problem hiding this comment.
JSON output mode aborts, the same as CI and no TTY. With --json, -j, or SHOPIFY_FLAG_JSON=1, renderInfo becomes one stderr JSON line and waitForEnter blocks with no visible prompt. The command looks hung. A script can only use an abort with the URL.
--no-input needs no extra check: isTTY() is false then (terminalSupportsPrompting).
Test: "aborts with the challenge URL instead of prompting in JSON output mode".
| renderInfo({ | ||
| headline: 'Verify your identity to continue.', | ||
| body: opened | ||
| ? 'A verification page opened in your browser.' | ||
| : 'Shopify needs to verify your identity before it can continue.', | ||
| link: opened ? undefined : {label: 'Open the verification page', url: challengeUrl}, | ||
| nextSteps: [ | ||
| ['Complete the verification in your browser, logged in with the same account you use in Shopify CLI.'], | ||
| ['Press', {userInput: 'Enter'}, 'here once you have completed it.'], | ||
| ], | ||
| orderedNextSteps: true, | ||
| }) | ||
|
|
||
| await waitForEnter(options.stdin ?? process.stdin) | ||
|
|
||
| recordEvent('theme-api:trust-challenge:acknowledged') | ||
| renderSuccess({headline: 'Verification complete. Continuing.'}) |
There was a problem hiding this comment.
Copy changes:
- "apply this theme change" is now "continue". The text no longer names the operation, so it stays correct for any theme request. Today only mutations get the challenge.
- The text says to use the same account in the browser as in the CLI. Identity stops with
mismatched_identity_accountif the accounts differ.
Edited: an earlier version said the reason was that Core also challenges reads. That was wrong: the CLI reads send no sessionId, so Core does not challenge them.
| * @param error - The error to render. | ||
| */ | ||
| export function renderThrownError(headline: string, error: Error | AbortError) { | ||
| if (error instanceof AbortSilentError) return |
There was a problem hiding this comment.
Ctrl+C at the prompt during theme dev rejects with AbortSilentError. That class extends FatalError, not AbortError, so this function rendered an empty "Error while polling for changes." or "Failed to upload file" banner. The early return covers the poll loop, the initial-sync abort, and the file-save catches. theme dev's own Ctrl+C handler (createKeypressHandler → onClose) does the exit.
Test: "renders nothing for AbortSilentError".
8ab28d3 to
45a041a
Compare
|
|
||
| const TRUST_CHALLENGE_ERROR_CODE = 'CHALLENGE_REQUIRED' | ||
|
|
||
| interface TrustChallengeOptions { |
There was a problem hiding this comment.
nit: TrustChallengeOptions.stdin is only used by one test, and that test mocks waitForEnter anyway. isTTY() also ignores it, so passing a custom stdin wouldn't change the TTY check the way throwInNonTTY({stdin}) does elsewhere. I'd drop the option and let waitForEnter default to process.stdin.
| stdin?: typeof process.stdin | ||
| } | ||
|
|
||
| let pendingChallenge: Promise<void> | undefined |
There was a problem hiding this comment.
nit: a one-line comment on why pendingChallenge can be process-wide would help: only Identity sessions get challenged, and every multi-environment theme command requires --password, so a process that can be challenged only ever has one Identity session.
There was a problem hiding this comment.
One shared prompt per process is safe. Core challenges only Identity sessions, and every multi-environment theme command needs --password. So a process that can get a challenge has only one Identity session.
| const challengeUrl = await trustChallengeUrl(error) | ||
| if (!challengeUrl) throw error | ||
|
|
||
| await completeTrustChallenge(challengeUrl, options) |
There was a problem hiding this comment.
One ordering the concurrency test misses: two batches are in flight, the first gets challenged and the user presses Enter, and the second batch's challenge response only arrives after pendingChallenge is cleared. That opens a second tab and prompt for a request that was sent before the verification. Probably rare, but large upload batches make it more likely. One fix: keep a counter of completed challenges, note it when a request starts, and if a challenge comes back for a request that started before the last completed challenge, retry it once without prompting.
| const challengeUrl = await trustChallengeUrl(error) | ||
| if (!challengeUrl) throw error | ||
|
|
||
| recordEvent('theme-api:trust-challenge:retry-challenged') |
There was a problem hiding this comment.
Not blocking: if the retry is challenged again, we abort partway through a theme push. When the verification just hasn't caught up yet, that leaves a half-uploaded theme. Did you consider showing the prompt again (say up to 3 times) before aborting? Fine as is if a second challenge usually means something is actually wrong.
There was a problem hiding this comment.
A second challenge right after Enter usually means the verification is not complete. One retry and then an abort with the URL keeps the behaviour clear. The abort message tells the user to run the command again.
| ): Promise<TResult> { | ||
| try { | ||
| const response = await adminRequestDoc(options) | ||
| const response = await withTrustChallengeRetry(() => adminRequestDoc(options)) |
There was a problem hiding this comment.
app dev with a theme app extension goes through this too: HostThemeManager.createHostTheme calls themeCreate, and Core runs SessionTrustedToCreateTheme on it. Its retry loop catches everything, so a Ctrl+C at the prompt (AbortSilentError) or the "still needs to verify" abort gets swallowed and retried. That can mean up to four browser tabs and prompts before it gives up, all inside a renderTasks spinner. I think the loop should rethrow AbortError/AbortSilentError (or at least the challenge ones), and the description should drop "App commands are unchanged". Could you also check how the banner renders under the spinner?
Core runs a Trust Battery session decision on every theme mutation. Admin Web can present the resulting identity challenge, but the CLI cannot, so Core currently suppresses challenges for CLI callers. That leaves the CLI as a way around the check, and Core will start surfacing challenges to CLI versions that can handle them. Theme Admin API requests now recognise a CHALLENGE_REQUIRED GraphQL error, open the challenge URL in the browser, wait for the user to press Enter once they have completed it, and retry the request once. A second challenge on the retry aborts with the URL instead of looping. Non-interactive runs (CI or no TTY) abort immediately with the URL and next steps. Concurrent requests, such as the parallel upload batches of `theme push`, share one prompt so the browser opens once and every waiting batch retries after the same verification. A request that starts while a challenge is still pending waits for it first, so it is sent once instead of being challenged and retried too. The prompt accepts Enter or Space only. `theme dev` keeps stdin in raw mode and binds single-letter shortcuts on the same stream; the dedicated wait ignores other keys and leaves raw mode as it found it, so answering the prompt never fires a shortcut. Only challenge URLs on the Admin host are honoured; anything else falls through to normal error handling. The handling is intentionally scoped to the theme API layer; app commands are unchanged.
- Abort instead of prompting in JSON output mode - Use copy that does not name the operation, and say that the browser must use the same account as the CLI - Render nothing for AbortSilentError in theme dev polling and upload errors - Cover @shopify/theme with the challenge changeset
The banner renders before the retry. The retry can still get a challenge. "Continuing." does not say that the verification is complete.
trustChallengeUrl now reads the errors with extractGraphQLErrors instead of its own copy.
45a041a to
3a65a93
Compare
1a3a3cd to
309f40b
Compare
- A request that was already in flight when a challenge was completed retries without a second prompt. A counter of completed challenges is noted when each request starts, so a late challenge response for an earlier request does not open another browser tab. - Remove `TrustChallengeOptions.stdin`. Only one test used it, and `isTTY()` did not read it. `waitForEnter` uses its `process.stdin` default. - The two challenge aborts are a `TrustChallengeError` (an `AbortError`), and the themes API exports `isTrustChallengeError`. The `app dev` host theme retry loop rethrows it and `AbortSilentError`, so Ctrl+C or a failed verification stops theme creation instead of prompting up to four times. Other theme creation errors are still retried.
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationspackages/cli-kit/dist/private/node/themes/trust-challenge.d.tsimport { AbortError } from '../../../public/node/error.js';
/**
* The abort that ends a Trust Battery challenge: the session cannot answer it, or
* the request was challenged again after the verification.
*/
export declare class TrustChallengeError extends AbortError {
}
/**
* Runs a theme Admin API request and, when Core answers with a Trust Battery
* challenge, waits for the user to complete it in the browser before retrying once.
*
* A request that starts while another request's challenge is still being completed
* waits for that challenge first, so it is sent once instead of being challenged
* itself and then retried. A request that was already in flight when a challenge
* was completed retries without a second prompt.
*
* @param request - The request to run and possibly retry.
* @returns The result of the request.
*/
export declare function withTrustChallengeRetry<T>(request: () => Promise<T>): Promise<T>;
/**
* Extracts the challenge URL from a Trust Battery `CHALLENGE_REQUIRED` GraphQL error.
*
* Only URLs on the Admin host are trusted. Anything else is not treated as a
* challenge and the original error surfaces through normal error handling.
*
* @param error - The error thrown by an Admin API request.
* @returns The challenge URL, or undefined when the error is not a trust challenge.
*/
export declare function trustChallengeUrl(error: unknown): Promise<string | undefined>;
packages/cli-kit/dist/private/node/ui/wait-for-enter.d.ts/**
* Waits for Enter (or Space) on stdin and ignores every other key. Ctrl+C rejects
* with AbortSilentError so the process exits the same way it would from any prompt.
*
* Raw mode is left the way it was found. Long-running commands such as `theme dev`
* keep stdin raw for their own single-key shortcuts, and the accepted keys are ones
* those commands do not bind, so pressing Enter here never triggers a shortcut.
*
* Chunks are compared as text rather than bytes: an active Ink renderer (for example
* the `theme push` progress bar) calls `stdin.setEncoding('utf8')`, after which stdin
* emits strings instead of Buffers.
*
* @param stdin - The stream to read from; defaults to process.stdin.
* @returns A promise that resolves when Enter or Space is pressed.
*/
export declare function waitForEnter(stdin?: typeof process.stdin): Promise<void>;
Existing type declarationspackages/cli-kit/dist/private/node/api/graphql.d.ts@@ -1,4 +1,4 @@
-import { Variables } from 'graphql-request';
+import { ClientError, Variables } from 'graphql-request';
export declare function debugLogRequestInfo(api: string, query: string, url: string, variables?: Variables, headers?: Record<string, string>): void;
export declare function sanitizeVariables(variables: Variables): string;
/**
@@ -11,4 +11,5 @@ export declare function sanitizeVariables(variables: Variables): string;
* the full JSON representation if no messages can be extracted.
*/
export declare function extractGraphQLErrorMessages(errors: any[] | undefined): string | undefined;
-export declare function errorHandler(api: string): (error: unknown, requestId?: string) => unknown;
\ No newline at end of file
+export declare function errorHandler(api: string): (error: unknown, requestId?: string) => unknown;
+export declare function extractGraphQLErrors(error: unknown): NonNullable<ClientError['response']['errors']>;
\ No newline at end of file
packages/cli-kit/dist/public/node/themes/api.d.ts@@ -34,6 +34,15 @@ export declare function metafieldDefinitionsByOwnerType(type: MetafieldOwnerType
};
}[]>;
export declare function passwordProtected(session: AdminSession): Promise<boolean>;
+/**
+ * Whether the error ends a Trust Battery challenge on a theme request. A caller that
+ * retries theme requests must rethrow it, or the user is asked to verify again for
+ * each attempt.
+ *
+ * @param error - The error to check.
+ * @returns True when the error ends a Trust Battery challenge.
+ */
+export declare function isTrustChallengeError(error: unknown): boolean;
type OnlineStoreThemeFileBody = {
__typename: 'OnlineStoreThemeFileBodyBase64';
contentBase64: string;
packages/cli-kit/dist/public/node/themes/theme-manager.d.ts@@ -11,12 +11,5 @@ export declare abstract class ThemeManager {
findOrCreate(name?: string, role?: Role): Promise<Theme>;
fetch(name?: string, role?: Role): Promise<Theme | undefined>;
generateThemeName(context: string): string;
- create(themeRole?: Role, themeName?: string): Promise<{
- createdAtRuntime: boolean;
- id: number;
- name: string;
- processing: boolean;
- role: string;
- src?: string;
- }>;
+ create(themeRole?: Role, themeName?: string): Promise<Theme>;
}
\ No newline at end of file
|
This branch has not been deployed
4 participants
Stacked on #8897. That PR routes the theme mutations through
requestThemeAdminDoc, so this PR only wraps that one function.WHY are these changes introduced?
Core runs a Trust Battery session decision on every theme mutation (
themeFilesUpsert,themeFilesDelete,themePublish,themeCreate, ...). It also runs it for theme file reads that pass asessionId, but the CLI's read queries send nosessionId. When the session looks risky the decision returns an identity challenge. Admin Web can show that challenge; the CLI can't, so Core currently suppresses challenges for CLI callers. That leaves the CLI as a way around the check.Core is going to start surfacing challenges to the CLI behind a shop flag (shop/world#2068823), gated on the CLI version from the User-Agent, so this can be dogfooded before release and old CLIs do not receive unsupported challenges. This PR is the CLI half.
WHAT is this pull request doing?
Theme Admin API requests now recognise a
CHALLENGE_REQUIREDGraphQL error and:redirect_tochallenge URL in the browser (or show the link if the browser can't be opened),What it looks like mid-
theme push:then after Enter:
A second challenge on the retry aborts with the URL instead of looping. In CI, without a TTY, with
--no-input, or with--json, the CLI aborts straight away with the URL and next steps.Some details worth knowing:
files/filereads only when the query passessessionId(breadth_resolvers/online_store_theme/files.rb).getThemeFileChecksumsandgetThemeFileBodiessend none, sofetchChecksums, thetheme devpoll, andtheme pullare not challenged. Reads still go throughrequestThemeAdminDoc(theme mutations moved there in Route theme mutations through requestThemeAdminDoc #8897), so a read challenge would get the same handling. That case is not tested end to end.mismatched_identity_account), so the banner and the abort message both say to log in with the same account you use in Shopify CLI.--json(andSHOPIFY_FLAG_JSON=1) turns the banner into a single stderr JSON diagnostic andwaitForEnterwould block with no visible prompt, so JSON output mode now aborts with the URL like CI and non-TTY already did.theme devexits cleanly on Ctrl+C at the prompt. The prompt's Ctrl+C throwsAbortSilentError, which the dev poll loop and upload catches were rendering as an empty "Error while polling for changes." / "Failed to upload file" banner.renderThrownErrornow returns early forAbortSilentError;theme dev's own Ctrl+C handler does the exit.requestThemeAdminDocincli-kit/.../themes/api.ts.app devwith a theme app extension also reaches it:HostThemeManager.createHostThemecallsthemeCreate, and Core runsSessionTrustedToCreateThemeon it. That retry loop caught every error, so Ctrl+C at the prompt or a failed verification started a new attempt, up to four prompts. The challenge aborts are now aTrustChallengeError, the themes API exportsisTrustChallengeError, and the loop rethrows it andAbortSilentError. Other theme creation errors are still retried. Not verified: how the banner renders underapp dev'srenderTasksspinner.theme pushfires upload batches in parallel and they all get challenged together. A shared in-flight promise means the browser opens once, every waiting batch retries after the same Enter, and the success banner renders once. A request that starts while a challenge is pending (e.g. saving another file intheme dev) waits for it first, so it's sent once instead of being challenged and retried too. A batch that was already in flight when the user pressed Enter, and gets its challenge only after that, retries once without a second prompt: each request notes how many challenges were completed when it started.theme devkeeps stdin in raw mode and bindst/p/e/gon the same stream. Instead of cli-kit's any-keykeypress(), a smallwaitForEnterignores other keys and leaves raw mode as it found it, so answering the prompt never fires a shortcut. Ctrl+C still aborts.theme push's progress bar is running, Ink redraws its frame and erases plain lines written underneath it, so the "press Enter" instruction lives inside the info banner rather than a separateoutputInfo. Ink'suseInputalso callsstdin.setEncoding('utf8'), so stdin emits strings instead of Buffers;waitForEnternormalises the chunk to text before matching. I found both of these tophatting, the first version usedoutputInfoand byte comparisons and looked frozen undertheme push.redirect_tomust behttps://onadminFqdn(); anything else falls through as a normal error, same as Admin Web.shptka_/shpat_are unaffected.Analytics:
theme-api:trust-challenge:{required,browser-opened,browser-not-opened,acknowledged,retry-challenged,non-interactive}viarecordEvent.How to manually test your changes?
Local end-to-end, done 2026-10-14. Setup: local Trust Battery, Identity, admin-web, storefront, and Core on shop/world#2068823. Flag on for shop 1. CLI from this branch with
packages/cli-kit/package.jsonset to4.10.0(not committed; rebuild withpnpm nx build cli --skip-nx-cacheafter each version change). A Trust Battery decision override forceschallenge(identity_session_user_verification) onSessionTrustedToUpsertThemeFilesfor the CLI session.https://admin.shop.dev/challenges/user_verification?token=…and the CLI shows the info banner above. Core logs:identity_session_user_verified=success. Override off, Enter: the success banner, then "The theme … was pushed successfully."--json: exit 1, no browser, the abort with the URL as a JSON error document on stdout.CI=1and--no-input: exit 1, the abort banner with the URL.4.9.3: push succeeds. Core logstheme_trust_battery_challenge_suppressedwithcli_version=4.9.3.4.10.0with the flag off: push succeeds, suppressed withcli_version=4.10.0.theme dev, override on: the initial-sync upload (themeFilesUpsert) is challenged and the prompt shows under the preview links after about 4 s.tdoes not answer the prompt (theme dev's owntshortcut still opens the preview). Ctrl+C at the prompt exits with code 0 and no "Error while polling" / "Failed to upload" / "Failed to perform the initial theme synchronization" banner. Not verified: a challenge on a poll or a read. The poll reads send nosessionId, so Core does not challenge them.Out of scope, seen on the base branch too: in
--jsonmode, an upload-phase error prints its JSON document twice, andtheme devrenders the initial-sync error banner twice.Without local Core you can fake the response:
packages/cli-kit/src/public/node/api/admin.ts, temporarily makeadminRequestDocreject once with:pnpm shopify theme push --store <store> --development(Identity login, not--password).admin.shopify.com/challenges/...and the info banner above appears. Presst— nothing happens. Press Enter — the success banner appears and the push completes.CI=1(or--no-input, or--json) and expect an immediate abort listing the challenge URL as a next step.Post-release steps
Core PR shop/world#2068823 pins
MIN_CHALLENGE_CAPABLE_CLI_VERSIONto the first release that contains this PR's merge commit (expected 4.10.0). If this ships under a different version, that constant needs to change before the Core PR merges.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add