fix(importAnalysis): strip url base before passing as safeModulePaths #13712

xinxinhe1810 · 2023-07-04T11:00:58Z

update call stripBase in utils.ts before passing to fsPathFromUrl (fix #9438)

Description

Additional context

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the PR Title Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

…FromUrl (fix vitejs#9438)

patak-dev

Tried it adding a /base/ to the fs-serve playground and it is working as expected. We could expand the tests, but I prefer to keep the current fs-serve using the default base.

playground/fs-serve/__tests__/fs-serve.spec.ts

playground/fs-serve/root/src/index.html

xinxinhe1810 · 2023-07-05T11:20:09Z

playground/fs-serve/__tests__/base/serve.ts

+    },
+  }
+
+  const rewriteTestRootOptions = {


You cannot directly use the configuration from vite.config-base.js here. In vite.config-base.js, the variable __dirname is used, which returns a value based on "playground" instead of "playground-temp" that is used for testing. I guess this may be a bug. That's why I am using "serve" to start the testing server instead of relying on vite.config-base.js.

The config file read in __tests__ from vite.config.js is different from the one read in playground-temp, especially when it involves expressions like __dirname.
like:

{ build: { rollupOptions: { input: { main: path.resolve(__dirname, 'src/index.html'), }, }, }, server: { fs: { strict: true, allow: [path.resolve(__dirname, 'src')], }, }

This is strange, we are using __dirname in the worker and assets playgrounds too in the configs. Maybe there is an issue here because the root is set?

You were correct, I sent a PR to fix the config variants handling here:

test: fix playground config variants #13725

playground/fs-serve/root/vite.config-base.js

patak-dev · 2023-07-05T19:23:59Z

CI tests always fail. What is the reason? I haven't encountered this failure locally.

These are flaky tests. We're having issues with them since last week, it is unrelated to this PR.

benmccann · 2023-07-07T14:58:19Z

packages/vite/src/node/plugins/importAnalysis.ts

@@ -556,8 +556,10 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
              }
            }

-            // record as safe modules
-            server?.moduleGraph.safeModulesPath.add(fsPathFromUrl(url))
+            // record as safe modules # stripBase: https://github.com/vitejs/vite/issues/9438#issuecomment-1486662486


it we're going to link to a comment it should probably be #9438 (comment), but it'd be nicer to just explain why it's necessary here rather than making someone copy paste a URL into their browser to read the explanation (same below)

I tried to fix it. Thanks for guiding me on this PR.

sapphi-red · 2023-07-24T05:25:56Z

packages/vite/src/node/server/middlewares/static.ts

+  // When the base path is set, the safeModulesPath does not include the base prefix internally.
+  // Therefore, when retrieving a file from safeModulesPath, the base path should be stripped.
+  // See https://github.com/vitejs/vite/issues/9438#issuecomment-1465270409
+  const file = fsPathFromUrl(stripBase(url, server.config.rawBase))


This function is called from serveRawFsMiddleware/serveStaticMiddleware/transformMiddleware that runs after baseMiddleware (this middleware strips the base). So I think we shouldn't run stripBase here.

vite/packages/vite/src/node/server/index.ts

Lines 614 to 646 in 1292ad0

// base

if (config.base !== '/') {

middlewares.use(baseMiddleware(server))

}

// open in editor support

middlewares.use('/__open-in-editor', launchEditorMiddleware())

// ping request handler

// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`

middlewares.use(function viteHMRPingMiddleware(req, res, next) {

if (req.headers['accept'] === 'text/x-vite-ping') {

res.writeHead(204).end()

} else {

next()

}

})

// serve static files under /public

// this applies before the transform middleware so that these files are served

// as-is without transforms.

if (config.publicDir) {

middlewares.use(

servePublicMiddleware(config.publicDir, config.server.headers),

)

}

// main transform middleware

middlewares.use(transformMiddleware(server))

// serve static files

middlewares.use(serveRawFsMiddleware(server))

middlewares.use(serveStaticMiddleware(root, server))

So that's how it is. I remove stripBase here

benmccann · 2023-07-24T15:14:48Z

playground/fs-serve/root/src/index.html

  // inside allowed dir, safe fetch
-  fetch('/src/safe.txt')
+  fetch(base + '/src/safe.txt')


if we've got /base/ then we'll end up with a //. This should probably use joinUrlSegments or path.posix.join

i use joinUrlSegments to fix that

packages/vite/src/node/plugins/importAnalysis.ts

Co-authored-by: Ben McCann <[email protected]>

The config will be loaded automatically as vitejs#13725 is merged

patak-dev · 2023-07-29T09:54:30Z

