-
Notifications
You must be signed in to change notification settings - Fork 30
Add a new homepage to the dev server #14338
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.
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
base: main
Are you sure you want to change the base?
Conversation
It contains a form to input a dotcom URL. Submitting the form will query frontend for information about that page, and then present a summary of this to the user. Included in that summary are links for opening the dotcom and apps versions of that page locally, in CODE, and on PROD. This supports only a single article URL for now, and the summary provides information on the article format, its main media, and the body elements it contains. Support for more kinds of pages (fronts for example), and viewing information about multiple pages at once, will come in a future update. As the new page does not yet have feature parity with the old homepage, it does not replace it for now, and is instead available on `/home`.
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new homepage to the dev server that allows developers to analyze Guardian articles by inputting a dotcom URL. The form queries the frontend for article information and displays a comprehensive summary including article format, main media, body elements, and convenient links to view the page across different environments (DEV, CODE, PROD) and platforms (Apps, Web).
- Adds a new
/home
route with a URL input form for article analysis - Creates article information display component with format, media, and body element summaries
- Implements environment-specific links for easy navigation between DEV, CODE, and PROD stages
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
dotcom-rendering/src/server/server.dev.ts | Adds the new home router to the dev server routing configuration |
dotcom-rendering/src/devServer/routers/home.tsx | Implements the backend logic for URL parsing, article fetching, and request handling |
dotcom-rendering/src/devServer/docs/home.tsx | Creates the React components for the homepage UI including form, article display, and environment links |
url.pathname = `${url.pathname}.json`; | ||
url.searchParams.append('dcr', 'true'); | ||
|
||
const json = await fetch(url).then((res) => res.json()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fetch request lacks error handling. If the request fails or returns a non-200 status, the subsequent .json() call will fail without providing a helpful error message to the user.
const json = await fetch(url).then((res) => res.json()); | |
let json; | |
try { | |
const res = await fetch(url); | |
if (!res.ok) { | |
throw new Error(`Failed to fetch article: ${res.status} ${res.statusText}`); | |
} | |
json = await res.json(); | |
} catch (err) { | |
console.error('Error fetching article:', err); | |
throw err; | |
} |
Copilot uses AI. Check for mistakes.
|
||
const elementToString = (element: FEElement): string => { | ||
const role = 'role' in element ? ` (${element.role})` : ''; | ||
const elem = element._type.split('.').at(-1)?.replace('BlockElement', ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using optional chaining with .at(-1) but not handling the undefined case. If split() returns an empty array, .at(-1) returns undefined, and calling .replace() on undefined will throw an error.
const elem = element._type.split('.').at(-1)?.replace('BlockElement', ''); | |
const elem = (element._type.split('.').at(-1) ?? '').replace('BlockElement', ''); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is incorrect for a couple of reasons:
split
will never return an empty array in this case, asseparator
is not an empty string and thelimit
argument has not been passed1.- Even if it did, an error would not be thrown because optional chaining is used; therefore the expression will short-circuit to
undefined
(not helpful perhaps, but not an error).
Footnotes
url.pathname = `${url.pathname}.json`; | ||
url.searchParams.append('dcr', 'true'); | ||
|
||
const json = await fetch(url).then((res) => res.json()); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 14 days ago
To fix the SSRF vulnerability, we should further restrict the user input so that only a safe, known set of paths can be requested from www.theguardian.com
. The best way is to maintain an allow-list of permitted paths (e.g., specific article paths or path patterns), or at minimum, validate that the path matches an expected format (such as /[section]/[article-slug]
). This validation should be performed in the parseUrl
function, before the URL is returned and used in the fetch. If the path does not match the expected pattern, the request should be rejected.
Steps:
- In
parseUrl
, after checking the host and protocol, add a check that the path matches a safe pattern (e.g.,/[a-z0-9-]+(/[a-z0-9-]+)*
). - Reject any path that contains suspicious elements (such as
..
, double slashes, or does not match the expected article path format). - Optionally, restrict or sanitize the query string as well, or disallow it entirely if not needed.
Required changes:
- Edit
parseUrl
indotcom-rendering/src/devServer/routers/home.tsx
to add path validation. - No new imports are needed; use a regular expression for path validation.
-
Copy modified lines R51-R61
@@ -50,2 +50,13 @@ | ||
|
||
// Restrict path to expected article format: e.g., /section/article-slug | ||
// Disallow suspicious paths (e.g., path traversal, double slashes, etc.) | ||
const pathPattern = /^\/[a-z0-9-]+(\/[a-z0-9-]+)*$/i; | ||
if (!pathPattern.test(url.pathname)) { | ||
return error({ | ||
kind: 'parseError', | ||
message: | ||
"'dotcomURL' path must match expected article format (e.g., /section/article-slug) and not contain suspicious characters.", | ||
}); | ||
} | ||
|
||
return ok(url); |
It contains a form to input a dotcom URL. Submitting the form will query frontend for information about that page, and then present a summary of this to the user. Included in that summary are links for opening the dotcom and apps versions of that page locally, in CODE, and on PROD.
This supports only a single article URL for now, and the summary provides information on the article format, its main media, and the body elements it contains. Support for more kinds of pages (fronts for example), and viewing information about multiple pages at once, will come in a future update.
As the new page does not yet have feature parity with the old homepage, it does not replace it for now, and is instead available on
/home
.Part of #13737.
Screenshots