fix(@angular/ssr): decode x-forwarded-prefix before validation#32906
fix(@angular/ssr): decode x-forwarded-prefix before validation#32906alan-agius4 wants to merge 1 commit intoangular:20.3.xfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the security of the x-forwarded-prefix header validation by decoding the header value before applying regex checks, preventing potential bypasses via URL encoding. It also introduces new test cases to verify the handling of encoded malicious prefixes and malformed URI components. A review comment correctly identified a function signature mismatch in the newly added tests where validateRequest was called with an extraneous third argument.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the validation of the "x-forwarded-prefix" header by decoding the value before checking it against invalid path patterns, ensuring that encoded malicious paths are correctly identified. It also includes updated test cases for encoded prefixes and malformed URI components. A security improvement was suggested to trim the decoded prefix to prevent potential bypasses using encoded whitespace characters.
|
/gemini review |
The `x-forwarded-prefix` header can be percent-encoded. Validating it without decoding can allow bypassing security checks if subsequent processors (such as the `URL` constructor or a browser) implicitly decode it. Key bypass scenarios addressed: - **Implicit Decoding by URL Parsers**: A regex check for a literal `..` might miss `%2e%2e`. However, if the prefix is later passed to a `URL` constructor, it will treat `%2e%2e` as `..`, climbing up a directory. - **Browser Role in Redirects**: If an un-decoded encoded path is sent in a `Location` header, the browser will decode it, leading to unintended navigation. - **Double Slash Bypass**: Checking for a literal `//` misses `%2f%2f`. URL parsers might treat leading double slashes as protocol-relative URLs, leading to Open Redirects if interpreted as a hostname. This change ensures the validation "speaks the same language" as the URL parsing system by decoding the prefix before running safety checks. It also introduces robust handling for malformed percent-encoding.
There was a problem hiding this comment.
Code Review
This pull request enhances the validation of the x-forwarded-prefix header in the Angular SSR utility by decoding and trimming the header value before performing security checks. It also introduces error handling for malformed URI components and expands the test suite to include encoded malicious paths and invalid encoding scenarios. I have no feedback to provide.
The
x-forwarded-prefixheader can be percent-encoded. Validating it without decoding can allow bypassing security checks if subsequent processors (such as theURLconstructor or a browser) implicitly decode it.Key bypass scenarios addressed:
..might miss%2e%2e. However, if the prefix is later passed to aURLconstructor, it will treat%2e%2eas.., climbing up a directory.Locationheader, the browser will decode it, leading to unintended navigation.//misses%2f%2f. URL parsers might treat leading double slashes as protocol-relative URLs, leading to Open Redirects if interpreted as a hostname.This change ensures the validation "speaks the same language" as the URL parsing system by decoding the prefix before running safety checks. It also introduces robust handling for malformed percent-encoding.