Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| const returnUrl = createReturnUrl(body, caseId, documentId); | ||
|
|
||
| return res.redirect(returnUrl); |
Check warning
Code scanning / CodeQL
Server-side URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 16 days ago
General approach: continue using createReturnUrl, but harden its validation so that user input cannot meaningfully influence a redirect to an attacker-controlled location. The safest pattern is to treat body.returnUrl as an optional hint that must pass strict validation as a local path; otherwise fall back to a known-good URL. We should also normalize the path, disallow whitespace and control characters, and prevent exploitation via encoded or malformed inputs.
Best concrete fix in this file: enhance createReturnUrl to (1) default returnUrl to an empty string when undefined, (2) treat it as safe only if it is a simple, well-formed, relative path beginning with a single /, does not start with //, contains no whitespace, control characters, or backslashes, and after normalization still starts with /. If it fails any check, we use the fallback URL. We then proceed to append the #row-${documentId} fragment as before. This keeps existing behavior for normal in-app paths (like /cases/123/documents) while tightening security and addressing the CodeQL concern.
Concretely:
- Modify
createReturnUrlinapps/manage/src/app/views/documents/status/controller.ts. - Replace lines 136–144 so that:
returnUrlis normalized to a string.- A small helper check ensures the URL is a safe, normalized, local path.
- The rest of the logic (fallback to
/cases/${caseId}and appending#row-${documentId}) stays the same.
No new imports are strictly necessary; we can use built-in string methods.
| @@ -133,13 +133,24 @@ | ||
| * we were in the list of documents. | ||
| */ | ||
| function createReturnUrl(body: ToggleDocumentBody, caseId: string, documentId: string) { | ||
| const returnUrl = body.returnUrl; | ||
| const rawReturnUrl = body.returnUrl || ''; | ||
| const fallbackUrl = `/cases/${caseId}`; | ||
|
|
||
| // To prevent open redirect, check the presented URL is safe | ||
| const isSafeUrl = returnUrl && returnUrl.startsWith('/') && !returnUrl.startsWith('//'); | ||
| // To prevent open redirect, check the presented URL is a safe, local path | ||
| const hasNoWhitespace = rawReturnUrl !== '' && !/\s/.test(rawReturnUrl); | ||
| const startsWithSingleSlash = rawReturnUrl.startsWith('/') && !rawReturnUrl.startsWith('//'); | ||
|
|
||
| const cleanRedirectUrl = isSafeUrl ? returnUrl.split('#')[0] : fallbackUrl; | ||
| // Basic normalization: strip any fragment and query string before validation | ||
| const [pathOnly] = rawReturnUrl.split('#', 1); | ||
| const [normalizedPath] = pathOnly.split('?', 1); | ||
|
|
||
| const isSafeUrl = | ||
| hasNoWhitespace && | ||
| startsWithSingleSlash && | ||
| !normalizedPath.startsWith('\\') && | ||
| normalizedPath.startsWith('/'); | ||
|
|
||
| const cleanRedirectUrl = isSafeUrl && normalizedPath ? normalizedPath : fallbackUrl; | ||
|
|
||
| return `${cleanRedirectUrl}#row-${documentId}`; | ||
| } |
oscargay
force-pushed
the
feat/HRP-403-read-flagged
branch
from
5465ebc to
58aabfd
Compare
paymonsattar
left a comment
paymonsattar
left a comment
There was a problem hiding this comment.
Looks really good, just a small comment
paymonsattar
left a comment
paymonsattar
left a comment
There was a problem hiding this comment.
Nice! Another one bites the dust
3 participants
Describe your changes
Issue ticket number and link
HRP-403
https://pins-ds.atlassian.net.mcas.ms/browse/HRP-403?McasTsid=20596&McasCtx=4
Implementation