-
Notifications
You must be signed in to change notification settings - Fork 774
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[miniflare] fix: allow relative modules scriptPath
outside of CWD
#4954
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
--- | ||
"miniflare": patch | ||
--- | ||
|
||
fix: allow relative `scriptPath`/`modulesRoot`s to break out of current working directory | ||
|
||
Previously, Miniflare would resolve relative `scriptPath`s against `moduleRoot` multiple times resulting in incorrect paths and module names. This would lead to `can't use ".." to break out of starting directory` `workerd` errors. This change ensures Miniflare uses `scriptPath` as is, and only resolves it relative to `modulesRoot` when computing module names. Note this bug didn't affect service workers. This allows you to reference a modules `scriptPath` outside the working directory with something like: | ||
|
||
```js | ||
const mf = new Miniflare({ | ||
modules: true, | ||
modulesRoot: "..", | ||
scriptPath: "../worker.mjs", | ||
}); | ||
``` | ||
|
||
Fixes #4721 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,9 @@ | ||
import assert from "assert"; | ||
import fs from "fs/promises"; | ||
import path from "path"; | ||
import test from "ava"; | ||
import { Miniflare, MiniflareCoreError, stripAnsi } from "miniflare"; | ||
import { utf8Encode } from "../../test-shared"; | ||
import { useCwd, useTmp, utf8Encode } from "../../test-shared"; | ||
|
||
const ROOT = path.resolve( | ||
__dirname, | ||
|
@@ -278,6 +279,30 @@ You must manually define your modules when constructing Miniflare: | |
at ${depPath}:2:8` | ||
); | ||
}); | ||
test.serial( | ||
"Miniflare: collects modules outside of working directory", | ||
async (t) => { | ||
// https://github.com/cloudflare/workers-sdk/issues/4721 | ||
const tmp = await useTmp(t); | ||
const child = path.join(tmp, "child"); | ||
await fs.mkdir(child); | ||
await fs.writeFile( | ||
path.join(tmp, "worker.mjs"), | ||
'export default { fetch() { return new Response("body"); } }' | ||
); | ||
useCwd(t, child); | ||
|
||
const mf = new Miniflare({ | ||
modules: true, | ||
modulesRoot: "..", | ||
scriptPath: "../worker.mjs", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we avoid the cwd change+resetting by choosing another tmp dir for the worker and using a relative path from the cwd to the other tmp dir? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we could. Was keen to test this behaviour using the CWD specifically since it's very simple, and is what users are most likely to hit. The |
||
}); | ||
t.teardown(() => mf.dispose()); | ||
|
||
const res = await mf.dispatchFetch("http://localhost"); | ||
t.is(await res.text(), "body"); | ||
} | ||
); | ||
test("Miniflare: suggests bundling on unknown module", async (t) => { | ||
// Try with npm-package-like import | ||
let mf = new Miniflare({ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is safe to make. Resolving to the
modulesRoot
root isn't correct here ifmodulePath
is relative. IfmodulePath
were absolute, this call would do nothing anyways.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
scriptPath
should always be relative tocwd
(orscript:NN
if provided inline). So if you wanted to force this to be absolute you would be more likely wanting to do onlypath.resolve(modulePath)
.Interestingly, I assumed that this function would be recursive but it isn't. It is only called in
getWorkerScript()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, the mutually recursive functions are
#visitJavaScriptModule()
and#visitModule()
below. 👍