Skip to content

Validate --port flags via a reusable cli-kit flag#7694

Open
amcaplan wants to merge 1 commit into
mainfrom
disallow-invalid-port-numbers
Open

Validate --port flags via a reusable cli-kit flag#7694
amcaplan wants to merge 1 commit into
mainfrom
disallow-invalid-port-numbers

Conversation

@amcaplan
Copy link
Copy Markdown
Contributor

@amcaplan amcaplan commented Jun 2, 2026

WHY are these changes introduced?

Fixes the production crash reported in https://github.com/shop/issues/issues/48221 (Observe error).

Running theme dev with an invalid --port value (out of range, negative, non-integer, or non-numeric) crashed with an unhandled error:

Error: options.port should be >= 0 and < 65536. Received type number (13245574).

The raw --port value was passed to checkPortAvailability(Number(options.port)) and on to server.listen({port}). Node's server.listen validates the port and throws a RangeError synchronously (ERR_SOCKET_BAD_PORT), which rejects checkPortAvailability'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 for app dev's port flags (Flags.integer with no min/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:

  • New portFlag in @shopify/cli-kit/node/cli: a thin wrapper over oclif's built-in Flags.integer constrained 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 --help description (… Must be between 1 and 65535.).
  • Wired into all four previously-unvalidated port flags: theme dev --port, and app dev's --localhost-port, --theme-app-extension-port, and --graphiql-port (previously Flags.integer with no bounds).
  • Defense-in-depth in cli-kit: checkPortAvailability now catches the synchronous RangeError and resolves false, while rethrowing any other (unexpected) error — so the primitive can't crash even if reached with a bad value.
  • Type cleanup: the theme --port is now carried as a number through DevServerContext (it was a string), matching how it's actually used (server.listen, URL templates).

theme console is unaffected (it has no --port flag).

How to test your changes?

  1. shopify theme dev --store <your-store> --port 13245574before: unhandled RangeError crash; 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.
  2. Try --port -1, --port abc, --port 0, --port 92.5, --port 0x10 → all rejected at parse time with a clean error.
  3. 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.
  4. shopify theme dev --store <your-store> --port 9293 (valid, free) → server starts normally; --port <a busy port>Port X is not available… (behavior unchanged).
  5. shopify theme dev --help → the --port entry 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

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing — I've identified the correct bump type (@shopify/cli-kit minor for the new public export; @shopify/theme and @shopify/app patch for the bug fix) and added a changeset

🤖 Generated with Claude Code

Copy link
Copy Markdown
Contributor Author

amcaplan commented Jun 2, 2026

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

@github-actions github-actions Bot added the Area: @shopify/theme @shopify/theme package issues label Jun 2, 2026
@amcaplan amcaplan force-pushed the disallow-invalid-port-numbers branch from 61d39a4 to 7a0a93d Compare June 2, 2026 15:40
@amcaplan amcaplan changed the title Validate theme dev --port and fail gracefully on invalid values Validate --port flags via a reusable cli-kit flag Jun 2, 2026
@github-actions github-actions Bot added Area: @shopify/cli @shopify/cli package issues and removed Area: @shopify/theme @shopify/theme package issues labels Jun 2, 2026
@amcaplan amcaplan force-pushed the disallow-invalid-port-numbers branch 2 times, most recently from 44f701a to 961d06d Compare June 2, 2026 18:29
`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>
@amcaplan amcaplan force-pushed the disallow-invalid-port-numbers branch from 961d06d to 16d24c5 Compare June 2, 2026 18:58
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

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/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
  */

@amcaplan amcaplan marked this pull request as ready for review June 2, 2026 21:48
@amcaplan amcaplan requested review from a team as code owners June 2, 2026 21:48
Copilot AI review requested due to automatic review settings June 2, 2026 21:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 portFlag to @shopify/cli-kit/node/cli (integer, constrained to 1..65535, range appended to --help text).
  • Switch theme dev and app dev port-related flags to use portFlag (and update theme dev internals/tests to treat port as a number).
  • Harden checkPortAvailability to handle synchronous invalid-port errors by returning false instead 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.

Comment on lines +96 to 106
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)
}
Comment on lines +164 to +166
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})
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: @shopify/cli @shopify/cli package issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants