Merge pull request #7269 from guardian/mxdvl/fix-dev-server

dotcom-rendering/scripts/test/amp-validation.js

@@ -67,7 +67,7 @@ amphtmlValidator.getInstance().then((validator) => {
 	].map((url) => {
 		// COPIED DIRECTLY FROM https://www.npmjs.com/package/amphtml-validator
 		http.get(
-			`${domain}/AMPArticle/https://www.theguardian.com/${url}`,
+			`${domain}/AMPArticle/https://amp.theguardian.com/${url}`,
 			(res) => {
 				let data = '';
 				res.on('data', function (chunk) {

dotcom-rendering/src/server/dev-server.ts

@@ -16,7 +16,7 @@ const ARTICLE_URL = /\/\d{4}\/[a-z]{3}\/\d{2}\//;
 /** fronts are a series of lowercase strings, dashes and forward slashes */
 const FRONT_URL = /^\/[a-z-/]+/;
 /** assets are paths like /assets/index.xxx.js */
-const ASSETS_URL = /assets\/.*\.js/;
+const ASSETS_URL = /^assets\/.+\.js/;
 
 // see https://www.npmjs.com/package/webpack-hot-server-middleware
 // for more info
@@ -25,10 +25,9 @@ export const devServer = (): Handler => {
 		const path = req.path.split('/')[1];
 
 		// handle urls with the ?url=… query param
-		const url = new URL(req.url, `http://localhost:3030/`);
-		const sourceUrl = url.searchParams.get('url');
-		if (sourceUrl !== null) {
-			return res.redirect(url.pathname + '/' + sourceUrl);
+		const sourceUrl = req.url.split('?url=')[1];
+		if (path && sourceUrl) {
+			return res.redirect(path + '/' + sourceUrl);
 		}
 
 		switch (path) {

dotcom-rendering/src/server/lib/get-content-from-url.js

@@ -10,16 +10,10 @@ const isStringTuple = (_) => typeof _[1] === 'string';
  * Get DCR content from a `theguardian.com` URL.
  * Takes in optional `X-Gu-*` headers to send.
  *
- * @param {string} _url
+ * @param {URL} url
  * @param {import('http').IncomingHttpHeaders} _headers
  */
-async function getContentFromURL(_url, _headers) {
-	if (!_url) {
-		throw new Error('The url query parameter is mandatory');
-	}
-
-	const url = new URL(_url);
-
+async function getContentFromURL(url, _headers) {
 	// searchParams will only work for the first set of query params because 'url' is already a query param itself
 	const searchparams = url.searchParams.toString();
 
@@ -52,20 +46,33 @@ exports.default = getContentFromURL;
 
 /**
  * @param {string} requestUrl
- * @returns {string}
+ * @returns {URL | undefined} the parsed URL, or false if there’s no fully qualified path
  */
 const parseURL = (requestUrl) => {
-	const url = decodeURIComponent(requestUrl.split('/').slice(2).join('/'));
-
-	return requestUrl.startsWith('/AMP') ? url.replace('www', 'amp') : url;
+	try {
+		return new URL(
+			decodeURIComponent(requestUrl.split('/').slice(2).join('/')),
+		);
+	} catch (error) {
+		return undefined;
+	}
 };
 
 exports.parseURL = parseURL;
 
 /** @type {import('webpack-dev-server').ExpressRequestHandler} */
 exports.getContentFromURLMiddleware = async (req, res, next) => {
-	if (req.path.split('/').length > 2) {
-		const sourceURL = parseURL(req.originalUrl);
+	const sourceURL = parseURL(req.originalUrl);
+
+	if (sourceURL) {
+		if (
+			req.path.startsWith('/AMP') &&
+			sourceURL.hostname === 'www.theguardian.com'
+		) {
+			res.redirect(
+				req.path.replace('www.theguardian.com', 'amp.theguardian.com'),
+			);
+		}
 
 		try {
 			req.body = await getContentFromURL(sourceURL, req.headers);

dotcom-rendering/src/server/lib/get-content-from-url.test.ts

@@ -1,7 +1,7 @@
 import { parseURL } from './get-content-from-url';
 
 describe('URL parser', () => {
-	test('parse DEV URL when one query is present', () => {
+	test('parse source URL when one query is present', () => {
 		const req = {
 			path: '/Article/https%3A%2F%2Fwww.theguardian.com%2Fpolitics%2Flive%2F2022%2Fapr%2F13%2Fboris-johnson-uk-politics-live-rishi-sunak-partygate-lockdown?filterKeyEvents=true',
 		};
@@ -10,10 +10,10 @@ describe('URL parser', () => {
 
 		const url = parseURL(req.path);
 
-		expect(url).toEqual(parsedURL);
+		expect(url?.href).toEqual(parsedURL);
 	});
 
-	test('parse DEV URL when multiple queries are present', () => {
+	test('parse source URL when multiple queries are present', () => {
 		const req = {
 			path: '/Article/https%3A%2F%2Fwww.theguardian.com%2Fpolitics%2Flive%2F2022%2Fapr%2F13%2Fboris-johnson-uk-politics-live-rishi-sunak-partygate-lockdown?filterKeyEvents=true&dcr=true&live=true',
 		};
@@ -22,10 +22,10 @@ describe('URL parser', () => {
 
 		const url = parseURL(req.path);
 
-		expect(url).toEqual(parsedURL);
+		expect(url?.href).toEqual(parsedURL);
 	});
 
-	test('parse URL when there are no queries', () => {
+	test('parse source URL when there are no queries', () => {
 		const req = {
 			path: '/Article/https%3A%2F%2Fwww.theguardian.com%2Fpolitics%2Flive%2F2022%2Fapr%2F13%2Fboris-johnson-uk-politics-live-rishi-sunak-partygate-lockdown',
 		};
@@ -34,6 +34,38 @@ describe('URL parser', () => {
 
 		const url = parseURL(req.path);
 
-		expect(url).toEqual(parsedURL);
+		expect(url?.href).toEqual(parsedURL);
+	});
+
+	test('parse source URL for localhost/frontend', () => {
+		const req = {
+			path: '/Article/http://localhost:9000/politics/live/2022/apr/13/boris-johnson-uk-politics-live-rishi-sunak-partygate-lockdown',
+		};
+		const parsedURL =
+			'http://localhost:9000/politics/live/2022/apr/13/boris-johnson-uk-politics-live-rishi-sunak-partygate-lockdown';
+
+		const url = parseURL(req.path);
+
+		expect(url?.href).toEqual(parsedURL);
+	});
+
+	test('parse URL returns undefined for assets', () => {
+		const req = {
+			path: '/assets/something.js',
+		};
+
+		const url = parseURL(req.path);
+
+		expect(url).toBeUndefined();
+	});
+
+	test('parse URL returns undefined for static paths', () => {
+		const req = {
+			path: '/frontend/static/icon.svg',
+		};
+
+		const url = parseURL(req.path);
+
+		expect(url).toBeUndefined();
 	});
 });