Repository navigation
Handle Trust Battery challenges in theme commands - #8562
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.
There was a problem hiding this comment.
Done in db92f0f: I removed the stdin option. The prompt now always reads 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.
There was a problem hiding this comment.
Done in db92f0f. Each request notes how many challenges were completed when it started. If its challenge arrives after a verification that completed while it was in flight, it retries once without a second tab or prompt. 260c865 adds one more case: if another prompt is open at that moment, the request waits for that prompt first. Both orderings have tests in trust-challenge.test.ts.
| 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?
There was a problem hiding this comment.
Done in db92f0f. The challenge aborts are now a TrustChallengeError, and the themes API exports isTrustChallengeError. HostThemeManager's retry loop rethrows it and AbortSilentError, so Ctrl+C or a failed verification stops after one prompt; other theme creation errors are still retried. The PR description no longer says "App commands are unchanged". I have not checked how the banner renders under the renderTasks spinner; the description says so.
45a041a to
3a65a93
Compare
1a3a3cd to
309f40b
Compare
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.
- 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.
A request that was in flight during an earlier verification retried at once when its challenge arrived, even if a newer request had opened another prompt in the meantime. That retry was challenged again and aborted while the prompt was still waiting for Enter. Such a request now waits for the open prompt first.
055a626 to
18a57f0
Compare
8c9b560 to
260c865
Compare
| try { | ||
| return await request() | ||
| } catch (error) { | ||
| const challengeUrl = await trustChallengeUrl(error) | ||
| if (!challengeUrl) throw error |
There was a problem hiding this comment.
this is the same as lines 39-43, should it be extracted to a reusable function?
There was a problem hiding this comment.
Done in 6f43f01. Both attempts now go through one helper, attemptRequest. It returns the result or the challenge URL, and throws any other error. withTrustChallengeRetry only decides what to do after each attempt: prompt once, then retry, then abort.
| * @param stdin - The stream to read from; defaults to process.stdin. | ||
| * @returns A promise that resolves when Enter or Space is pressed. | ||
| */ | ||
| export function waitForEnter(stdin: typeof process.stdin = process.stdin): Promise<void> { |
There was a problem hiding this comment.
Could we extend the existing keypress() helper to support accepted keys and preserve the previous raw mode? This also looks like a useful place to handle string chunks from Ink, so both callers use the same input handling.
There was a problem hiding this comment.
Done in 6f43f01. keypress() in public/node/ui.tsx now:
- accepts a
keysoption: only those keys end the wait, and other keys are ignored. Without the option, any key ends it, as before. - sets raw mode back to its earlier state. Before, it always turned raw mode off, which would break
theme dev's shortcuts. - reads each chunk as text, so it also detects Ctrl+C when Ink has set the stdin encoding. This fixes the same problem for
mkcert, the other caller.
The Trust Battery prompt calls keypress(process.stdin, undefined, {keys: ['\r', '\n', ' ']}), and waitForEnter is removed. Its tests moved to the keypress tests in ui.test.ts, and a changeset covers the new option. I also checked it in a real terminal: with stdin already raw and UTF-8 encoded, t and p are ignored, Enter resolves, and raw mode stays on.
- withTrustChallengeRetry runs both attempts through one helper, attemptRequest. It returns the result or the challenge URL and throws any other error. - keypress accepts a keys option. Only those keys end the wait. It sets raw mode back to its earlier state and reads chunks as text, so it also detects Ctrl+C when Ink has set the stdin encoding. The Trust Battery prompt calls keypress with Enter and Space, and the waitForEnter helper is removed.
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, unless another prompt is open:
* then it waits for that prompt too.
*
* @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>;
Existing type declarationspackages/cli-kit/dist/public/node/ui.d.ts@@ -375,10 +375,30 @@ export interface RenderDangerousConfirmationPromptOptions extends Omit<Dangerous
*
*/
export declare function renderDangerousConfirmationPrompt({ renderOptions, ...props }: RenderDangerousConfirmationPromptOptions, uiDebugOptions?: UIDebugOptions): Promise<boolean>;
-/** Waits for any key to be pressed except Ctrl+C which will terminate the process. */
+interface KeypressOptions {
+ /**
+ * The keys that end the wait, for example `['\r']` for Enter. Other keys are ignored.
+ * Without this option, any key ends the wait.
+ */
+ keys?: string[];
+}
+/**
+ * Waits for a key to be pressed. Ctrl+C rejects with AbortSilentError, which terminates the process.
+ *
+ * Raw mode is set back to the state it had before the call. A command that keeps stdin raw for
+ * its own single-key shortcuts (for example `theme dev`) keeps them.
+ *
+ * Chunks are read as text: an active Ink renderer calls `stdin.setEncoding('utf8')`, and then
+ * stdin emits strings instead of Buffers.
+ *
+ * @param stdin - The stream to read from.
+ * @param uiDebugOptions - Debug options; `skipTTYCheck` allows a non-TTY stdin.
+ * @param options - The keys that end the wait.
+ * @returns A promise that resolves when an accepted key is pressed.
+ */
export declare const keypress: (stdin?: NodeJS.ReadStream & {
fd: 0;
-}, uiDebugOptions?: UIDebugOptions) => Promise<unknown>;
+}, uiDebugOptions?: UIDebugOptions, { keys }?: KeypressOptions) => Promise<void>;
interface IsTTYOptions {
stdin?: NodeJS.ReadStream;
uiDebugOptions?: UIDebugOptions;
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;
|
Builds on #8897 (merged). 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, and the Enter prompt would block with no visible text, 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. cli-kit'skeypress()now accepts akeysoption and sets raw mode back to its earlier state; the prompt calls it with Enter and Space, so other keys never answer it. Ctrl+C still aborts. (Before,keypress()always turned raw mode off when it finished, which would have brokentheme dev's shortcuts.)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;keypress()now reads each chunk as text before it matches keys or Ctrl+C. This also fixes Ctrl+C detection forkeypress()'s other caller (mkcert). 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