Skip to content

Commit

Permalink
fix webpack resolver state leakage
Browse files Browse the repository at this point in the history 
I found a case where we were accidentally leaking state between WebpackModuleRequests. This is hard to avoid since webpack *really* pushes us toward using mutation on the shared ResolveData object.

I solved it by making our mutations happen only at the precise moment when we're letting a request bubble back out to webpack.

This is expected to fail tests however because this bug was masking another bug. Specifically, we have several test apps that use things like the `page-title` helper. That helper is (1) used with ambiguous syntax, (2) published in a v2 addon. It turns out we cannot resolve the helper due to #1686, and this was only "working" by accident since the failed attempt at resolving the helper got leaked back to webpack and webpack *does* understand package.json exports and would find it.
  • Loading branch information
@ef4
ef4 committed Dec 6, 2023
1 parent b446d89 commit 000d1ea
Showing 1 changed file with 142 additions and 77 deletions.
219 changes: 142 additions & 77 deletions packages/webpack/src/webpack-resolver-plugin.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { dirname, resolve } from 'path';
import type { ModuleRequest, ResolverFunction, Resolution } from '@embroider/core';
import { Resolver as EmbroiderResolver, ResolverOptions as EmbroiderResolverOptions } from '@embroider/core';
import type { Compiler, Module } from 'webpack';
import type { Compiler, Module, ResolveData } from 'webpack';
import assertNever from 'assert-never';
import escapeRegExp from 'escape-string-regexp';

