Full portal #2
Full portal #2
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| const redirectUrl = state | ||
| ? `${postLogoutRedirectUri}${postLogoutRedirectUri.includes("?") ? "&" : "?"}state=${encodeURIComponent(state)}` | ||
| : postLogoutRedirectUri; | ||
| res.redirect(redirectUrl); |
Check warning
Code scanning / CodeQL
Server-side URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
The best way to fix this is to make the allowed-redirect check in validateLogoutRequest fully robust against common attacks. Prefix ("wildcard") matching should be made much stricter, forbidding matching on anything except exact host and correct path prefix (e.g., not subdomains), or, ideally, be removed entirely. If wildcard/prefix matching must be supported, it should only allow matching within the same origin/domain, and never across domains.
Specifically, in validateLogoutRequest (in packages/issuer-oidc/src/provider.ts), we must:
- When handling an allowlisted URI that ends with
*, match only if the prefix matches and the result stays within the same origin (scheme + host + port) as the allowed pattern's base. This prevents an attacker from abusing URLs likehttp://trusted.com/*to redirect tohttp://trusted.com.evil.com/or similar. - Optionally, simply disallow
*patterns altogether, and require all allowed URIs to be exact strings. - Use the Node.js WHATWG
URLconstructor to robustly parse and compare scheme/host/port, not just string prefix.
Implement this by parsing both the allowed and provided URIs, comparing origin, and making sure the path check does not allow subdomain "evasion."
All changes are limited to the validateLogoutRequest method in packages/issuer-oidc/src/provider.ts.
| @@ -5,6 +5,7 @@ | ||
| */ | ||
|
|
||
| import * as jose from "jose"; | ||
| // Use global URL if available, else import from 'url' (Node.js < 10 fallback) | ||
| import { | ||
| KeyManager, | ||
| KeyManagerConfig, | ||
| @@ -1440,13 +1441,26 @@ | ||
| // Check if post_logout_redirect_uri is allowed for this RP | ||
| const allowedUris = rp.oidcRPMetaDataOptionsPostLogoutRedirectUris || []; | ||
| const uriAllowed = allowedUris.some((allowed) => { | ||
| // Check exact match or pattern match | ||
| // Check exact match | ||
| if (allowed === post_logout_redirect_uri) { | ||
| return true; | ||
| } | ||
| // Check if it's a prefix match (e.g., http://example.com/* pattern) | ||
| // Strict wildcard/prefix matching: only allow if same origin, and path matches prefix | ||
| if (allowed.endsWith("*")) { | ||
| return post_logout_redirect_uri.startsWith(allowed.slice(0, -1)); | ||
| try { | ||
| const allowedBase = allowed.slice(0, -1); // remove the '*' | ||
| const allowedUrl = new URL(allowedBase); | ||
| const logoutUrl = new URL(post_logout_redirect_uri); | ||
| // Origins (scheme + host + port) must match exactly | ||
| if (allowedUrl.origin !== logoutUrl.origin) { | ||
| return false; | ||
| } | ||
| // Path must start with allowed prefix | ||
| return logoutUrl.pathname.startsWith(allowedUrl.pathname); | ||
| } catch (e) { | ||
| // ignore on parse errors, not allowed | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| }); |
| const redirectUrl = state | ||
| ? `${postLogoutRedirectUri}${postLogoutRedirectUri.includes("?") ? "&" : "?"}state=${encodeURIComponent(state)}` | ||
| : postLogoutRedirectUri; | ||
| res.redirect(redirectUrl); |
Check warning
Code scanning / CodeQL
Server-side URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
To fix the problem without altering existing functionality, we need to strengthen the URI validation logic within the validateLogoutRequest function in provider.ts:
- General fix strategy: Only accept post-logout redirect URIs if they exactly match the allowed URIs listed for the RP/client OR (if pattern matching is absolutely required) validate the URI against a well-defined regular expression or a domain/host match, not just a raw prefix check.
- Detailed fix steps:
- In
validateLogoutRequest(inprovider.ts), change the prefix-match logic (allowed.endsWith("*")) so that it only validates subpaths of the allowed base URI and does not allow anything after the registered path except for legitimate subpaths. - To avoid open redirect vulnerabilities, restrict
*patterns to apply only to paths on a trusted domain, and ensure the input can’t be abused for open redirects:- Parse the candidate URI and check its origin (protocol + host) matches that of the allowed pattern.
- If it uses patterns, use URL parsing to verify the prefix is not trivially bypassable (e.g., ensure only paths, not domains, are matched).
- Provide a utility function for safe matching, e.g.,
isSafeRedirectUri(allowed, uri).
- In
- Files/regions to change: Only the prefix checking portion of
validateLogoutRequestinprovider.ts, since that is where the tainted value is authorized or rejected. - Required: Definition and use of a URI-safe matching function, using only standard library code (
URL).
| @@ -1440,13 +1440,27 @@ | ||
| // Check if post_logout_redirect_uri is allowed for this RP | ||
| const allowedUris = rp.oidcRPMetaDataOptionsPostLogoutRedirectUris || []; | ||
| const uriAllowed = allowedUris.some((allowed) => { | ||
| // Check exact match or pattern match | ||
| // Check exact match | ||
| if (allowed === post_logout_redirect_uri) { | ||
| return true; | ||
| } | ||
| // Check if it's a prefix match (e.g., http://example.com/* pattern) | ||
| // Secure pattern match: Only allow wildcard at the end of the path (not domain!), and only for same origin. | ||
| if (allowed.endsWith("*")) { | ||
| return post_logout_redirect_uri.startsWith(allowed.slice(0, -1)); | ||
| try { | ||
| const allowedBase = allowed.slice(0, -1); | ||
| const inputUrl = new URL(post_logout_redirect_uri); | ||
| const allowedUrl = new URL(allowedBase, inputUrl.origin); | ||
| // Check that origins match exactly, and path starts with allowedBase path | ||
| if ( | ||
| inputUrl.origin === allowedUrl.origin && | ||
| inputUrl.pathname.startsWith(allowedUrl.pathname) | ||
| ) { | ||
| return true; | ||
| } | ||
| } catch { | ||
| // If either is invalid, reject | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| }); |
Add hasOwnProperty check for locationRegexp and validate that locationCondition is a function before calling it. This prevents prototype pollution attacks where vhost could be "constructor" or "__proto__".
| */ | ||
| export function base32Decode(input: string): Buffer { | ||
| // Remove padding and convert to uppercase | ||
| const cleanInput = input.replace(/=+$/, "").toUpperCase(); |
Check failure
Code scanning / CodeQL
Polynomial regular expression used on uncontrolled data High
No description provided.