Skip to content

[Tests] Cover the listing name containment guard - #8941

Draft
github-actions[bot] wants to merge 1 commit into
mainfrom
tests-maintenance-39180710266
Draft

github-actions[bot] wants to merge 1 commit into
mainfrom
tests-maintenance-39180710266

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

validateListingName in packages/theme/src/cli/utilities/theme-file-path.ts guards the user-supplied --listing flag on theme dev, theme push and theme share, and has no co-located test. It was added by #8834 ("Keep theme files in the selected directory") to stop a listing name from escaping the theme directory, so its rejection branches are the containment contract rather than incidental validation.

Of the five rejection branches, only one is exercised today, and only indirectly: theme-listing.test.ts:205 passes '../outside' through ensureListingExists, which happens to hit the ..-separator branch. Nothing asserts that an empty name, a bare ., a bare .., a Windows \ separator, or a drive-letter prefix is rejected, and nothing pins down which names stay accepted. A refactor of that predicate could drop the Windows separator or the drive-letter check and every test in the repository would still pass.

This is not a flaky-test fix. Reviewing tests-main.yml runs created between 2026-10-12T00:26Z and 2026-10-19T00:26Z UTC (36 runs across main and stable/4.9, all first attempts, no reruns) found four failures, all on windows-latest, all timeouts, and all reducing to two signatures: the PowerShell record test in app-security-commands.test.ts (runs 38773610537, 38766274961, 38709303616, 38383005193) and the 32-hex secret test in secret-safety.test.ts (run 38766274961). Both already have open fixes, #8832 and #8928 respectively, so there is no actionable flake left in the window.

WHAT is this pull request doing?

Adds packages/theme/src/cli/utilities/theme-file-path.test.ts with two table-driven cases over validateListingName: nine names it must reject, and four it must accept.

The accepted names are the ones that make the predicate's boundary explicit — ..periods and ... start with two periods but are ordinary directory names, so the function compares against . and .. exactly rather than matching a prefix. Without those cases a stricter rewrite that rejected anything starting with .. would look correct.

The function is pure and synchronous, so the test needs no temporary directory and no filesystem access. The names are passed directly rather than driven through ensureListingExists, which keeps each case a statement about the guard instead of about the caller, and avoids the real git-and-scan setup that makes the existing listing tests slow. No production code is changed.

Validation: the new file passes locally on Linux Node 26.1.0 (13 passed), five times under --sequence.shuffle, and alongside all 46 test files in packages/theme/src/cli/utilities (621 passed). nx type-check theme and ESLint on the new file both pass. I confirmed the tests earn their keep by temporarily neutralizing the predicate: all nine rejection cases fail and the four acceptance cases still pass, then restored it.

Remaining uncertainty: the drive-letter and backslash cases describe Windows-shaped input but the predicate is plain string matching with no platform branch, so they assert the same thing on every OS — I verified them on Linux only, which is sufficient for this function but does not exercise real Windows path handling.

How to manually test your changes?

shopify theme dev --listing ../outside
shopify theme push --listing 'C:\outside'

Both should fail with Unsafe listing name.

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 (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

This branch has not been deployed

No deployments
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.

0 participants