refactor(maker-appx)!: reimplement as a legacy wrapper over the MSIX maker - #4392
claude[bot] wants to merge 17 commits into
Conversation
…maker `@electron-forge/maker-appx` no longer depends on `electron-windows-store` (and its Desktop App Converter / MakeCert tooling). It now maps `MakerAppXConfig` onto `electron-windows-msix` options and packages through the same code path as `@electron-forge/maker-msix`, so existing Forge configs keep type-checking and building. The class and the ignored config options are marked `@deprecated` in favour of `@electron-forge/maker-msix`. `toMsixArch` is exported from `@electron-forge/maker-msix` so the ia32 -> x86 mapping is shared. `electron-windows-store` and its ambient typings are removed from the repo, and the slow make spec no longer generates a MakeCert dev certificate on Windows. BREAKING CHANGE: `@electron-forge/maker-appx` now produces a `.msix` file (under `make/appx/<arch>/`) instead of `.appx`, and depends on `electron-windows-msix` instead of `electron-windows-store`. The `containerVirtualization`, `createConfigParams`, `createPriParams`, `deploy`, `desktopConverter`, `expandedBaseImage`, `flatten`, `finalSay` and `makeappxParams` options are ignored with a warning. The maker no longer creates a development certificate next to the output when `devCert` is unset (signing falls through to `electron-windows-msix`), the `createDefaultCertificate` export is gone, and a missing publisher (no `publisher` option and no `author.name` in package.json) is now an error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked a couple of other candidate issues and ruled them out: finalSay is now handled the same as the other listed UNSUPPORTED_OPTIONS (warned and ignored, matching the PR's stated intent, not silently dropped without warning), and toMsixArch throwing for architectures outside x64/arm64/ia32 mirrors the same validation already present in the sibling maker-msix package rather than being a new regression unique to this PR.
Extended reasoning...
This change reworks @ electron-forge/maker-appx into a thin compatibility shim over electron-windows-msix, sharing toMsixArch with maker-msix, and removes the old electron-windows-store/cross-spawn/SDK-discovery implementation along with its dependencies, typings, and test fixture. Three confirmed findings are already queued as inline comments: a catastrophic-backtracking-prone ported X.500 DN regex, a behavior change making publisher validation unconditional (previously only enforced when generating a default cert), and a missing directory-clearing step before move() that can cause repeat electron-forge make runs to fail. These are substantive enough that a human should look regardless of anything I add here, so I am not restating them.
I additionally reviewed two items from the ruled-out candidate list to confirm they are not additional bugs: the finalSay option is folded into the same UNSUPPORTED_OPTIONS warn-and-ignore path as the other legacy hooks (consistent, not a silent behavior change), and toMsixArch's throw on unsupported target architectures is inherited directly from the existing maker-msix implementation, not new logic introduced by this PR that would uniquely need scrutiny here.
Given the confirmed findings already flag the security- and correctness-relevant risk areas (regex DoS surface, validation regression, packaging idempotency), and this PR touches deprecated packaging code with dependency and lockfile changes, human review remains warranted. I am not duplicating the inline findings in this body per instructions, and I have nothing further beyond the two ruled-out items above to add.
…ore move
Addresses three review findings on the legacy MSIX wrapper:
- Rewrite the ported X.500 DN regex so the quoted-value sub-pattern has no
nested overlapping quantifiers ("[^"]*" instead of
"[^"\\]*(?:[^"][^"\\]*)*"). The accepted language is unchanged, but an
unterminated quoted value can no longer trigger catastrophic backtracking.
- Only run the publisher DN validation when no devCert is configured,
matching the base-branch gating (isValidPublisherName only ran inside
createDefaultCertificate). With a devCert the publisher is forwarded
verbatim. The missing/empty-publisher error stays unconditional.
- Restore this.ensureDirectory(outPath) for make/appx/<arch> before moving
the .msix in, so re-running `electron-forge make` overwrites the previous
artifact instead of failing with "dest already exists".
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Resolve the yarn.lock conflict between the removal of the electron-windows-store dependency tree on this branch and the new ink-based multi-logger dependencies on next by taking the lockfile from next and regenerating it with the pinned yarn 4.18.0. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
The merge commit carried the lockfile from next verbatim; regenerate it with the pinned yarn 4.18.0 so the pruned electron-windows-store tree from this branch stays removed alongside the new multi-logger entries. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
There was a problem hiding this comment.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
yarn.lock— Merge commit 7025482 resolved the yarn.lock conflict by keeping the pre-refactor @ electron-forge/maker-appx entry: it still lists cross-spawn and electron-windows-store (with electron-windows-store marked optional) instead of @ electron-forge/maker-msix and electron-windows-msix. This contradicts packages/maker/appx/package.json, which now declares dependencies on @ electron-forge/maker-msix and electron-windows-msix ("^2.0.4") and no longer depends on cross-spawn or electron-windows-store. A lockfile/manifest mismatch like this makesyarn install --immutable(used in CI) fail, and even a mutable install won't link electron-windows-msix into maker-appx's node_modules. Fix: regenerate yarn.lock (e.g.yarn install) so the maker-appx entry matches its package.json dependencies.Extended reasoning...
The merge commit's message says the lockfile conflict was resolved by "taking the lockfile from next and regenerating it with the pinned yarn 4.18.0", but the regenerated maker-appx entry in yarn.lock (lines 843-857) still shows dependencies cross-spawn ^7.0.3 and electron-windows-store ^2.1.0 with electron-windows-store marked optional -- the pre-refactor dependency set. Meanwhile packages/maker/appx/package.json (checked directly) lists @ electron-forge/maker-msix workspace:* and electron-windows-msix ^2.0.4, with no cross-spawn or electron-windows-store. Running
yarn install --immutablein CI compares package.json manifests against yarn.lock and fails because the appx workspace's declared dependency set (per package.json) doesn't match the lockfile entry. Even without --immutable,yarn installon node-modules linker (.yarnrc.yml: nodeLinker: node-modules) would not create a node_modules/electron-windows-msix link scoped correctly for this workspace's own resolution record, and MakerAppX.ts'simport { packageMSIX } from 'electron-windows-msix'and `import { toMsixArch } from '@…Verification: normal. This change introduces a lockfile-vs-package.json mismatch that the base branch does not have. At base (5fb2088), the maker-appx workspace entry in yarn.lock (lines 843-857) matched its package.json:
cross-spawn: "npm:^7.0.3"andelectron-windows-store: "npm:^2.1.0"(optional). package.json at base also declared exactly those (cross-spawn dependency + electron-windows-store…
|
Re the yarn.lock finding on the merge commit 7025482: already addressed in 92c2841, which regenerates the lockfile with the pinned Yarn 4.18.0. At head the Generated by Claude Code |
There was a problem hiding this comment.
Looks good — the three issues flagged in my previous review (unconditional/backtracking-prone publisher validation, and the missing ensureDirectory before move()) have all been fixed in commit 3c51ff1 and are now covered by dedicated regression tests (including a timing assertion against catastrophic backtracking). The subsequent commits only merge/regenerate the lockfile from next with no further changes to the maker-appx code. The two optional nits I raised (finally-block rm potentially masking a successful return, and no warning for signtoolParams/certPass without devCert) remain unaddressed but are minor.
Extended reasoning...
This is a follow-up review of a PR I've already reviewed three times. In the second round I raised three high-severity findings on MakerAppX.ts: (1) make() no longer cleared the output directory before move(), which would fail on re-runs; (2) the ported validDNRegex had an overlapping-quantifier shape vulnerable to catastrophic backtracking on unterminated quoted publisher values; (3) publisher validation ran unconditionally even when a user-supplied devCert was set, unlike the old gated behavior. I verified via git show 3c51ff1 that all three were fixed: ensureDirectory(outPath) is now called before the temp-folder work begins, the quoted-value regex was rewritten to "[^"]*" (no overlapping stars) with an explanatory comment, and the validation check is now gated behind !this.config.devCert. The spec file was extended with tests exercising each fix, including a 50,000-character unterminated-quote publisher with an explicit sub-1-second timing assertion, confirming the backtracking fix actually works rather than just changing behavior superficially.
Two commits landed after the fix commit (7025482 and 92c2841), but both are pure merges from the next branch plus a lockfile regeneration — git show --stat confirms no changes touched packages/maker/appx/src/* in either. So nothing new was introduced to re-review in the maker-appx logic itself.
Two optional (yellow) findings from my third review round remain open in the code: the finally block still unconditionally awaits fs.rm(tmpFolder, ...), which could theoretically mask a successful return if cleanup throws (e.g., a locked file on Windows), and signtoolParams/certPass are still silently dropped without a warning when devCert isn't set (unlike other unsupported options, which are listed and warned). These are edge-case/polish issues rather than functional bugs affecting the main success path, and both were explicitly called out as "(optional)" in my prior review. This run's bug hunt also found no new issues. Given the critical correctness issues are fixed and verified with tests, and no third-party reviewer has an outstanding CHANGES_REQUESTED or unaddressed objection (all prior review activity is from this same automated review identity), I'm comfortable approving while noting the two still-open minor nits in the body for transparency.
`next` was renamed to `main`; bring the branch up to date with the new base. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Pick up the zizmor cache-poisoning fixes from #4401 so the required workflow audit passes on this PR. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
…nd dev cert Addresses three review findings on the legacy MSIX wrapper: - Remove the remaining catastrophic-backtracking paths from the ported X.500 DN regex. Unquoted values now exclude the `;` separator (and `=`/`"`), and whitespace before a separator is owned by the value (or the quoted alternative) instead of also by the separator, so inputs like `C=;C=;C=...` or `C= , C= , C=...` are rejected in linear time. All well-formed publisher forms are still accepted. - Strip the legacy `app\` / `app/` prefix from `packageExecutable` case-insensitively, so `App\MyApp.exe` no longer ends up as `app\App\MyApp.exe` in the manifest. - When no `devCert` is configured, move the `dev_cert.cer` / `dev_cert.pfx` that electron-windows-msix writes into its `outputDir` next to the `.msix` before the temp folder is deleted, so the self-signed certificate can be trusted on a test device like the old maker's dev cert could. The returned artifacts are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Also keep a failed temp-folder cleanup from turning a successful make() into an error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
The .msix is already in place when the generated dev_cert.* files are moved next to it, so a failed move now only warns. Docs, README and the devCert JSDoc all describe the saved development certificate and its password source. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Close two gaps between the old electron-windows-store maker and the electron-windows-msix wrapper that would have broken existing configs: - When `windowsKit` is unset, search the default Windows Kits install folders for makeappx.exe, makepri.exe and signtool.exe (unversioned folder first, then the newest SDK, with the x64 fallback on ARM64 hosts) instead of relying on the single hard-coded SDK version that electron-windows-msix falls back to. This also covers custom manifests whose MinVersion does not match an installed SDK. - Fail early with the expected file names when a custom `assets` folder lacks icon.png, Square44x44Logo.png or Square150x150Logo.png, which the generated manifest refers to, rather than letting makeappx.exe fail on folders that use the old SampleAppx.*.png names. - Document the certificate password, timestamp server, asset naming and Windows Kit lookup differences in the migration notice and config docs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B9pxG5QqvZqoCqacKSpXRN
… host SDK The MakerAppX spec mocked `pathExists` and `fs.readdir` as pass-throughs to the real filesystem, so on GitHub's windows-latest runners (which ship a real Windows 10 SDK) `findWindowsKit()` located `C:\Program Files (x86)\Windows Kits\10\bin\10.0.26100.0\x64` and the default-config tests expecting `windowsKitPath` to be undefined failed. Make the mock defaults hermetic: anything under the Windows Kit roots is reported as missing (`pathExists` -> false, `readdir` -> ENOENT) while other paths still pass through, so the generated dev-certificate tests keep using their real temp folder. Tests that need an SDK still install fake kits via installWindowsKits() or mock fs.readdir explicitly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Resolves the yarn.lock conflict from the dependency bump in #4408 by taking main's lockfile and regenerating it with the pinned Yarn 4.18.0; `yarn install --immutable` passes on the result. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B9pxG5QqvZqoCqacKSpXRN
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
This branch has not been deployed
1 participant
Requested by Erick Zhao · Slack thread
Before:
@electron-forge/maker-appxshelled out toelectron-windows-store(Desktop App Converter, MakeCert,makeappx.exe) and produced a.appx, with its own SDK discovery and dev-cert generation.After: the package is a deprecated compatibility layer over the MSIX code path.
make()mapsMakerAppXConfigontoelectron-windows-msixoptions (sharingtoMsixArchfrom@electron-forge/maker-msix) and moves the resulting.msixtomake/appx/<arch>/<packageName>.msix. Existing configs keep type-checking; the class and unmappable options carry@deprecatedpointing at@electron-forge/maker-msix.electron-windows-storeeverywhere (appxoptionalDependencies, rootoptionalDependencies,yarn.config.cjs, ambient typings, lockfile) plus the now-unusedcross-spawntypings andbogus-private-key.pvkfixture.makeVersionWinStoreCompatible).toMsixArchfrom@electron-forge/maker-msix.appx.md, makers index now cites MSIX,SUPPORT.mddebug namespace updated.assets/manifest/windowsKitpackageAssets/appManifest/windowsKitPathdevCert/certPass/signtoolParamswindowsSignOptions.{certificateFile,certificatePassword,signWithParams}(only whendevCertset)makePri(default off)createPri(defaultfalse, preserving AppX behaviour)packageName(default: name without dashes)manifestVariables.packageIdentity+<packageName>.msixfile namepackageDisplayName/packageDescription/packageBackgroundColormanifestVariablespackageVersion(default<version>.0)manifestVariables.packageVersion, normalized viamakeVersionWinStoreCompatiblepackageExecutable(app\Name.exe)manifestVariables.appExecutablewith theapp\prefix stripped (the library prepends it)publisher(CN=…, validated as X.500 DN)manifestVariables.publisherunchanged (library accepts theCN=form)containerVirtualization,createConfigParams,createPriParams,deploy,desktopConverter,expandedBaseImage,flatten,finalSay,makeappxParamsCompatibility with existing AppX configs (comparing
electron-windows-store2.1.0 againstelectron-windows-msix2.0.4 /@electron/windows-sign1.2.2):Windows Kits\10\bin\x64and everybin\10.*\x64folder;electron-windows-msixonly checks one hard-coded SDK version (10.0.26100.0) when nowindowsKitPathis given, and with a custommanifestit derives the version fromMinVersion(manifests based on the old template say10.0.14316.0). WhenwindowsKitis unset,make()now searches the default Windows Kits folders itself (unversioned first, then newest SDK, x64 fallback on ARM64 hosts) and only falls back to the library default if nothing is found.assets. The generated manifest referencesicon.png,Square44x44Logo.pngandSquare150x150Logo.png, not the oldSampleAppx.*.pngnames, and the user's folder replaces the defaults.make()now fails early naming the missing files (and the old→new names) whenassetsis set without a custommanifest.devCertnow needscertPassunlesssigntoolParamsis passed (old MakeCert certs had no password); signing timestamps againsttimestamp.digicert.comby default (WINDOWS_TIMESTAMP_SERVERoverrides);certPass/signtoolParamswithoutdevCertwarn and are ignored.Tests:
packages/maker/appx/spec/MakerAppX.spec.tsrewritten against a mockedelectron-windows-msix(72 tests: defaults, every mapping, publisher/version errors, per-option warnings, arch conversion, output path, Windows Kit search, asset validation);yarn lint,yarn build,yarn knip,yarn constraintspass.Open questions
.msixundermake/appx/<arch>/(kept the legacy folder so existing publish pipelines still find it).createDefaultCertificateis removed withelectron-windows-store. WithoutdevCert,electron-windows-msixsigns with a self-signed dev cert, which the maker now keeps next to the.msixasdev_cert.cer/dev_cert.pfx(matching the base maker's behaviour;maker-msixitself does not persist it).🤖 Generated with Claude Code
https://claude.ai/code/session_016NS1e4vLnWtkyAYGs2Mrmg
Generated by Claude Code