IIUC, this issue can't be exploited, no? In that case, let's merge it for Vite 5 (we'll start the beta soon). @sapphi-red let me know if you think it is important to get this one in a patch though.

bluwy · 2023-07-29T13:55:23Z

I'm not sure if we need to hold off this change. I think it'd be nice to fix as patch though.

sapphi-red · 2023-07-29T14:25:13Z

I think this issue can't be exploited. But I think it'd be good to merge this in a patch.

…s safeModulePaths

xinxinhe1810 and others added 2 commits July 4, 2023 18:59

fix(node): update call stripBase in utils.ts before passing to fsPath…

fa92a74

…FromUrl (fix vitejs#9438)

Merge branch 'vitejs:main' into fix/base-module-path

4fcfcf4

patak-dev previously approved these changes Jul 4, 2023

View reviewed changes

patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Jul 4, 2023

add test for base fs-serve

7dd7d4c

xinxinhe1810 dismissed patak-dev’s stale review via 7dd7d4c July 5, 2023 11:12

xinxinhe1810 commented Jul 5, 2023

View reviewed changes

This comment was marked as off-topic.

Sign in to view

patak-dev reviewed Jul 5, 2023

View reviewed changes

playground/fs-serve/root/vite.config-base.js Outdated Show resolved Hide resolved

patak-dev mentioned this pull request Jul 5, 2023

test: fix playground config variants #13725

Merged

4 tasks

update base for Absolute URL pathname

f9831af

benmccann reviewed Jul 7, 2023

View reviewed changes

xinxinhe1810 and others added 2 commits July 10, 2023 11:00

update: Modify comments for better

1cc1ef8

Merge branch 'vitejs:main' into fix/base-module-path

aaf7799

xinxinhe1810 requested review from patak-dev and benmccann July 14, 2023 10:00

sapphi-red reviewed Jul 24, 2023

View reviewed changes

chore: isFileServingAllowed is doesn't need stripBase

eb81f16

benmccann reviewed Jul 24, 2023

View reviewed changes

packages/vite/src/node/plugins/importAnalysis.ts Outdated Show resolved Hide resolved

xinxinhe1810 and others added 3 commits July 25, 2023 10:34

Update packages/vite/src/node/plugins/importAnalysis.ts

5bb2605

Co-authored-by: Ben McCann <[email protected]>

chore: use joinUrlSegments join fetch url

d075dff

chore: remove serve.ts

4556271

The config will be loaded automatically as vitejs#13725 is merged

sapphi-red approved these changes Jul 25, 2023

View reviewed changes

patak-dev added this to the 5.0 milestone Jul 29, 2023

bluwy approved these changes Jul 29, 2023

View reviewed changes

bluwy changed the title ~~fix(node): update call stripBase in utils.ts before passing to fsPath…~~ fix(importAnalysis): strip url base before passing as safeModulePaths Jul 29, 2023

patak-dev merged commit 1ab06a8 into vitejs:main Jul 29, 2023

patak-dev removed this from the 5.0 milestone Jul 29, 2023

xinxinhe1810 deleted the fix/base-module-path branch July 30, 2023 07:04

sapphi-red added a commit that referenced this pull request Sep 17, 2024

fix(importAnalysis): backport #13712, strip url base before passing a…

fa25564

…s safeModulePaths

patak-dev pushed a commit that referenced this pull request Sep 17, 2024

fix(importAnalysis): backport #13712, strip url base before passing a…

5d1d81e

…s safeModulePaths

	// base
	if (config.base !== '/') {
	middlewares.use(baseMiddleware(server))
	}

	// open in editor support
	middlewares.use('/__open-in-editor', launchEditorMiddleware())

	// ping request handler
	// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
	middlewares.use(function viteHMRPingMiddleware(req, res, next) {
	if (req.headers['accept'] === 'text/x-vite-ping') {
	res.writeHead(204).end()
	} else {
	next()
	}
	})

	// serve static files under /public
	// this applies before the transform middleware so that these files are served
	// as-is without transforms.
	if (config.publicDir) {
	middlewares.use(
	servePublicMiddleware(config.publicDir, config.server.headers),
	)
	}

	// main transform middleware
	middlewares.use(transformMiddleware(server))

	// serve static files
	middlewares.use(serveRawFsMiddleware(server))
	middlewares.use(serveStaticMiddleware(root, server))

Uh oh!

fix(importAnalysis): strip url base before passing as safeModulePaths #13712

fix(importAnalysis): strip url base before passing as safeModulePaths #13712

Uh oh!

Conversation

xinxinhe1810 commented Jul 4, 2023

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

patak-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xinxinhe1810 Jul 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

patak-dev commented Jul 5, 2023

Uh oh!

benmccann Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patak-dev commented Jul 29, 2023

Uh oh!

bluwy commented Jul 29, 2023

Uh oh!

sapphi-red commented Jul 29, 2023

Uh oh!

Uh oh!

xinxinhe1810 Jul 5, 2023 •

edited

Loading

benmccann Jul 7, 2023 •

edited

Loading