fix(@angular/ssr): avoid caching non-SSG page lookups#33365
Conversation
Only cache CommonEngine SSG lookup results after the target file is confirmed to be a prerendered SSG page. Missing pages and static files without the SSG marker can be derived from request URLs, so retaining those negative results allows attacker-controlled paths to grow the process cache without bound.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the CommonEngine class in Angular SSR to optimize how SSG pages are tracked. It removes the logic that explicitly sets pageIsSSG to false when a file does not exist or matches the prerender path, and ensures that pageIsSSG is only set to true when the SSG marker is found in the static file. There are no review comments to evaluate, and I have no further feedback to provide.
| const isSSG = SSG_MARKER_REGEXP.test(content); | ||
| this.pageIsSSG.set(pagePath, isSSG); | ||
| if (isSSG) { | ||
| this.pageIsSSG.set(pagePath, true); |
There was a problem hiding this comment.
As I understand, I don't see any use for it when is false. Additionally, I'm not sure if we would need a test, although I can't think of a way to perform one at the moment.
| this.pageIsSSG.set(pagePath, true); | ||
| } | ||
|
|
||
| return isSSG ? content : undefined; |
There was a problem hiding this comment.
NIT:
| return isSSG ? content : undefined; | |
| return undefined; |
| const isSSG = SSG_MARKER_REGEXP.test(content); | ||
| this.pageIsSSG.set(pagePath, isSSG); | ||
| if (isSSG) { | ||
| this.pageIsSSG.set(pagePath, true); |
There was a problem hiding this comment.
| this.pageIsSSG.set(pagePath, true); | |
| return content; |
There was a problem hiding this comment.
@alan-agius4
Q: The this.pageIsSSG.set(pagePath, true); is no longer necessary? or yes, as I understood it was necessary for this line
if (this.pageIsSSG.get(pagePath)) {
// Serve pre-rendered page.
return fs.promises.readFile(pagePath, 'utf-8');
}There was a problem hiding this comment.
Sorry, I mean return in the if statement not removing the `this.pageIsSSG.set’
There was a problem hiding this comment.
I thought the same thing, I had my doubts, it's already updated in the fixup!
There was a problem hiding this comment.
A somewhat tangential request and sorry for the inconvenience.
I had two issue tracker reports that resulted in security advisories:
- https://issuetracker.google.com/u/1/issues/509941006 ->GHSA-f3m7-gqxr-g87x
- https://issuetracker.google.com/u/1/issues/510537066 -> GHSA-692r-grfm-v8x7
Both issues were closed (infeasible) , but since the corresponding advisories (including credits) were published, would it be possible to review them and update their status to Accepted/Fixed?
2 participants
Only cache CommonEngine SSG lookup results after the target file is confirmed to be a prerendered SSG page.
Missing pages and static files without the SSG marker can be derived from request URLs, so retaining those negative results allows attacker-controlled paths to grow the process cache without bound. Over time, this can lead to unbounded memory growth and a denial of service.