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.

Sign up for GitHub

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

Jump to bottom

Fix truncated basepath #3345

Merged
merged 7 commits into from Jan 15, 2022
Merged

Fix truncated basepath #3345

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
71f2f46
Fix svelte-kit dev/preview when config.kit.paths.base is set
andrewsoutar Jan 14, 2022
d2e4aea
add test of truncated base
Rich-Harris Jan 14, 2022
7ff5b81
changeset
Rich-Harris Jan 14, 2022
0d6ef14
handle edge cases
Rich-Harris Jan 14, 2022
700ad5f
actually i guess this belongs here
Rich-Harris Jan 14, 2022
2980840
decode then check
Rich-Harris Jan 14, 2022
291c4f6
fix newly failing test
Rich-Harris Jan 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/curly-pets-vanish.md
View file
Open in desktop
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

Handle requests for /basepath
4 changes: 3 additions & 1 deletion packages/kit/src/core/adapt/prerender/prerender.js
View file
Open in desktop
Expand Up @@ -229,7 +229,9 @@ export async function prerender({ cwd, out, log, config, build_data, fallback, a
if (!is_root_relative(resolved)) continue;

const parsed = new URL(resolved, 'http://localhost');
const pathname = decodeURI(parsed.pathname).replace(config.kit.paths.base, '');

if (!parsed.pathname.startsWith(config.kit.paths.base)) continue;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to decode first before checking this?

Copy link
Contributor

Choose a reason for hiding this comment

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

In a couple of other places, it seems to be checked without decoding. paths.base doesn't seem to be documented one way or the other (whether it's encoded or decoded); on the other hand, the encoded form isn't canonical, so e.g. even with a "normal"-looking base of /foo, someone visiting via /%66%6f%6f might not match, even though it's the "same" URL. (The same issue, I would imagine, applies to the other places where it's checked as well - including probably in adapters - and behavior would vary depending on whether whatever is upstream canonicalizes the URL.) Honestly, not sure how much of a problem that is.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated the PR

const pathname = decodeURI(parsed.pathname).slice(config.kit.paths.base.length) || '/';

const file = pathname.slice(1);
if (files.has(file)) continue;
Expand Down
4 changes: 3 additions & 1 deletion packages/kit/src/runtime/server/index.js
View file
Open in desktop
Expand Up @@ -90,6 +90,8 @@ export async function respond(incoming, options, state = {}) {
resolve: async (request, opts) => {
if (opts && 'ssr' in opts) ssr = /** @type {boolean} */ (opts.ssr);

if (!request.url.pathname.startsWith(options.paths.base)) return;

if (state.prerender && state.prerender.fallback) {
return await render_response({
url: request.url,
Expand All @@ -105,7 +107,7 @@ export async function respond(incoming, options, state = {}) {
});
}

const decoded = decodeURI(request.url.pathname).replace(options.paths.base, '');
const decoded = decodeURI(request.url.pathname).slice(options.paths.base.length) || '/';

for (const route of options.manifest._.routes) {
const match = route.pattern.exec(decoded);
Expand Down
3 changes: 3 additions & 0 deletions packages/kit/test/apps/options-2/svelte.config.js
View file
Open in desktop
@@ -1,6 +1,9 @@
/** @type {import('@sveltejs/kit').Config} */
const config = {
kit: {
paths: {
base: '/basepath'
},
serviceWorker: {
register: false
}
Expand Down
16 changes: 9 additions & 7 deletions packages/kit/test/apps/options-2/test/test.js
View file
Open in desktop
Expand Up @@ -3,22 +3,24 @@ import { test } from '../../../utils.js';

/** @typedef {import('@playwright/test').Response} Response */

test.describe.parallel('Service worker', () => {
test('serves /', async ({ page }) => {
await page.goto('/');
test.describe.parallel('paths.base', () => {
test('serves /basepath', async ({ page }) => {
await page.goto('/basepath');
expect(await page.textContent('h1')).toBe('Hello');
});
});

test.describe.parallel('Service worker', () => {
if (!process.env.DEV) {
test('build /service-worker.js', async ({ request }) => {
const response = await request.get('/service-worker.js');
test('build /basepath/service-worker.js', async ({ request }) => {
const response = await request.get('/basepath/service-worker.js');
const content = await response.text();

expect(content).toMatch(/\/_app\/start-[a-z0-9]+\.js/);
});

test('does not register /service-worker.js', async ({ page }) => {
await page.goto('/');
test('does not register /basepath/service-worker.js', async ({ page }) => {
await page.goto('/basepath');
expect(await page.content()).not.toMatch(/navigator\.serviceWorker/);
});
}
Expand Down