Redact store signup JWTs from analytics payloads - #8444
Open
craigmichaelmartin wants to merge 1 commit into
Open
Redact store signup JWTs from analytics payloads#8444craigmichaelmartin wants to merge 1 commit into
craigmichaelmartin wants to merge 1 commit into
Conversation
`store stripe-auth` takes a signup JWT that is a bearer credential for the target store. Command arguments are reported to Monorail verbatim in `sensitive.args`, and `sanitizePayload` only knew about Theme Access passwords, so every invocation sent the credential off the machine — including the happy path where the browser opens and nothing is printed. Redact the signup credential in the three shapes the payload can carry it: as a command-line flag value, as a JSON key, and as a URL query parameter. The bare-value alternative now treats a JSON escape as part of the value. Applied to the existing store-password rule this also fixes a latent failure where a quoted value produced a string that no longer parsed, which threw inside `sanitizePayload` and dropped the whole event. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Assisted-By: devx/39092f35-5041-4a88-ab8d-808c98322495
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Related to the internal security finding tracked as Vault 69736 (
cli / oauth-client).shopify store stripe-authaccepts a signup JWT via--signup. That JWT is a bearer credential for the target store, and the Stripe Projects setup guide that drives this command classifies it as a server secret that must never be printed.Command arguments are reported to Monorail verbatim as
sensitive.args(prerun.ts→startAnalytics({args})→args: startArgs.join(' ')), andsanitizePayloadonly knew about Theme Access passwords and--store-password. So the credential left the machine on every invocation that passes the flag — not only on the fallback path where the URL is printed.Reproduced before the change with a fake token and analytics delivery disabled:
How this relates to the other two PRs. This is complementary, not a rival fix — there is no file overlap.
openURLargv, and the CLI's own argv for stdin callersBoth of those are confined to
packages/store; this sink is inpackages/cli-kit, so it survives either of them landing. Any merge order works. This is a sink-side backstop and does not close the finding on its own — the credential still reaches the browser's authorization URL, and it still sits in the invoking shell's history and this process's own argv while--signupremains a documented flag.WHAT is this pull request doing?
Adds three rules to
sanitizePayloadcovering the shapes the payload can carry the credential in, mirroring the three that already exist forstore-password:--signup <jwt>and--signup=<jwt>"signup": "<jwt>", as reached viacmd_all_environment_flagssignup=<jwt>inside a URL inerror_messageormetadataTwo deliberate choices worth flagging for review:
The bare-value alternative changed from
[^\s"]+to(?:\\.|[^\s"\\])+. Redaction runs on the serialized payload, so a value quoted on the command line arrives with its quotes escaped. The old class stops at neither, which had two bad outcomes I verified against the current rule:--store-passwordrule--store-password "secret"JSON.parsethrows insidesanitizePayload→ whole event dropped--store-password 'a"b'Simply excluding
\\would stop the throw but leak the value instead. Treating a JSON escape as part of the value handles both, so I applied it to the existingstore-passwordrule too rather than leaving one correct rule next to a broken one. That is the only behaviour change outsidesignup, and it is covered by its own test.The URL rule has a
(?<![\w-])guard. Without it, an unrelatedfrom_signup=truewould be masked. With it,https://partners.shopify.com/signupandfrom_signup=are untouched — both asserted in tests.Known limitation, stated so the next reader doesn't over-trust it: this redaction is coupled to the parameter name. If core renames
signup, the rules go stale with no test failure. That is inherent to sink-side redaction and is why the source-side fix in #8428 still matters — they are complements, not substitutes.How to test your changes?
5 of the 6 new tests fail without the
analytics.tschange; the sixth is the over-redaction guard, which passes both before and after by design.End-to-end, with delivery disabled so nothing is transmitted:
Expect
--signup *****, and zero occurrences of the token anywhere in the payload. Repeat with--signup=<jwt>to cover the other spelling.Also run locally:
eslintandprettier --checkon both files clean,nx run cli-kit:type-checkclean,pnpm knipclean, and the fullpackages/cli-kitsuite at 1869 passed. The 2 failures inhooks/deprecations.test.tsreproduce on a cleanmainand are unrelated.Post-release steps
None.
Checklist
sensitive.args,cmd_all_environment_flags,error_messageandmetadatanow masksignupvalues. No field is added or removed.AGENTS.mdscopes changesets to user-visible behaviour. Same call as 37e3ad3 "Prevent GitHub credentials from being logged", which shipped five files with no changeset.