flarum / flarum/framework

Chunk id collisions between extensions: ExportRegistry.chunks is not keyed by namespace

Open
#5,027 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
6.7k
Forks
883
Avg merge
15h 16m
Merged PRs (30d)
73

Description

## Current Behavior

`ExportRegistry.chunks` is keyed by chunk id alone, with no namespace:

```ts
// framework/core/js/src/common/ExportRegistry.ts:143-158
addChunkModule(chunkId, moduleId, namespace, urlPath): void {
if (!this.chunks.has(chunkId.toString())) {
this.chunks.set(chunkId.toString(), { namespace, urlPath, modules: [urlPath] });
} else {
this.chunks.get(chunkId.toString())?.modules?.push(urlPath);
}

this.chunkModules.set(`${namespace}:${urlPath}`, { chunkId, moduleId });
}
```

Every extension gets its chunk ids from `DeterministicChunkIdsPlugin` with `maxLength: 3`, so all extensions draw from the same 1000 slots with no coordination. When two extensions get the same id, the second one is appended to the first one's `modules` array, and `chunkUrl()` (`:211-228`) returns the first one's asset URL for both.

The second extension's `import()` then either fetches the wrong file, or throws `TypeError: Cannot read properties of undefined (reading 'call')` with no network request at all. The second case happens because all extensions share one jsonp array (`webpackChunkmodule_exports`, from `output.library: 'module.exports'`), so a foreign chunk marks the id installed in every runtime.

Ids are hashed from the chunk name, not its contents, so a collision does not go away when either extension changes its code. Nothing warns about it either: `addChunkModule` just takes the `else` branch.

`flarum/audit` has `forum/components/ActorAuditModal` at id 767 today. Any extension with a chunk name hashing to 767 collides with it.

Collision chance for a new extension against the 25 forum ids the bundled set uses today:

| chunks emitted | P(collision) |
|---|---|
| 3 | 7% |
| 26 | 49% |

## Steps to Reproduce

1. Confirm two names collide, using webpack's own hash function:

```js
const numberHash = require('./node_modules/webpack/lib/util/numberHash');
for (const name of ['forum/components/ActorAuditModal', 'common/builder/canvas/minimap'])
console.log(name, numberHash(name + '0', 1000)); // both print 767
```

2. Build two extensions with different composer names, one lazily importing `src/forum/components/ActorAuditModal`, the other `src/common/builder/canvas/minimap`. Their `js/dist/forum.js` files contain:

```
flarum.reg.addChunkModule("767","936","flarum-audit","forum/components/ActorAuditModal")
flarum.reg.addChunkModule("767","264","acme-widgets","common/builder/canvas/minimap")
```

3. Install both, load a forum page, and check the registry:

```js
flarum.reg.chunkUrl(767)
// "/assets/js/flarum-audit/forum/components/ActorAuditModal.js"
// expected "/assets/js/acme-widgets/common/builder/canvas/minimap.js"
```

4. Open the feature in the second extension that lazily imports its chunk. If audit's chunk 767 was loaded first, the import resolves with no network request and throws `TypeError`. If it was not, the wrong file is fetched and the import fails with `ChunkLoadError`.

## Expected Behavior

Each extension's `import()` resolves to its own chunk, whatever else is installed and whatever order extensions register in. A chunk id only means something inside the extension that emitted it, so `chunks` should be keyed by namespace and id, the way `chunkModules` already is.

## Environment

- Flarum version: 2.0.0-rc.8, `2.x` branch at ecd7962
- Website URL: n/a, local checkout
- Webserver: n/a
- Hosting environment: local development
- PHP version: 8.5.9
- Database: n/a
- Browser: not reproduced in a browser yet, see below
- flarum-webpack-config: 3.0.4, webpack 5.110.3, Node 26.5.0

I reproduced this by transpiling `ExportRegistry.ts` from the repo and replaying the real `addChunkModule` calls from two compiled bundles against it, then evaluating both entry bundles in one Node vm with a fake network. The jsonp code and module tables are webpack's real output, but the script tag and the network are simulated. I have not opened it in a browser yet. I can do that first if you want it before looking at a fix.

## Possible Solution

Key `chunks` by `${namespace}:${chunkId}`, and add an optional trailing `namespace` argument to the three lookup methods.

