fix(CodeSigningPlugin): sign assets at processAssets ANALYSE stage before REPORT#1379
fix(CodeSigningPlugin): sign assets at processAssets ANALYSE stage before REPORT#1379JhohellsDL wants to merge 2 commits intocallstack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 94a9e64 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@JhohellsDL is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
|
Is there some simple way to test this locally? What would you recommend? @JhohellsDL |
There was a problem hiding this comment.
the PR is pretty solid 👍
I tested it locally with apps/tester-app using temporary processAssets REPORT-stage capture plugin & the results satisfied my expectations:
- capture report showed all remote chunks signed at REPORT stage (they're showed unsigned on
mainbranch tho) - remote chunk loads successfully when
verifyScriptSignature: 'strict'& public key embedded in the app
Screen.Recording.2026-04-14.at.17.17.40.mp4
can you just consider a few suggestions before merge:
| compiler.hooks.thisCompilation.tap( | ||
| 'RepackCodeSigningPlugin', | ||
| (compilation) => { | ||
| // @ts-ignore — sources is available on both rspack and webpack compilers |
There was a problem hiding this comment.
is it possible to get rid of @ts-ignore here?
| expect(chunkBundle.length).toBeGreaterThan(1280); | ||
| }); | ||
|
|
||
| it('exposes signed chunk assets to processAssets REPORT (after ANALYSE signing)', async () => { |
There was a problem hiding this comment.
this test proves assets are signed by REPORT, which is good 👍 but what about one more test for asserting the actual delta from old behavior? it might be done by checking that content at REPORT is larger than unsigned chunk or by detecting /* RCSSB */ substring in content
wdyt?
There was a problem hiding this comment.
Good call. I extended the test so it doesn’t only assert the JWT-shaped tail at REPORT.
There is now a processAssets tap at one stage before ANALYSE (same chunk iteration as REPORT) to snapshot the asset before CodeSigningPlugin runs. Then at REPORT we assert:
the pre-sign snapshot has no /* RCSSB / marker
the REPORT snapshot includes / RCSSB */
REPORT content is strictly longer than the pre-sign snapshot
The existing regex checks on the chunk vs main bundle are unchanged. That should document the regression we care about: anything observing assets at REPORT must see the already signed bytes, not only the final emitted file.
| const source = asset.source.source(); | ||
| const content = Buffer.isBuffer(source) | ||
| ? source | ||
| : Buffer.from(source); | ||
|
|
||
| logger.debug(`Signing ${file}`); | ||
| /** generate bundle hash */ | ||
| const hash = crypto | ||
| .createHash('sha256') | ||
| .update(content) | ||
| .digest('hex'); | ||
| /** generate token */ | ||
| const token = jwt.sign({ hash }, privateKey, { | ||
| algorithm: 'RS256', | ||
| }); | ||
| /** combine the bundle and the token */ | ||
| const signedBundle = Buffer.concat( | ||
| [content, Buffer.from(BEGIN_CS_MARK), Buffer.from(token)], | ||
| content.length + TOKEN_BUFFER_SIZE | ||
| ); |
There was a problem hiding this comment.
[nit]
what about making this logic a bit more readable by moving this code into dedicated signAsset() helper?
| const source = asset.source.source(); | |
| const content = Buffer.isBuffer(source) | |
| ? source | |
| : Buffer.from(source); | |
| logger.debug(`Signing ${file}`); | |
| /** generate bundle hash */ | |
| const hash = crypto | |
| .createHash('sha256') | |
| .update(content) | |
| .digest('hex'); | |
| /** generate token */ | |
| const token = jwt.sign({ hash }, privateKey, { | |
| algorithm: 'RS256', | |
| }); | |
| /** combine the bundle and the token */ | |
| const signedBundle = Buffer.concat( | |
| [content, Buffer.from(BEGIN_CS_MARK), Buffer.from(token)], | |
| content.length + TOKEN_BUFFER_SIZE | |
| ); | |
| const signedBundle = this.signAsset(file, asset, privateKey, BEGIN_CS_MARK, TOKEN_BUFFER_SIZE); |
There was a problem hiding this comment.
Addressed the nit: extracted the hash/JWT/buffer concat path into a private signAsset() helper so the processAssets callback stays easier to scan. Behavior unchanged; CodeSigningPlugin tests still pass.
| // Sign at ANALYSE (2000) so assets are signed before any plugin | ||
| // running at REPORT (5000) — e.g. withZephyr() — captures them. | ||
| // The original assetEmitted hook fires after processAssets completes, | ||
| // which is too late when Zephyr uploads assets at REPORT stage. |
There was a problem hiding this comment.
[nit] this comment might be a bit shorter
| // Sign at ANALYSE (2000) so assets are signed before any plugin | |
| // running at REPORT (5000) — e.g. withZephyr() — captures them. | |
| // The original assetEmitted hook fires after processAssets completes, | |
| // which is too late when Zephyr uploads assets at REPORT stage. | |
| // Sign at ANALYSE (2000) so later processAssets consumers, | |
| // such as Zephyr at REPORT (5000), receive already-signed assets |
3 participants
Summary
Fixes #1377
CodeSigningPluginwas signing bundles incompiler.hooks.assetEmitted,which fires after
processAssetscompletes. When usingwithZephyr(),Zephyr captures and uploads assets at
PROCESS_ASSETS_STAGE_REPORT(5000)— before
assetEmittedfires — resulting in unsigned bundles being uploadedto the CDN, making
verifyScriptSignature: 'strict'ineffective.Changes
assetEmittedtoprocessAssetsatPROCESS_ASSETS_STAGE_ANALYSE(2000), before Zephyr'sREPORTstage (5000)compilation.updateAsset()instead of reading/writing from disk
chunkFilenamesSet andemithook — no longer needed sincesigning iterates
compilation.chunksdirectly insideprocessAssetsREPORTstage## Behaviorsection explaining the signing stageTesting
REPORTstage confirmingassets are already signed when captured
withZephyr()— bundlesuploaded to CDN now contain the signature and
verifyScriptSignature: 'strict'works correctly