Make status code check more strict for sitemap plugin (#10779)

adrianlyjak · bholmesdev · florian-lefebvre · web-flow · commit cefeadf0a4a5 · 2024-05-21T13:01:35.000+02:00
Co-authored-by: Ben Holmes &lt;hey@bholmes.dev&gt;
Co-authored-by: Florian Lefebvre &lt;contact@florian-lefebvre.dev&gt;
diff --git a/.changeset/nasty-turtles-obey.md b/.changeset/nasty-turtles-obey.md
@@ -0,0 +1,5 @@
+---
+"@astrojs/sitemap": patch
+---
+
+Fixes false positives for status code routes like `404` and `500` when generating sitemaps.
diff --git a/packages/integrations/sitemap/src/index.ts b/packages/integrations/sitemap/src/index.ts
@@ -47,14 +47,23 @@ const PKG_NAME = '@astrojs/sitemap';
 const OUTFILE = 'sitemap-index.xml';
 const STATUS_CODE_PAGES = new Set(['404', '500']);
 
-function isStatusCodePage(pathname: string): boolean {
-	if (pathname.endsWith('/')) {
-		pathname = pathname.slice(0, -1);
-	}
-	const end = pathname.split('/').pop() ?? '';
-	return STATUS_CODE_PAGES.has(end);
-}
-
+const isStatusCodePage = (locales: string[]) => {
+	const statusPathNames = new Set(
+		locales
+			.flatMap((locale) => [...STATUS_CODE_PAGES].map((page) => `${locale}/${page}`))
+			.concat([...STATUS_CODE_PAGES])
+	);
+
+	return (pathname: string): boolean => {
+		if (pathname.endsWith('/')) {
+			pathname = pathname.slice(0, -1);
+		}
+		if (pathname.startsWith('/')) {
+			pathname = pathname.slice(1);
+		}
+		return statusPathNames.has(pathname);
+	};
+};
 const createPlugin = (options?: SitemapOptions): AstroIntegration => {
 	let config: AstroConfig;
 
@@ -88,9 +97,9 @@ const createPlugin = (options?: SitemapOptions): AstroIntegration => {
 						);
 						return;
 					}
-
+					const shouldIgnoreStatus = isStatusCodePage(Object.keys(opts.i18n?.locales ?? {}));
 					let pageUrls = pages
-						.filter((p) => !isStatusCodePage(p.pathname))
+						.filter((p) => !shouldIgnoreStatus(p.pathname))
 						.map((p) => {
 							if (p.pathname !== '' && !finalSiteUrl.pathname.endsWith('/'))
 								finalSiteUrl.pathname += '/';
@@ -107,7 +116,7 @@ const createPlugin = (options?: SitemapOptions): AstroIntegration => {
 						 * Dynamic URLs have entries with `undefined` pathnames
 						 */
 						if (r.pathname) {
-							if (isStatusCodePage(r.pathname ?? r.route)) return urls;
+							if (shouldIgnoreStatus(r.pathname ?? r.route)) return urls;
 
 							// `finalSiteUrl` may end with a trailing slash
 							// or not because of base paths.
diff --git a/packages/integrations/sitemap/test/fixtures/static/astro.config.mjs b/packages/integrations/sitemap/test/fixtures/static/astro.config.mjs
@@ -2,7 +2,15 @@ import sitemap from '@astrojs/sitemap';
 import { defineConfig } from 'astro/config';
 
 export default defineConfig({
-  integrations: [sitemap()],
+  integrations: [sitemap({
+      i18n: {
+          defaultLocale: 'it',
+          locales: {
+              it: 'it-IT',
+              de: 'de-DE',
+          }
+      }
+  })],
 	site: 'http://example.com',
       redirects: {
         '/redirect': '/'
diff --git a/packages/integrations/sitemap/test/fixtures/static/src/pages/products-by-id/[id].astro b/packages/integrations/sitemap/test/fixtures/static/src/pages/products-by-id/[id].astro
@@ -0,0 +1,11 @@
+---
+export async function getStaticPaths() {
+    return [
+        { params: { id: '404' } },
+        { params: { id: '405' } },
+    ]
+}
+
+const { id } = Astro.params
+---
+<!DOCTYPE html><html> <head><title>Product #{id}</title></head> <body> <h1>Product #404</h1> <p>This is a product that just happens to have an ID of {id}. It is found!</p></body></html>
diff --git a/packages/integrations/sitemap/test/staticPaths.test.js b/packages/integrations/sitemap/test/staticPaths.test.js
@@ -36,6 +36,11 @@ describe('getStaticPaths support', () => {
 		assert.equal(urls.includes('http://example.com/123/'), true);
 	});
 
+	it('includes numerical 404 pages if not for i18n', () => {
+		assert.equal(urls.includes('http://example.com/products-by-id/405/'), true);
+		assert.equal(urls.includes('http://example.com/products-by-id/404/'), true);
+	});
+
 	it('should render the endpoint', async () => {
 		const page = await fixture.readFile('./it/manifest');
 		assert.match(page, /I'm a route in the "it" language./);
