Repository navigation
Validate --client-id in App Security check, record and review - #8764
Conversation
github-actions
Bot
added
the
no-changelog
f1ca397 to
c44a3ca
Compare
| Use \`--exclude\` to skip more paths. Each value is a glob that is matched against the path relative to the working directory, so a path above it starts with \`../\`, and a name at any depth needs \`**/\`, for example \`--exclude '**/generated'\`. Repeat the flag to add globs. An exclusion can't remove the selected app configuration file. Quote each value so your shell doesn't expand \`*\`. The coding-agent instructions this check offers repeat the globs. Other \`app security\` commands don't take \`--exclude\` or \`--no-git-ignore\`, so pass the same flags each time you run the check. | ||
|
|
||
| Use \`--list-files\` to check the scope before scanning: it prints the files the check would gather, one path per line and relative to the app directory (\`{"files": [...]}\` with \`--json\`), and then stops. It writes no results and never prompts. \`--client-id\` is accepted but has no effect on the list. | ||
| Use \`--list-files\` to check the scope before scanning: it prints the files the check would gather, one path per line and relative to the app directory (\`{"files": [...]}\` with \`--json\`), and then stops. It writes no results and never prompts. \`--client-id\` is still checked, but doesn't change the list. |
There was a problem hiding this comment.
Non-blocking suggestion: qualify never prompts now that --client-id can start login.
The compiled-CLI smoke entered device login and requested a browser launch with --list-files and --json, including redirected execution outside CI and a synthetic expired session. --no-input and CI correctly prevented the browser request and polling. The login requirement is intentional, and the CLI treats JSON output and no-input as separate controls.
Could the help say that these modes skip App Security's selection/instructions prompts, but authentication may still require user action, and show --no-input for automation? The later JSON output never prompts sentence needs the same qualification. If the stronger no-prompt promise is intended, it needs to reach authentication too.
The smoke kept the real resolver, lookup and auth code. Transport responses were synthetic and the browser launch was blocked; no live account or completed login was tested.
| }), | ||
| pickClientId: async (appDirectory) => (await fetchOrCreateOrganizationApp(appCreationDefaults(appDirectory))).apiKey, | ||
| lookUpApp: async (clientId) => { | ||
| await appFromIdentifiers({apiKey: clientId}) |
There was a problem hiding this comment.
Non-blocking suggestion: omit the deploy-only --reset advice for App Security callers.
The shared missing-app error says Pass --reset to your command to create a new app. In the compiled-CLI smoke, check, record and review all displayed that step, but replaying it on each command failed with Nonexistent flag: --reset (exit 2). Saved findings stayed unchanged.
Could we keep the account/permission guidance but omit or replace the reset step for these callers, while preserving it for commands that support it? Please add coverage for the real error's recovery text, not only a mocked one-line AbortError.
This help smoke replaced only the leaf platform-client lookup/account methods. The shared error formatter and command parsers were real; real missing-app API responses were not tested.
There was a problem hiding this comment.
Fixed in 5da30fe. appFromIdentifiers now takes offerReset (default true, so other callers are unchanged), and the app security lookup passes false: the permission and shopify auth login steps stay, the --reset step is gone. Covered against the real error text: context.test.ts builds the error with offerReset: false, and app-security-selection.test.ts runs the real appFromIdentifiers behind the default lookup with a client that finds no app, and asserts the next steps mention shopify auth login and not --reset.
Separately: app init and app config link also call appFromIdentifiers and keep the --reset step. I haven't checked whether they take --reset, so I left them alone here.
Look up the --client-id value in the user's account, as deploy does, before gathering, scanning, writing or reading results, and before the no-app-configuration prompt. The TOML's client_id and the picker's client ID aren't looked up. clean and instructions don't opt in, so results saved under a mistyped client ID can still be removed.
--client-id= passed an empty string, which the truthiness check skipped, so check --list-files listed files without the lookup. The value was passed, so it's looked up like any other.
The shared missing-app error suggests passing --reset, which the app security commands don't take. appFromIdentifiers takes offerReset, true by default, and the app security lookup passes false; the account and login steps stay.
e441552 to
764ecc1
Compare
4 participants
WHY are these changes introduced?
shopify app security check,recordandreviewaccept any--client-idvalue. A mistyped ID isn't caught:checkwrites results under it,recordrecords findings under it, andreviewreports on results that don't belong to any app.app deploycatches the same mistake by looking the ID up first.WHAT is this pull request doing?
check(including--list-files),recordandreviewlook up the--client-idvalue in the user's account, usingappFromIdentifiers, as deploy does. An unknown ID aborts with deploy's account-aware message before anything is gathered, scanned, written or read, and before the "scan without app configuration?" prompt.--client-idvalue is checked. The TOML's ownclient_idisn't, and a client ID picked from the app list already comes from the API.cleanandinstructionsdon't check it, so results saved under a mistyped ID can still be removed. The commands share one selection resolver, so the lookup is an explicit opt-in (validateClientIdFlag) and the lookup itself is an injected dependency (lookUpApp). Tests don't need the network or a login.--pathdirectory, the app configuration, and--without-app-configrequiring--client-id.--client-idnow requires a login. The help for the three commands says so, and--list-filesnow says the ID is still checked.After
Invalid ids are rejected
Valid ids are accepted
How to manually test your changes?
From a logged-in CLI, with
APPan app directory,GOODa client ID from your account andBADa made-up one:pnpm shopify app security check --path $APP --client-id $BAD --skip-instructionsfails with "No app with client ID … found", and$APP/.shopify/app-security/has no$BADdirectory. Repeat with--list-filesinstead of--skip-instructions, then withreview, and withecho '{}' | pnpm shopify app security record: each fails the same way.--client-id $GOODproceed. With{}on stdin,recordfails on document validation instead.pnpm shopify app security check --path $APP --skip-instructions, without--client-id, makes no lookup.mkdir -p $APP/.shopify/app-security/$BAD && pnpm shopify app security clean --path $APP --client-id $BADremoves the directory without a lookup.pnpm shopify app security check --path <directory with no TOML> --client-id $BADfails without asking whether to scan without app configuration first.Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add