Fix Astro Preview Pathing Issues (#2338)

* Fix preview issues triggered by pageUrlFormat & trailingSlash options

* format

* changeset

* nit: fix host message

.changeset/spotty-turtles-complain.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix preview issues triggered by pageUrlFormat & trailingSlash options

packages/astro/src/core/dev/template/4xx.ts

@@ -64,9 +64,16 @@ export function subpathNotUsedTemplate(base: string, pathname: string) {
 		statusCode: 404,
 		title: 'Not found',
 		tabTitle: '404: Not Found',
-		body: `
+		body: `<p>In your <code>buildOptions.site</code> you have your base path set to <a href="${base}">${base}</a>. Do you want to go there instead?</p>
-      <p>In your <code>buildOptions.site</code> you have your base path set to <a href="${base}">${base}</a>. Do you want to go there instead?</p>
+<p>Come to our <a href="https://astro.build/chat">Discord</a> if you need help.</p>`,
-      <p>Come to our <a href="https://astro.build/chat">Discord</a> if you need help.</p>
+	});
-    `,
+}
+
+export function notFoundTemplate(pathname: string, message = 'Not found') {
+	return template({
+		pathname,
+		statusCode: 404,
+		title: message,
+		tabTitle: `404: ${message}`,
 	});
 }

packages/astro/src/core/path.ts

@@ -34,3 +34,7 @@ export function prependDotSlash(path: string) {
 
 	return './' + path;
 }
+
+export function trimSlashes(path: string) {
+	return path.replace(/^\/|\/$/g, '');
+}

packages/astro/src/core/preview/index.ts

@@ -1,16 +1,16 @@
 import type { AstroConfig } from '../../@types/astro';
 import type { LogOptions } from '../logger';
+import type { Stats } from 'fs';
 
 import http from 'http';
 import { performance } from 'perf_hooks';
 import send from 'send';
 import { fileURLToPath } from 'url';
+import fs from 'fs';
 import * as msg from '../dev/messages.js';
 import { error, info } from '../logger.js';
-import { subpathNotUsedTemplate, default as template } from '../dev/template/4xx.js';
+import { subpathNotUsedTemplate, notFoundTemplate, default as template } from '../dev/template/4xx.js';
-import { prependForwardSlash } from '../path.js';
+import { appendForwardSlash, trimSlashes } from '../path.js';
-import * as npath from 'path';
-import * as fs from 'fs';
 
 interface PreviewOptions {
 	logging: LogOptions;
@@ -23,100 +23,94 @@ export interface PreviewServer {
 	stop(): Promise<void>;
 }
 
-type SendStreamWithPath = send.SendStream & { path: string };
-
-function removeBase(base: string, pathname: string) {
-	if (base === pathname) {
-		return '/';
-	}
-	let requrl = pathname.substr(base.length);
-	return prependForwardSlash(requrl);
-}
-
 /** The primary dev action */
 export default async function preview(config: AstroConfig, { logging }: PreviewOptions): Promise<PreviewServer> {
 	const startServerTime = performance.now();
-	const base = config.buildOptions.site ? new URL(config.buildOptions.site).pathname : '/';
+	const pageUrlFormat = config.buildOptions.pageUrlFormat;
+	const trailingSlash = config.devOptions.trailingSlash;
+	const forceTrailingSlash = trailingSlash === 'always';
+	const blockTrailingSlash = trailingSlash === 'never';
+
+	/** Default file served from a directory. */
+	const defaultFile = 'index.html';
+
+	const defaultOrigin = 'http://localhost';
+
+	const sendOptions = {
+		extensions: pageUrlFormat === 'file' ? ['html'] : false,
+		index: false,
+		root: fileURLToPath(config.dist),
+	};
+
+	/** Base request URL. */
+	let baseURL = new URL(appendForwardSlash(config.buildOptions.site || ''), defaultOrigin);
 
 	// Create the preview server, send static files out of the `dist/` directory.
 	const server = http.createServer((req, res) => {
-		if (!req.url!.startsWith(base)) {
+		const requestURL = new URL(req.url as string, defaultOrigin);
+
+		// respond 404 to requests outside the base request directory
+		if (!requestURL.pathname.startsWith(baseURL.pathname)) {
 			res.statusCode = 404;
-			res.end(subpathNotUsedTemplate(base, req.url!));
+			res.end(subpathNotUsedTemplate(baseURL.pathname, requestURL.pathname));
 			return;
 		}
 
-		switch (config.devOptions.trailingSlash) {
+		/** Relative request path. */
-			case 'always': {
+		const pathname = requestURL.pathname.slice(baseURL.pathname.length - 1);
-				if (!req.url?.endsWith('/')) {
-					res.statusCode = 404;
-					res.end(
-						template({
-							title: 'Not found',
-							tabTitle: 'Not found',
-							pathname: req.url!,
-						})
-					);
-					return;
-				}
-				break;
-			}
-			case 'never': {
-				if (req.url?.endsWith('/')) {
-					res.statusCode = 404;
-					res.end(
-						template({
-							title: 'Not found',
-							tabTitle: 'Not found',
-							pathname: req.url!,
-						})
-					);
-					return;
-				}
-				break;
-			}
-			case 'ignore': {
-				break;
-			}
-		}
 
-		let sendpath = removeBase(base, req.url!);
+		const isRoot = pathname === '/';
-		const sendOptions: send.SendOptions = {
+		const hasTrailingSlash = isRoot || pathname.endsWith('/');
-			root: fileURLToPath(config.dist),
+
+		let tryTrailingSlash = true;
+		let tryHtmlExtension = true;
+
+		let url: URL;
+
+		const onErr = (message: string) => {
+			res.statusCode = 404;
+			res.end(notFoundTemplate(pathname, message));
 		};
-		if (config.buildOptions.pageUrlFormat === 'file' && !sendpath.endsWith('.html')) {
+
-			sendOptions.index = false;
+		const onStat = (err: NodeJS.ErrnoException | null, stat: Stats) => {
-			const parts = sendpath.split('/');
+			switch (true) {
-			let lastPart = parts.pop();
+				// retry nonexistent paths without an html extension
-			switch (config.devOptions.trailingSlash) {
+				case err && tryHtmlExtension && hasTrailingSlash && !blockTrailingSlash:
-				case 'always': {
+				case err && tryHtmlExtension && !hasTrailingSlash && !forceTrailingSlash && !pathname.endsWith('.html'):
-					lastPart = parts.pop();
+					tryHtmlExtension = false;
-					break;
+					return fs.stat((url = new URL(url.pathname + '.html', url)), onStat);
+
+				// 404 on nonexistent paths (that are yet handled)
+				case err !== null:
+					return onErr('Path not found');
+
+				// 404 on directories when a trailing slash is present but blocked
+				case stat.isDirectory() && hasTrailingSlash && blockTrailingSlash && !isRoot:
+					return onErr('Prohibited trailing slash');
+
+				// 404 on directories when a trailing slash is missing but forced
+				case stat.isDirectory() && !hasTrailingSlash && forceTrailingSlash && !isRoot:
+					return onErr('Required trailing slash');
+
+				// retry on directories when a default file is missing but allowed (that are yet handled)
+				case stat.isDirectory() && tryTrailingSlash:
+					tryTrailingSlash = false;
+					return fs.stat((url = new URL(url.pathname + (url.pathname.endsWith('/') ? defaultFile : '/' + defaultFile), url)), onStat);
+
+				// 404 on existent directories (that are yet handled)
+				case stat.isDirectory():
+					return onErr('Path not found');
+
+				// handle existent paths
+				default:
+					send(req, fileURLToPath(url), {
+						extensions: false,
+						index: false,
+					}).pipe(res);
 			}
-				case 'never': {
+		};
-					// lastPart is the actually last part like `page`
+
-					break;
+		fs.stat((url = new URL(trimSlashes(pathname), config.dist)), onStat);
-				}
-				case 'ignore': {
-					// this could end in slash, so resolve either way
-					if (lastPart === '') {
-						lastPart = parts.pop();
-					}
-					break;
-				}
-			}
-			const part = lastPart || 'index';
-			sendpath = npath.sep + npath.join(...parts, `${part}.html`);
-		}
-		send(req, sendpath, sendOptions)
-			.once('directory', function (this: SendStreamWithPath, _res, path) {
-				if (config.buildOptions.pageUrlFormat === 'directory' && !path.endsWith('index.html')) {
-					return this.sendIndex(path);
-				} else {
-					this.error(404);
-				}
-			})
-			.pipe(res);
 	});
 
 	let { hostname, port } = config.devOptions;
@@ -132,7 +126,7 @@ export default async function preview(config: AstroConfig, { logging }: PreviewO
 				httpServer = server.listen(port, hostname, () => {
 					if (!showedListenMsg) {
 						info(logging, 'astro', msg.devStart({ startupTime: performance.now() - timerStart }));
-						info(logging, 'astro', msg.devHost({ host: `http://${hostname}:${port}${base}` }));
+						info(logging, 'astro', msg.devHost({ host: `http://${hostname}:${port}${baseURL.pathname}` }));
 					}
 					showedListenMsg = true;
 					resolve();

packages/astro/test/preview-routing.test.js

@@ -30,17 +30,17 @@ describe('Preview Routing', () => {
 				expect(response.status).to.equal(404);
 			});
 
-			it('404 when loading subpath root with trailing slash', async () => {
+			it('200 when loading subpath root with trailing slash', async () => {
 				const response = await fixture.fetch('/blog/');
-				expect(response.status).to.equal(404);
-			});
-
-			it('200 when loading subpath root without trailing slash', async () => {
-				const response = await fixture.fetch('/blog');
 				expect(response.status).to.equal(200);
 				expect(response.redirected).to.equal(false);
 			});
 
+			it('404 when loading subpath root without trailing slash', async () => {
+				const response = await fixture.fetch('/blog');
+				expect(response.status).to.equal(404);
+			});
+
 			it('404 when loading another page with subpath used', async () => {
 				const response = await fixture.fetch('/blog/another/');
 				expect(response.status).to.equal(404);
@@ -92,7 +92,6 @@ describe('Preview Routing', () => {
 			it('404 when loading subpath root without trailing slash', async () => {
 				const response = await fixture.fetch('/blog');
 				expect(response.status).to.equal(404);
-				expect(response.redirected).to.equal(false);
 			});
 
 			it('200 when loading another page with subpath used', async () => {
@@ -148,10 +147,9 @@ describe('Preview Routing', () => {
 				expect(response.status).to.equal(200);
 			});
 
-			it('200 when loading subpath root without trailing slash', async () => {
+			it('404 when loading subpath root without trailing slash', async () => {
 				const response = await fixture.fetch('/blog');
-				expect(response.status).to.equal(200);
+				expect(response.status).to.equal(404);
-				expect(response.redirected).to.equal(false);
 			});
 
 			it('200 when loading another page with subpath used', async () => {
@@ -207,17 +205,17 @@ describe('Preview Routing', () => {
 				expect(response.status).to.equal(404);
 			});
 
-			it('404 when loading subpath root with trailing slash', async () => {
+			it('200 when loading subpath root with trailing slash', async () => {
 				const response = await fixture.fetch('/blog/');
-				expect(response.status).to.equal(404);
-			});
-
-			it('200 when loading subpath root without trailing slash', async () => {
-				const response = await fixture.fetch('/blog');
 				expect(response.status).to.equal(200);
 				expect(response.redirected).to.equal(false);
 			});
 
+			it('404 when loading subpath root without trailing slash', async () => {
+				const response = await fixture.fetch('/blog');
+				expect(response.status).to.equal(404);
+			});
+
 			it('404 when loading another page with subpath used', async () => {
 				const response = await fixture.fetch('/blog/another/');
 				expect(response.status).to.equal(404);
@@ -272,7 +270,6 @@ describe('Preview Routing', () => {
 			it('404 when loading subpath root without trailing slash', async () => {
 				const response = await fixture.fetch('/blog');
 				expect(response.status).to.equal(404);
-				expect(response.redirected).to.equal(false);
 			});
 
 			it('200 when loading another page with subpath used', async () => {
@@ -331,10 +328,9 @@ describe('Preview Routing', () => {
 				expect(response.status).to.equal(200);
 			});
 
-			it('200 when loading subpath root without trailing slash', async () => {
+			it('404 when loading subpath root without trailing slash', async () => {
 				const response = await fixture.fetch('/blog');
-				expect(response.status).to.equal(200);
+				expect(response.status).to.equal(404);
-				expect(response.redirected).to.equal(false);
 			});
 
 			it('200 when loading another page with subpath used', async () => {