fix(@angular/ssr): disallow x-forwarded-prefix starting with a backslash#32771
fix(@angular/ssr): disallow x-forwarded-prefix starting with a backslash#32771alan-agius4 wants to merge 2 commits intoangular:mainfrom
Conversation
alan-agius4
added
action: review
![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 correctly addresses a validation gap by disallowing x-forwarded-prefix headers that start with a backslash. The regular expression has been updated effectively, and a corresponding test case has been added to cover this scenario. My review includes a few suggestions to improve the clarity of the error message and a related test description to ensure they accurately reflect all invalid prefix conditions being checked.
Updated the INVALID_PREFIX_REGEX to ensure that prefixes starting with a backslash are considered invalid. Previously, only multiple slashes or dot segments were explicitly disallowed at the start. Also updated the associated validation error message and unit tests to reflect this change.
alan-agius4
force-pushed
the
prefix-url
branch
2 times, most recently
from
c0bb10a to
2210255
Compare
Updates createRedirectResponse to accept an optional Record<string, string> of headers, allowing custom headers to be merged into the redirect response. The Location and Vary: X-Forwarded-Prefix headers are automatically set to ensure correct routing and proxy behavior. AngularServerApp now passes relevant headers from the matched route or response context when creating a redirect.
| * Regular expression to validate that the prefix is valid. | ||
| */ | ||
| const INVALID_PREFIX_REGEX = /^[/\\]{2}|(?:^|[/\\])\.\.?(?:[/\\]|$)/; | ||
| const INVALID_PREFIX_REGEX = /^\\|^\/[/\\]|(?:^|[/\\])\.\.?(?:[/\\]|$)/; |
There was a problem hiding this comment.
Nit: Parens might make the order of operations clearer, /(^\\|^\/)[/\\].../
| headers: { | ||
| ...headers, | ||
| 'Location': location, | ||
| 'Vary': 'X-Forwarded-Prefix', |
There was a problem hiding this comment.
Consider: What if someone tries to set Vary: foo, seems like we'd override with our Vary: X-Forwarded-Prefix. Should we merge that to Vary: X-Forwarded-Prefix, foo?
| status, | ||
| headers: { | ||
| ...headers, | ||
| 'Location': location, |
There was a problem hiding this comment.
Nit: Should we warn or error (probably limited to ngDevMode) if the user specifies a Location header given that we'll ignore it?
2 participants
Updated the INVALID_PREFIX_REGEX to ensure that prefixes starting with a backslash are considered invalid. Previously, only multiple slashes or dot segments were explicitly disallowed at the start.
Also updated the associated validation error message and unit tests to reflect this change.