Skip to content

Replace store auth with PKCE and add token refresh#7136

Closed
RyanDJLee wants to merge 2 commits intographite-base/7136from
ryandjlee/feat/store-auth-pkce
Closed

Replace store auth with PKCE and add token refresh#7136
RyanDJLee wants to merge 2 commits intographite-base/7136from
ryandjlee/feat/store-auth-pkce

Conversation

@RyanDJLee
Copy link
Copy Markdown
Contributor

@RyanDJLee RyanDJLee commented Mar 31, 2026

Summary

  • Replace confidential client OAuth flow with PKCE (RFC 7636) for shopify store auth — no client_secret needed
  • Add headless token refresh to shopify store execute — auto-refreshes expired tokens before API calls
  • Bind loopback server to 127.0.0.1 (not localhost) per RFC 8252 §8.3
  • Add outputDebug logging throughout auth, refresh, and session loading (visible with --verbose)

What changed

config.ts — New 1P app client ID (7e9cb568cfd431c538f36d1ad3f2b4f6), port 13387, 127.0.0.1 binding, maskToken() helper

auth.ts — Full PKCE rewrite: code_challenge/code_verifier replace client_secret/HMAC. Constant-time state comparison. HTML escaping in error callback. response_type=code per RFC 6749. Preserves upstream StoreAuthBootstrap DI pattern.

session.ts — Extended schema: refreshToken, expiresAt, refreshTokenExpiresAt, associatedUser. Added isSessionExpired() with 4-minute margin.

execute.ts — Added refreshStoreToken() for headless refresh via grant_type=refresh_token. loadAuthenticatedStoreSession is now async, checks expiry before API calls. Clears session on 401.

commands/store/auth.ts — Removed --client-secret-file flag. Updated description to reference PKCE.

Security properties

  • code_challenge + code_challenge_method=S256 + response_type=code in authorize URL
  • code_verifier + redirect_uri in token exchange (no client_secret)
  • Constant-time state comparison (timingSafeEqual)
  • HTML escaped in error callback (XSS prevention on loopback)
  • Refresh response validated for access_token presence before persisting
  • All tokens masked in --verbose output (first 10 chars + ***)
  • 401 during execute clears stored session

Test plan

  • 41/41 unit tests passing (5 test files)
  • Type check clean
  • Lint clean
  • E2E: Auth against production (j1wdgc-40.myshopify.com) — PKCE flow, token + refresh token stored
  • E2E: Execute against production — { shop { name: "My Store", id: "gid://shopify/Shop/84355743800" } }
  • E2E: Refresh against production (simulated expiry) — token rotated, query succeeded with new token
  • E2E: Second execute after refresh — no re-refresh, cached token used directly

🤖 Generated with Claude Code

Replace the confidential client OAuth flow with PKCE (RFC 7636) for
`shopify store auth`. The CLI no longer requires a client_secret file —
authentication uses code_challenge/code_verifier instead.

Key changes:
- PKCE auth: code_challenge + code_challenge_method=S256 in authorize URL,
  code_verifier + redirect_uri in token exchange (no client_secret)
- Loopback server binds to 127.0.0.1 (not localhost) per RFC 8252 §8.3
- Constant-time state comparison via timingSafeEqual
- HTML escaping in error callback responses (XSS prevention)
- response_type=code in authorize URL per RFC 6749 §4.1.1
- Token refresh: execute auto-refreshes expired tokens headlessly via
  grant_type=refresh_token before API calls (4-min expiry margin)
- Session schema extended with refreshToken, expiresAt, associatedUser
- Production debug logging via outputDebug (visible with --verbose)
- Tokens masked in debug output (first 10 chars + ***)

Verified E2E against production Core (j1wdgc-40.myshopify.com):
auth → execute → refresh → execute (with refreshed token)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 31, 2026

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 82.3% 15381/18688
🟡 Branches 74.64% 7548/10112
🟢 Functions 81.47% 3855/4732
🟢 Lines 82.69% 14542/17587

Test suite run success

4041 tests passing in 1542 suites.

Report generated by 🧪jest coverage report action from a9c24d1

Switch from test-connector-app to the registered 1P app
(7e9cb568cfd431c538f36d1ad3f2b4f6) with f_admin_oauth_pkce_enabled.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

Differences in type declarations

