Validate --port flags via a reusable cli-kit flag#7694
Conversation
61d39a4 to
7a0a93d
Compare
44f701a to
961d06d
Compare
`theme dev --port 13245574` (and other out-of-range, negative,
non-integer, or non-numeric values) crashed with an unhandled RangeError
("options.port should be >= 0 and < 65536"). The raw value reached
server.listen / checkPortAvailability, whose synchronous RangeError
(ERR_SOCKET_BAD_PORT) escaped uncaught because only the asynchronous
'error' event was handled. The same unbounded path existed for app dev's
port flags (Flags.integer with no min/max).
Add a reusable portFlag to @shopify/cli-kit/node/cli built on oclif's
Flags.integer constrained to [1, 65535]; it rejects non-integer and
out-of-range values at parse time and surfaces the valid range in each
flag's --help description. Wire it into every previously-unvalidated
port flag: theme dev --port and app dev's --localhost-port,
--theme-app-extension-port, and --graphiql-port. As defense-in-depth,
checkPortAvailability now catches the synchronous RangeError (resolving
false) while rethrowing any other error, and the theme --port is carried
as a number through DevServerContext rather than a string.
Fixes the crash reported in shop/issues#48221
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
961d06d to
16d24c5
Compare
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 declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/cli.d.ts@@ -39,6 +39,19 @@ export declare const globalFlags: {
export declare const jsonFlag: {
json: import("@oclif/core/interfaces").BooleanFlag<boolean>;
};
+/**
+ * Builds a flag that only accepts a valid port number. The flag parses its
+ * value as an integer and rejects anything that isn't a whole number between 1 and
+ * 65535, so commands fail with a clear message instead of crashing on an out-of-range
+ * port. The accepted range is appended to the description so it shows up in .
+ * @param options - Optional overrides for the flag's description, environment variable, and visibility.
+ * @returns An oclif integer flag constrained to the valid port range.
+ */
+export declare const portFlag: (options?: {
+ description?: string;
+ env?: string;
+ hidden?: boolean;
+}) => import("@oclif/core/interfaces").OptionFlag<number | undefined, import("@oclif/core/interfaces").CustomOptions>;
/**
* Clear the CLI cache, used to store some API responses and handle notifications status
*/
|
There was a problem hiding this comment.
Pull request overview
This PR prevents crashes caused by invalid --port values by centralizing port validation at oclif parse time via a reusable portFlag, and adding a defensive fallback in checkPortAvailability. It also normalizes the theme dev server port to be a number throughout the theme dev context.
Changes:
- Add
portFlagto@shopify/cli-kit/node/cli(integer, constrained to1..65535, range appended to--helptext). - Switch
theme devandapp devport-related flags to useportFlag(and update theme dev internals/tests to treat port as a number). - Harden
checkPortAvailabilityto handle synchronous invalid-port errors by returningfalseinstead of crashing.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli-kit/src/public/node/cli.ts | Introduces reusable portFlag wrapper around Flags.integer with port-range messaging. |
| packages/cli-kit/src/public/node/cli.test.ts | Adds unit coverage for portFlag parsing/rejection behavior. |
| packages/cli-kit/src/public/node/tcp.ts | Adds try/catch to prevent synchronous invalid-port exceptions from escaping. |
| packages/cli-kit/src/public/node/tcp.test.ts | Adds regression tests for out-of-range/invalid ports returning false. |
| packages/theme/src/cli/commands/theme/dev.ts | Switches theme dev --port to portFlag (parse-time validation). |
| packages/theme/src/cli/services/dev.ts | Carries port as a number and uses bounded port availability logic. |
| packages/theme/src/cli/services/dev.test.ts | Adds tests asserting checkPortAvailability receives the numeric port and errors on unavailable ports. |
| packages/theme/src/cli/utilities/theme-environment/types.ts | Updates DevServerContext.options.port type from string to number. |
| packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts | Updates test contexts to use numeric ports. |
| packages/theme/src/cli/utilities/theme-environment/proxy.test.ts | Updates proxy test context to use numeric port. |
| packages/theme/src/cli/utilities/theme-environment/hot-reload/server.test.ts | Updates hot-reload server test context to use numeric port. |
| packages/theme/src/cli/utilities/theme-ext-environment/theme-ext-server.ts | Stops stringifying port when building dev server context for theme extensions. |
| packages/app/src/cli/commands/app/dev.ts | Switches app dev port flags (--localhost-port, --theme-app-extension-port, --graphiql-port) to portFlag. |
| packages/cli/README.md | Updates generated help text to include the validated port range. |
| packages/cli/oclif.manifest.json | Updates manifest flag descriptions to include the validated port range. |
| docs-shopify.dev/generated/generated_docs_data_v2.json | Regenerates docs data to reflect updated flag descriptions. |
| docs-shopify.dev/commands/interfaces/theme-dev.interface.ts | Updates generated theme dev interface docs with port range requirement. |
| docs-shopify.dev/commands/interfaces/app-dev.interface.ts | Updates generated app dev interface docs with port range requirement. |
| .changeset/theme-dev-port-validation.md | Changeset for theme package patch release. |
| .changeset/strong-pumas-port.md | Changeset for cli-kit minor release exposing the new public portFlag. |
| .changeset/app-dev-port-validation.md | Changeset for app package patch release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let port: number | ||
| if (options.port) { | ||
| if (!(await checkPortAvailability(options.port))) { | ||
| throw new AbortError( | ||
| `Port ${options.port} is not available. Try a different port or remove the --port flag to use an available port.`, | ||
| ) | ||
| } | ||
| port = options.port | ||
| } else { | ||
| port = await getAvailableTCPPort(DEFAULT_PORT) | ||
| } |
| export const portFlag = (options: {description?: string; env?: string; hidden?: boolean} = {}) => { | ||
| const description = [options.description, 'Must be between 1 and 65535.'].filter(Boolean).join(' ') | ||
| return Flags.integer({min: 1, max: 65535, ...options, description}) |

WHY are these changes introduced?
Fixes the production crash reported in https://github.com/shop/issues/issues/48221 (Observe error).
Running
theme devwith an invalid--portvalue (out of range, negative, non-integer, or non-numeric) crashed with an unhandled error:The raw
--portvalue was passed tocheckPortAvailability(Number(options.port))and on toserver.listen({port}). Node'sserver.listenvalidates the port and throws aRangeErrorsynchronously (ERR_SOCKET_BAD_PORT), which rejectscheckPortAvailability's promise — it only handles the asynchronous'error'event. Nothing validated the value first, so the error escaped as an unhandled crash. The same unbounded path existed forapp dev's port flags (Flags.integerwith nomin/max).WHAT is this pull request doing?
Validate port flags at the right altitude — once, at parse time, via a single reusable flag — instead of ad-hoc per command:
portFlagin@shopify/cli-kit/node/cli: a thin wrapper over oclif's built-inFlags.integerconstrained to[1, 65535]. Out-of-range or non-integer values are rejected at oclif parse time — a clean, handled validation error that names the offending value (e.g.Expected an integer less than or equal to 65535 but received: 70000), not an unhandled crash, and not reported to Bugsnag. The valid range is also appended to each flag's--helpdescription (… Must be between 1 and 65535.).theme dev --port, andapp dev's--localhost-port,--theme-app-extension-port, and--graphiql-port(previouslyFlags.integerwith no bounds).checkPortAvailabilitynow catches the synchronousRangeErrorand resolvesfalse, while rethrowing any other (unexpected) error — so the primitive can't crash even if reached with a bad value.--portis now carried as anumberthroughDevServerContext(it was astring), matching how it's actually used (server.listen, URL templates).theme consoleis unaffected (it has no--portflag).How to test your changes?
shopify theme dev --store <your-store> --port 13245574→ before: unhandledRangeErrorcrash; after: a clean validation error naming the value (e.g.Expected an integer less than or equal to 65535 but received: 13245574) and a clean exit.--port -1,--port abc,--port 0,--port 92.5,--port 0x10→ all rejected at parse time with a clean error.shopify app dev --localhost-port 70000(likewise--theme-app-extension-port/--graphiql-port) → same parse-time validation instead of an unbounded value reaching the network layer.shopify theme dev --store <your-store> --port 9293(valid, free) → server starts normally;--port <a busy port>→Port X is not available…(behavior unchanged).shopify theme dev --help→ the--portentry now shows… Must be between 1 and 65535.Or run the unit tests:
pnpm vitest run packages/cli-kit/src/public/node/cli.test.ts packages/cli-kit/src/public/node/tcp.test.ts packages/theme/src/cli/services/dev.test.ts.Checklist
@shopify/cli-kitminorfor the new public export;@shopify/themeand@shopify/apppatchfor the bug fix) and added a changeset🤖 Generated with Claude Code