`framework/core/js/src/common/ExportRegistry.ts`:

```diff
addChunkModule(chunkId, moduleId, namespace, urlPath): void {
- if (!this.chunks.has(chunkId.toString())) {
- this.chunks.set(chunkId.toString(), { namespace, urlPath, modules: [urlPath] });
+ const key = `${namespace}:${chunkId}`;
+ if (!this.chunks.has(key)) {
+ this.chunks.set(key, { namespace, urlPath, modules: [urlPath] });
} else {
- this.chunks.get(chunkId.toString())?.modules?.push(urlPath);
+ this.chunks.get(key)?.modules?.push(urlPath);
}

- getChunk(chunkId: number | string): Chunk | null {
- const chunk = this.chunks.get(chunkId.toString()) ?? null;
+ getChunk(chunkId: number | string, namespace?: string): Chunk | null {
+ // No namespace means a bundle built before this change.
+ const legacyKey = namespace ? null : [...this.chunks.keys()].find((k) => k.endsWith(`:${chunkId}`));
+ const chunk = this.chunks.get(namespace ? `${namespace}:${chunkId}` : legacyKey ?? '') ?? null;

- async loadChunk(original, url, done, key, chunkId): Promise {
- const chunkUrl = this.chunkUrl(chunkId) || url;
+ async loadChunk(original, url, done, key, chunkId, namespace?: string): Promise {
+ const chunkUrl = this.chunkUrl(chunkId, namespace) || url;

- chunkUrl(chunkId: number | string): string | null {
- const chunk = this.getChunk(chunkId.toString());
+ chunkUrl(chunkId: number | string, namespace?: string): string | null {
+ const chunk = this.getChunk(chunkId.toString(), namespace);
```

plus the same optional parameters on `IChunkRegistry` (`:48`, `:53`).

`js-packages/webpack-config/src/OverrideChunkLoaderFunction.cjs:21`, so the runtime can say which extension is asking. `namespace` is already computed at `:11`:

```diff
- '\nconst originalLoadChunk = __webpack_require__.l;' +
- '\n__webpack_require__.l = flarum.reg.loadChunk.bind(flarum.reg, originalLoadChunk);'
+ '\nconst originalLoadChunk = __webpack_require__.l;' +
+ '\n__webpack_require__.l = (url, done, key, chunkId) =>' +
+ ` flarum.reg.loadChunk(originalLoadChunk, url, done, key, chunkId, ${JSON.stringify(namespace)});`
```

With both applied, the correct file is fetched and the correct module is returned.

Notes:

- `namespace` is optional and last, so bundles built with the current webpack-config keep working. They take the legacy branch and get today's first-registrant-wins behaviour.
- The fix only helps extensions rebuilt with the updated webpack-config, since the namespace has to come from the emitted runtime. Already published bundles keep colliding.
- `getChunk`, `chunkUrl` and `loadChunk` are on the public `IChunkRegistry`, but I found no call site outside `ExportRegistry` itself and the webpack runtime, in core, `js-packages` or `extensions/*/js/src`.
- Falling back to the `url` webpack computed does not work here. `Extend\Frontend` adds every extension's dist as a source file to the same compiler and `Assets::makeJs()` concatenates them into one asset, so every runtime auto-detects the same `publicPath`. That is why core overrides `__webpack_require__.l` in the first place.
- If changing the signatures is not wanted, a `console.warn` in `addChunkModule` when a second namespace registers an id already present would at least make this diagnosable. That is two lines and fixes nothing.

Until there is a release, an extension can work around it with `optimization.chunkIds: false` plus a small plugin setting `chunk.id` to `/`, and `output.chunkLoadingGlobal: 'webpackChunk'`. Both are needed. Over a 29 file package that cost me 332 gzip bytes in total.

I am happy to open a PR with the two file change and a regression test.

Contributor guide

Open the contributing guide

Research direction

Start with framework/core/js/src/common/ExportRegistry.ts, especially addChunkModule, getChunk, chunkUrl, and loadChunk, then inspect js-packages/webpack-config/src/OverrideChunkLoaderFunction.cjs. Reproduce the collision using the two names in the issue and trace the emitted runtime namespace. Done means independently registered extension chunks resolve to their own assets and modules, with a regression test covering the collision.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, typescript, webpack
Domain
build-system, frontend, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
65/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.