fix(assets): Solidify Node endpoint (#10284) (#10287)

* fix(assets): Solidify Node endpoint (#10284) * fix(assets): Solidify Node endpoint * chore: changeset * fix merge * fix test --------- Co-authored-by: Erika <[email protected]>
withastro · Mar 1, 2024 · a90d685 · a90d685
1 parent 5470b80
commit a90d685
Show file tree

Hide file tree

Showing 4 changed files with 144 additions and 12 deletions.
diff --git a/.changeset/soft-boxes-allow.md b/.changeset/soft-boxes-allow.md
@@ -0,0 +1,7 @@
+---
+"astro": patch
+---
+
+Fixes an issue where in Node SSR, the image endpoint could be used maliciously to reveal unintended information about the underlying system.
+
+Thanks to Google Security Team for reporting this issue.
diff --git a/packages/astro/src/assets/endpoint/node.ts b/packages/astro/src/assets/endpoint/node.ts
@@ -1,28 +1,52 @@
+/* eslint-disable no-console */
+import os from 'node:os';
+import { isAbsolute } from 'node:path';
+import { fileURLToPath, pathToFileURL } from 'node:url';
 import { isRemotePath, removeQueryString } from '@astrojs/internal-helpers/path';
 import { readFile } from 'fs/promises';
 import mime from 'mime/lite.js';
-import os from 'os';
 import type { APIRoute } from '../../@types/astro.js';
 import { getConfiguredImageService, isRemoteAllowed } from '../internal.js';
 import { etag } from '../utils/etag.js';
 // @ts-expect-error
-import { assetsDir, imageConfig } from 'astro:assets';
+import { assetsDir, outDir, imageConfig } from 'astro:assets';
 
 function replaceFileSystemReferences(src: string) {
 	return os.platform().includes('win32') ? src.replace(/^\/@fs\//, '') : src.replace(/^\/@fs/, '');
 }
 
 async function loadLocalImage(src: string, url: URL) {
-	const filePath = import.meta.env.DEV
-		? removeQueryString(replaceFileSystemReferences(src))
-		: new URL('.' + src, assetsDir);
+	const assetsDirPath = fileURLToPath(assetsDir);
+
+	let fileUrl;
+	if (import.meta.env.DEV) {
+		fileUrl = pathToFileURL(removeQueryString(replaceFileSystemReferences(src)));
+	} else {
+		try {
+			fileUrl = new URL('.' + src, outDir);
+			const filePath = fileURLToPath(fileUrl);
+
+			if (!isAbsolute(filePath) || !filePath.startsWith(assetsDirPath)) {
+				return undefined;
+			}
+		} catch (err: unknown) {
+			return undefined;
+		}
+	}
+
 	let buffer: Buffer | undefined = undefined;
 
 	try {
-		buffer = await readFile(filePath);
+		buffer = await readFile(fileUrl);
 	} catch (e) {
-		const sourceUrl = new URL(src, url.origin);
-		buffer = await loadRemoteImage(sourceUrl);
+		// Fallback to try to load the file using `fetch`
+		try {
+			const sourceUrl = new URL(src, url.origin);
+			buffer = await loadRemoteImage(sourceUrl);
+		} catch (err: unknown) {
+			console.error('Could not process image request:', err);
+			return undefined;
+		}
 	}
 
 	return buffer;
@@ -57,7 +81,11 @@ export const GET: APIRoute = async ({ request }) => {
 		const transform = await imageService.parseURL(url, imageConfig);
 
 		if (!transform?.src) {
-			throw new Error('Incorrect transform returned by `parseURL`');
+			const err = new Error(
+				'Incorrect transform returned by `parseURL`. Expected a transform with a `src` property.'
+			);
+			console.error('Could not parse image transform from URL:', err);
+			return new Response('Internal Server Error', { status: 500 });
 		}
 
 		let inputBuffer: Buffer | undefined = undefined;
@@ -73,7 +101,7 @@ export const GET: APIRoute = async ({ request }) => {
 		}
 
 		if (!inputBuffer) {
-			return new Response('Not Found', { status: 404 });
+			return new Response('Internal Server Error', { status: 500 });
 		}
 
 		const { data, format } = await imageService.transform(inputBuffer, transform, imageConfig);
@@ -88,6 +116,12 @@ export const GET: APIRoute = async ({ request }) => {
 			},
 		});
 	} catch (err: unknown) {
-		return new Response(`Server Error: ${err}`, { status: 500 });
+		console.error('Could not process image request:', err);
+		return new Response(
+			import.meta.env.DEV ? `Could not process image request: ${err}` : `Internal Server Error`,
+			{
+				status: 500,
+			}
+		);
 	}
 };
diff --git a/packages/astro/src/assets/vite-plugin-assets.ts b/packages/astro/src/assets/vite-plugin-assets.ts
@@ -65,13 +65,14 @@ export default function assets({
 					export { default as Picture } from "astro/components/Picture.astro";
 
 					export const imageConfig = ${JSON.stringify(settings.config.image)};
-					export const assetsDir = new URL(${JSON.stringify(
+					export const outDir = new URL(${JSON.stringify(
 						new URL(
 							isServerLikeOutput(settings.config)
 								? settings.config.build.client
 								: settings.config.outDir
 						)
 					)});
+					export const assetsDir = new URL(${JSON.stringify(settings.config.build.assets)}, outDir);
 					export const getImage = async (options) => await getImageInternal(options, imageConfig);
 				`;
 				}

diff --git a/packages/astro/test/core-image.test.js b/packages/astro/test/core-image.test.js
@@ -1023,6 +1023,96 @@ describe('astro:image', () => {
 			expect(response.status).to.equal(200);
 		});
 
+		it('endpoint handle malformed requests', async () => {
+			const badPaths = [
+				'../../../../../../../../../../../../etc/hosts%00',
+				'../../../../../../../../../../../../etc/hosts',
+				'../../boot.ini',
+				'/../../../../../../../../%2A',
+				'../../../../../../../../../../../../etc/passwd%00',
+				'../../../../../../../../../../../../etc/passwd',
+				'../../../../../../../../../../../../etc/shadow%00',
+				'../../../../../../../../../../../../etc/shadow',
+				'/../../../../../../../../../../etc/passwd^^',
+				'/../../../../../../../../../../etc/shadow^^',
+				'/../../../../../../../../../../etc/passwd',
+				'/../../../../../../../../../../etc/shadow',
+				'/./././././././././././etc/passwd',
+				'/./././././././././././etc/shadow',
+				'....................etcpasswd',
+				'....................etcshadow',
+				'....................etcpasswd',
+				'....................etcshadow',
+				'/..../..../..../..../..../..../etc/passwd',
+				'/..../..../..../..../..../..../etc/shadow',
+				'.\\./.\\./.\\./.\\./.\\./.\\./etc/passwd',
+				'.\\./.\\./.\\./.\\./.\\./.\\./etc/shadow',
+				'....................etcpasswd%00',
+				'....................etcshadow%00',
+				'....................etcpasswd%00',
+				'....................etcshadow%00',
+				'%0a/bin/cat%20/etc/passwd',
+				'%0a/bin/cat%20/etc/shadow',
+				'%00/etc/passwd%00',
+				'%00/etc/shadow%00',
+				'%00../../../../../../etc/passwd',
+				'%00../../../../../../etc/shadow',
+				'/../../../../../../../../../../../etc/passwd%00.jpg',
+				'/../../../../../../../../../../../etc/passwd%00.html',
+				'/..%c0%af../..%c0%af../..%c0%af../..%c0%af../..%c0%af../..%c0%af../etc/passwd',
+				'/..%c0%af../..%c0%af../..%c0%af../..%c0%af../..%c0%af../..%c0%af../etc/shadow',
+				'/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/etc/passwd,',
+				'/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/etc/shadow,',
+				'%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%,25%5c..%25%5c..%25%5c..%25%5c..%00',
+				'/%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..,%25%5c..%25%5c..%25%5c..%25%5c..%00',
+				'%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%,25%5c..%25%5c..%	25%5c..%25%5c..%00',
+				'%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%,25%5c..%25%5c..%		25%5c..%25%5c..%255cboot.ini',
+				'/%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..%25%5c..,%25%5c..%25%5c..%25%5c..%25%5c..winnt/desktop.ini',
+				'\\&apos;/bin/cat%20/etc/passwd\\&apos;',
+				'\\&apos;/bin/cat%20/etc/shadow\\&apos;',
+				'../../../../../../../../conf/server.xml',
+				'/../../../../../../../../bin/id|',
+				'C:/inetpub/wwwroot/global.asa',
+				'C:inetpubwwwrootglobal.asa',
+				'C:/boot.ini',
+				'C:\boot.ini',
+				'../../../../../../../../../../../../localstart.asp%00',
+				'../../../../../../../../../../../../localstart.asp',
+				'../../../../../../../../../../../../boot.ini%00',
+				'../../../../../../../../../../../../boot.ini',
+				'/./././././././././././boot.ini',
+				'/../../../../../../../../../../../boot.ini%00',
+				'/../../../../../../../../../../../boot.ini',
+				'/..../..../..../..../..../..../boot.ini',
+				'/.\\./.\\./.\\./.\\./.\\./.\\./boot.ini',
+				'....................\boot.ini',
+				'....................\boot.ini%00',
+				'....................\boot.ini',
+				'/../../../../../../../../../../../boot.ini%00.html',
+				'/../../../../../../../../../../../boot.ini%00.jpg',
+				'/.../.../.../.../.../	',
+				'..%c0%af../..%c0%af../..%c0%af../..%c0%af../..%c0%af../..%c0%af../boot.ini',
+				'/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/%2e%2e/boot.ini',
+				'../prerender/index.html',
+			];
+
+			const app = await fixture.loadTestAdapterApp();
+
+			for (const path of badPaths) {
+				let request = new Request('http://example.com/_image?href=' + path);
+				let response = await app.render(request);
+				const body = await response.text();
+
+				expect(response.status).to.equal(500);
+				expect(body).includes('Internal Server Error');
+			}
+
+			// Server should still be running
+			let request = new Request('http://example.com/');
+			let response = await app.render(request);
+			expect(response.status).to.equal(200);
+		});
+
 		it('prerendered routes images are built', async () => {
 			const html = await fixture.readFile('/client/prerender/index.html');
 			const $ = cheerio.load(html);