Skip to content
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

fix!: return 404 for resources requests outside the base path #5657

Merged
merged 12 commits into from
Oct 12, 2023

Conversation

Artur-
Copy link
Contributor

@Artur- Artur- commented Nov 12, 2021

Fixes #5656

Description

Return 404 for resources outside the base path

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 Commit 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.

@patak-dev patak-dev changed the title fix: Return 404 for resources requests outside the base path fix: return 404 for resources requests outside the base path Mar 10, 2023
@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label May 22, 2023
@sapphi-red sapphi-red added this to the 5.0 milestone May 22, 2023
@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Oct 12, 2023

📝 Ran ecosystem CI: Open

suite result
analogjs ✅ success
astro ✅ success
histoire ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
nx ✅ success
previewjs ✅ success
qwik ❌ failure
rakkas ✅ success
sveltekit ❌ failure
unocss ✅ success
vike ✅ success
vite-plugin-pwa ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ❌ failure

@sapphi-red
Copy link
Member

qwik and vitest are failing with main branch too.
plugin-vue's fail is a flaky one.
svelte kit's fail is fine I guess. The test expects https://github.com/sveltejs/kit/blob/6dd025ceaae3ee17b11744b4ac01dccd61755fc0/packages/kit/src/exports/vite/utils.js#L96
https://github.com/sveltejs/kit/blob/6dd025ceaae3ee17b11744b4ac01dccd61755fc0/packages/kit/src/exports/vite/dev/index.js#L439-L441
to respond but because our base middleware is before that, our middleware responds with a blank text instead.

sapphi-red
sapphi-red previously approved these changes Oct 12, 2023
sapphi-red
sapphi-red previously approved these changes Oct 12, 2023
@bluwy
Copy link
Member

bluwy commented Oct 12, 2023

Maybe we could also add a similar hint like SvelteKit? Seems like a useful addition. I also think we should have a test for this before merging.

@sapphi-red
Copy link
Member

Added a message same with SvelteKit and also added a test 👍

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Oct 12, 2023

📝 Ran ecosystem CI: Open

suite result
analogjs ✅ success
astro ✅ success
histoire ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
nx ✅ success
previewjs ✅ success
qwik ❌ failure
rakkas ✅ success
sveltekit ✅ success
unocss ✅ success
vike ✅ success
vite-plugin-pwa ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ❌ failure

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@bluwy bluwy changed the title fix: return 404 for resources requests outside the base path fix!: return 404 for resources requests outside the base path Oct 12, 2023
@patak-dev patak-dev merged commit 40fd2d9 into vitejs:main Oct 12, 2023
10 checks passed
mbland added a commit to mbland/vitest-utils-browser-import that referenced this pull request Dec 6, 2023
These changes helped me find the definitive source of the problem in
Vite 5.0.0-beta.8:

- "fix!: return 404 for resources requests outside the base path"
  vitejs/vite@40fd2d9
  vitejs/vite#5657
  vitejs/vite#5656
  https://vitejs.dev/guide/migration.html#advanced

- Related:
  vuejs/vitepress#3239
  sveltejs/kit#2958

Of course, this is effectively a bug fix for Vite, pointing at a bug in
Vitest. Vite is now more strict about enforcing the Public Base Path,
and Vitest needs to propagate config.base somehow:

- https://vitejs.dev/config/shared-options.html#base
- https://vitejs.dev/guide/build.html#public-base-path

I'll explain the workaround in the next commit. What follows describes
how my methodology to pinpoint the problem evolved and concluded.

---

Adding the vite/packages/vite as a "file:" dependency and adding its
directory as part of the workspace proved challenging. The TypeScript
compiler (`tsc`) kept erroring out on the `tsc --emitDeclarationOnly
--outDir temp/node -p src/node` step of the build.

It took a few tries to realize that I needed to remove vite/node_modules
before every "pnpm build" in vite/. Somehow some stale artifacts were
poisoning subsequent runs.

That left the problem of running `pnpm i` in this project, which would
still try to rebuild vite/packages/vite and choke on the TypeScript.

After skimming vite/README.md, I realized I could try adding
vite/packages/vite as a "link:" dependency instead of a "file:"
dependency. I also learned I could use pnpm.overrides in package.json to
ensure other packages used this local version. I kept vitest/packages/*
in the workspace to make sure they used this linked version of
vite/packages/vite.

After that, I could now reliably build the two local packages, and have
the local vitest use the local vite.

Each time in the ../vite repository, I would:

```sh
rm -rf packages/vite/node_modules
git reset --hard <tag or commit>
pnpm i --frozen-lockfile
pnpm build
```

Then I'd go back to this repository and:

```sh
pnpm i
pnpm test
```

And here's the results of my bisection (I didn't actually use `git
bisect`, but followed the same essential process):

```text
v5.0.0-beta.12 FAIL
v5.0.0-beta.6  OK
v5.0.0-beta.9  FAIL
v5.0.0-beta.7  OK
v5.0.0-beta.8  FAIL
```text

Once I had my culprit, I went back into ../vite and ran:

```sh
gitk v5.0.0-beta.7..v5.0.0-beta.8
```

And once I did that, I could see the the commit immediately after
v5.0.0-beta.7 affected base path handling. So I repeated the process,
setting `<tag or commit>` in ../vite to
40fd2d9bf4073420e6c334f48dc3b63558b688ce. Indeed, this build failed,
confirming that commit as the culprit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using a base path, resources should not be served without the base path
4 participants