Expand Down Expand Up @@ -42,29 +42,32 @@ export class EmbroiderPlugin {
let defaultResolve = getDefaultResolveHook(nmf.hooks.resolve.taps);
let adaptedResolve = getAdaptedResolve(defaultResolve);

nmf.hooks.resolve.tapAsync({ name: '@embroider/webpack', stage: 50 }, (state: unknown, callback: CB) => {
let request = WebpackModuleRequest.from(state, this.#babelLoaderPrefix, this.#appRoot);
if (!request) {
defaultResolve(state, callback);
return;
nmf.hooks.resolve.tapAsync(
{ name: '@embroider/webpack', stage: 50 },
(state: ExtendedResolveData, callback: CB) => {
let request = WebpackModuleRequest.from(state, this.#babelLoaderPrefix, this.#appRoot);
if (!request) {
defaultResolve(state, callback);
return;
}

this.#resolver.resolve(request, adaptedResolve).then(
resolution => {
switch (resolution.type) {
case 'not_found':
callback(resolution.err);
break;
case 'found':
callback(null, resolution.result);
break;
default:
throw assertNever(resolution);
}
},
err => callback(err)
);
}

this.#resolver.resolve(request, adaptedResolve).then(
resolution => {
switch (resolution.type) {
case 'not_found':
callback(resolution.err);
break;
case 'found':
callback(null, resolution.result);
break;
default:
throw assertNever(resolution);
}
},
err => callback(err)
);
});
);
});
}
}
Expand All @@ -89,7 +92,7 @@ function getAdaptedResolve(
): ResolverFunction<WebpackModuleRequest, Resolution<Module, null | Error>> {
return function (request: WebpackModuleRequest): Promise<Resolution<Module, null | Error>> {
return new Promise(resolve => {
defaultResolve(request.state, (err, value) => {
defaultResolve(request.toWebpackResolveData(), (err, value) => {
if (err) {
// unfortunately webpack doesn't let us distinguish between Not Found
// and other unexpected exceptions here.
Expand All @@ -102,88 +105,150 @@ function getAdaptedResolve(
};
}

type ExtendedResolveData = ResolveData & {
contextInfo: ResolveData['contextInfo'] & { _embroiderMeta?: Record<string, any> };
};

class WebpackModuleRequest implements ModuleRequest {
readonly specifier: string;
readonly fromFile: string;
readonly meta: Record<string, any> | undefined;

static from(state: any, babelLoaderPrefix: string, appRoot: string): WebpackModuleRequest | undefined {
// when the files emitted from our virtual-loader try to import things,
// those requests show in webpack as having no issuer. But we can see here
// which requests they are and adjust the issuer so they resolve things from
// the correct logical place.
if (!state.contextInfo?.issuer && Array.isArray(state.dependencies)) {
static from(
state: ExtendedResolveData,
babelLoaderPrefix: string,
appRoot: string
): WebpackModuleRequest | undefined {
let specifier = state.request;
if (
specifier.includes(virtualLoaderName) || // prevents recursion on requests we have already sent to our virtual loader
specifier.startsWith('!') // ignores internal webpack resolvers
) {
return;
}

let fromFile: string | undefined;
if (state.contextInfo.issuer) {
fromFile = state.contextInfo.issuer;
} else {
// when the files emitted from our virtual-loader try to import things,
// those requests show in webpack as having no issuer. But we can see here
// which requests they are and adjust the issuer so they resolve things from
// the correct logical place.
for (let dep of state.dependencies) {
let match = virtualRequestPattern.exec(dep._parentModule?.userRequest);
let match = virtualRequestPattern.exec((dep as any)._parentModule?.userRequest);
if (match) {
state.contextInfo.issuer = new URLSearchParams(match.groups!.query).get('f');
state.context = dirname(state.contextInfo.issuer);
fromFile = new URLSearchParams(match.groups!.query).get('f')!;
break;
}
}
}

if (
typeof state.request === 'string' &&
typeof state.context === 'string' &&
typeof state.contextInfo?.issuer === 'string' &&
state.contextInfo.issuer !== '' &&
!state.request.includes(virtualLoaderName) && // prevents recursion on requests we have already sent to our virtual loader
!state.request.startsWith('!') // ignores internal webpack resolvers
) {
return new WebpackModuleRequest(babelLoaderPrefix, appRoot, state);
if (!fromFile) {
return;
}

return new WebpackModuleRequest(
babelLoaderPrefix,
appRoot,
specifier,
fromFile,
state.contextInfo._embroiderMeta,
false,
state
);
}

constructor(
private babelLoaderPrefix: string,
private appRoot: string,
public state: {
request: string;
context: string;
contextInfo: {
issuer: string;
_embroiderMeta?: Record<string, any> | undefined;
};
},
public isVirtual = false
) {
// these get copied here because we mutate the underlying state as we
// convert one request into the next, and it seems better for debuggability
// if the fields on the previous request don't change when you make a new
// one (although it is true that only the newest one has a a valid `state`
// that can actually be handed back to webpack)
this.specifier = state.request;
this.fromFile = state.contextInfo.issuer;
this.meta = state.contextInfo._embroiderMeta ? { ...state.contextInfo._embroiderMeta } : undefined;
}
readonly specifier: string,
readonly fromFile: string,
readonly meta: Record<string, any> | undefined,
readonly isVirtual: boolean,
private originalState: ExtendedResolveData
) {}

get debugType() {
return 'webpack';
}

// Webpack mostly relies on mutation to adjust requests. We could create a
// whole new ResolveData instead, and that would allow defaultResolving to
// happen, but for the output of that process to actually affect the
// downstream code in Webpack we would still need to mutate the original
// ResolveData with the results (primarily the `createData`). So since we
// cannot avoid the mutation anyway, it seems best to do it earlier rather
// than later, so that everything from here forward is "normal".
//
// Technically a NormalModuleLoader `resolve` hook *can* directly return a
// Module, but that is not how the stock one works, and it would force us to
// copy more of Webpack's default behaviors into the inside of our hook. Like,
// we would need to invoke afterResolve, createModule, createModuleClass, etc,
// just like webpack does if we wanted to produce a Module directly.
//
// So the mutation strategy is much less intrusive, even though it means there
// is the risk of state leakage all over the place.
//
// We mitigate that risk by waiting until the last possible moment to apply
// our desired ModuleRequest fields to the ResolveData. This means that as
// requests evolve through the module-resolver they aren't actually all
// mutating the shared state. Only when a request is allowed to bubble back
// out to webpack does that happen.
toWebpackResolveData(): ExtendedResolveData {
this.originalState.request = this.specifier;
this.originalState.context = dirname(this.fromFile);
this.originalState.contextInfo.issuer = this.fromFile;
this.originalState.contextInfo._embroiderMeta = this.meta;
return this.originalState;
}

alias(newSpecifier: string) {
this.state.request = newSpecifier;
return new WebpackModuleRequest(this.babelLoaderPrefix, this.appRoot, this.state) as this;
if (newSpecifier === this.specifier) {
return this;
}
return new WebpackModuleRequest(
this.babelLoaderPrefix,
this.appRoot,
newSpecifier,
this.fromFile,
this.meta,
this.isVirtual,
this.originalState
) as this;
}
rehome(newFromFile: string) {
if (this.fromFile === newFromFile) {
return this;
} else {
this.state.contextInfo.issuer = newFromFile;
this.state.context = dirname(newFromFile);
return new WebpackModuleRequest(this.babelLoaderPrefix, this.appRoot, this.state) as this;
}
return new WebpackModuleRequest(
this.babelLoaderPrefix,
this.appRoot,
this.specifier,
newFromFile,
this.meta,
this.isVirtual,
this.originalState
) as this;
}
virtualize(filename: string) {
let params = new URLSearchParams();
params.set('f', filename);
params.set('a', this.appRoot);
let next = this.alias(`${this.babelLoaderPrefix}${virtualLoaderName}?${params.toString()}!`);
next.isVirtual = true;
return next;
return new WebpackModuleRequest(
this.babelLoaderPrefix,
this.appRoot,
`${this.babelLoaderPrefix}${virtualLoaderName}?${params.toString()}!`,
this.fromFile,
this.meta,
true,
this.originalState
) as this;
}
withMeta(meta: Record<string, any> | undefined): this {
this.state.contextInfo._embroiderMeta = meta;
return new WebpackModuleRequest(this.babelLoaderPrefix, this.appRoot, this.state) as this;
return new WebpackModuleRequest(
this.babelLoaderPrefix,
this.appRoot,
this.specifier,
this.fromFile,
meta,
this.isVirtual,
this.originalState
) as this;
}
}

0 comments on commit 000d1ea

Please sign in to comment.