We 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:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/fs.d.ts
@@ -1,6 +1,6 @@
 import { OverloadParameters } from '../../private/common/ts/overloaded-parameters.js';
 import { RandomNameFamily } from '../common/string.js';
-import { findUp as internalFindUp, findUpSync as internalFindUpSync } from 'find-up';
+import { findUp as internalFindUp } from 'find-up';
 import { ReadStream, WriteStream } from 'fs';
 import type { Pattern, Options as GlobOptions } from 'fast-glob';
 /**
@@ -335,14 +335,6 @@ export declare function defaultEOL(): EOL;
  * @returns The first path found that matches or  if none could be found.
  */
 export declare function findPathUp(matcher: OverloadParameters<typeof internalFindUp>[0], options: OverloadParameters<typeof internalFindUp>[1]): ReturnType<typeof internalFindUp>;
-/**
- * Find a file by walking parent directories.
- *
- * @param matcher - A pattern or an array of patterns to match a file name.
- * @param options - Options for the search.
- * @returns The first path found that matches or  if none could be found.
- */
-export declare function findPathUpSync(matcher: OverloadParameters<typeof internalFindUp>[0], options: OverloadParameters<typeof internalFindUp>[1]): ReturnType<typeof internalFindUpSync>;
 export interface MatchGlobOptions {
     matchBase: boolean;
     noglobstar: boolean;
packages/cli-kit/dist/public/node/is-global.d.ts
@@ -26,14 +26,6 @@ export declare function installGlobalCLIPrompt(): Promise<InstallGlobalCLIPrompt
  * Infers the package manager used by the global CLI.
  *
  * @param argv - The arguments passed to the process.
- * @param env - The environment variables of the process.
  * @returns The package manager used by the global CLI.
  */
-export declare function inferPackageManagerForGlobalCLI(argv?: string[], env?: NodeJS.ProcessEnv): PackageManager;
-/**
- * Returns the project directory for the given path.
- *
- * @param directory - The path to search upward from.
- * @returns The project root directory, or undefined if not found.
- */
-export declare function getProjectDir(directory: string): string | undefined;
\ No newline at end of file
+export declare function inferPackageManagerForGlobalCLI(argv?: string[]): PackageManager;
\ No newline at end of file
packages/cli-kit/dist/public/node/node-package-manager.d.ts
@@ -25,7 +25,7 @@ export type DependencyType = 'dev' | 'prod' | 'peer';
 /**
  * A union that represents the package managers available.
  */
-export declare const packageManager: readonly ["yarn", "npm", "pnpm", "bun", "homebrew", "unknown"];
+export declare const packageManager: readonly ["yarn", "npm", "pnpm", "bun", "unknown"];
 export type PackageManager = (typeof packageManager)[number];
 /**
  * Returns an abort error that's thrown when the package manager can't be determined.
packages/cli-kit/dist/public/node/upgrade.d.ts
@@ -4,38 +4,11 @@
  *
  * @returns A string with the command to run.
  */
-export declare function cliInstallCommand(): string | undefined;
-/**
- * Runs the CLI upgrade using the appropriate package manager.
- * Determines the install command and executes it.
- *
- * @throws AbortError if the package manager or command cannot be determined.
- */
-export declare function runCLIUpgrade(): Promise<void>;
-/**
- * Returns the version to auto-upgrade to, or undefined if auto-upgrade should be skipped.
- * Auto-upgrade is disabled by default and must be enabled via .
- * Also skips for CI, pre-release versions, or when no newer version is available.
- *
- * @returns The version string to upgrade to, or undefined if no upgrade should happen.
- */
-export declare function versionToAutoUpgrade(): string | undefined;
-/**
- * Shows a daily upgrade-available warning for users who have not enabled auto-upgrade.
- * Skipped in CI and for pre-release versions. When auto-upgrade is enabled this is a no-op
- * because the postrun hook will handle the upgrade directly.
- */
-export declare function warnIfUpgradeAvailable(): Promise<void>;
+export declare function cliInstallCommand(): string;
 /**
  * Generates a message to remind the user to update the CLI.
  *
  * @param version - The version to update to.
  * @returns The message to remind the user to update the CLI.
  */
-export declare function getOutputUpdateCLIReminder(version: string): string;
-/**
- * Prompts the user to enable or disable automatic upgrades, then persists their choice.
- *
- * @returns Whether the user chose to enable auto-upgrade.
- */
-export declare function promptAutoUpgrade(): Promise<boolean>;
\ No newline at end of file
+export declare function getOutputUpdateCLIReminder(version: string): string;
\ No newline at end of file
packages/cli-kit/dist/public/node/version.d.ts
@@ -18,13 +18,4 @@ export declare function globalCLIVersion(): Promise<string | undefined>;
  * @param version - The version to check.
  * @returns True if the version is a pre-release version.
  */
-export declare function isPreReleaseVersion(version: string): boolean;
-/**
- * Checks if there is a major version change between two versions.
- * Pre-release versions (0.0.0-*) are treated as not having a major version change.
- *
- * @param currentVersion - The current version.
- * @param newerVersion - The newer version to compare against.
- * @returns True if there is a major version change.
- */
-export declare function isMajorVersionChange(currentVersion: string, newerVersion: string): boolean;
\ No newline at end of file
+export declare function isPreReleaseVersion(version: string): boolean;
\ No newline at end of file
packages/cli-kit/dist/private/node/conf-store.d.ts
@@ -24,7 +24,6 @@ export interface ConfSchema {
     devSessionStore?: string;
     currentDevSessionId?: string;
     cache?: Cache;
-    autoUpgradeEnabled?: boolean;
 }
 /**
  * Get session.
@@ -126,18 +125,6 @@ interface RunWithRateLimitOptions {
  * @returns true, or undefined if the task was not run.
  */
 export declare function runWithRateLimit(options: RunWithRateLimitOptions, config?: LocalStorage<ConfSchema>): Promise<boolean>;
-/**
- * Get auto-upgrade preference.
- *
- * @returns Whether auto-upgrade is enabled, or undefined if never set.
- */
-export declare function getAutoUpgradeEnabled(config?: LocalStorage<ConfSchema>): boolean | undefined;
-/**
- * Set auto-upgrade preference.
- *
- * @param enabled - Whether auto-upgrade should be enabled.
- */
-export declare function setAutoUpgradeEnabled(enabled: boolean, config?: LocalStorage<ConfSchema>): void;
 export declare function getConfigStoreForPartnerStatus(): LocalStorage<Record<string, {
     status: true;
     checkedAt: string;
packages/cli-kit/dist/public/node/hooks/postrun.d.ts
@@ -5,11 +5,4 @@ import { Hook } from '@oclif/core';
  * @returns Whether post run hook has completed.
  */
 export declare function postRunHookHasCompleted(): boolean;
-export declare const hook: Hook.Postrun;
-/**
- * Auto-upgrades the CLI after a command completes, if a newer version is available.
- * The entire flow is rate-limited to once per day unless forced via SHOPIFY_CLI_FORCE_AUTO_UPGRADE.
- *
- * @returns Resolves when the upgrade attempt (or fallback warning) is complete.
- */
-export declare function autoUpgradeIfNeeded(): Promise<void>;
\ No newline at end of file
+export declare const hook: Hook.Postrun;
\ No newline at end of file
packages/cli-kit/dist/public/node/hooks/prerun.d.ts
@@ -11,7 +11,6 @@ export declare function parseCommandContent(cmdInfo: {
     pluginAlias?: string;
 }): CommandContent;
 /**
- * Triggers a background check for a newer CLI version (non-blocking).
- * The result is cached and consumed by the postrun hook for auto-upgrade.
+ * Warns the user if there is a new version of the CLI available
  */
-export declare function checkForNewVersionInBackground(): void;
\ No newline at end of file
+export declare function warnOnAvailableUpgrade(): Promise<void>;
\ No newline at end of file
packages/cli-kit/dist/private/node/api/graphql.d.ts
@@ -1,14 +1,4 @@
 import { 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;
-/**
- * Extracts human-readable error messages from a GraphQL errors array.
- *
- * Some APIs (e.g. App Management) return structured errors nested inside
- * . When those are present, we extract
- * them so the CLI displays a clean message instead of a raw JSON dump.
- * Falls back to each error's top-level  field, and ultimately to
- * 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

@dmerand dmerand changed the base branch from store-execute to graphite-base/7136 March 31, 2026 21:00
@dmerand dmerand closed this Mar 31, 2026
Copy link
Copy Markdown
Contributor

dmerand commented Mar 31, 2026

Closed because it's folded into the downstack PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants