New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Secure manual URL redirection in the frontend #13892
Conversation
|
Hi @brianrodri please assign the required reviewer(s) for this PR. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Unassigning @Radesh-kumar since they have already approved the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Unassigning @srijanreddy98 since they have already approved the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSafeReturnUrl() current implementation still allows redirecting to external websites.
| @@ -128,6 +128,46 @@ export class UtilsService { | |||
| element.offsetHeight < element.scrollHeight); | |||
| } | |||
| } | |||
|
|
|||
| // Determines whether the URL is pointing to a page on the Oppia site. | |||
| getSafeReturnUrl(urlString: string): string { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSafeReturnUrl('//evil.com') returns //evil.com, what may allow redirect to external websites:
new URL('//evil.com')(line 144) throws exception but validation continuesnew URL('//evil.com', document.baseURI);(line 157) succeeds'//evil.com'.charAt(0) !== '/'(line 163) condition is satisfied
Only relative URLs should be considered safe.
If, redirecting to external websites is a requirement, then known host names should be kept in an AllowList and the urlString checked against that list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks! Redirecting to external websites is not a requirement, so we're fine with only trusting relative paths. Do you know of an API that can detect them securely? Or would checking for a second / suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not an Angular expert but would some of the available Router methods do the job e.g. navigateByUrl()?
I see that Angular Router has a parseUrl() method: it may worth a look at.
If I had to do it in plain JavaScript/TypeScript, probably I would go with something simple (please don't use it as is without further testing):
getSafeReturnUrl(urlString: string): string {
const anchor = document.createElement('a');
anchor.href = urlString;
return (anchor.host === window.location.host)? urlString : '/';
}| Input | Output |
|---|---|
//evil.com |
/ |
https://evil.com |
/ |
javascript:alert(1) |
/ |
data:,Hello%2C%20World%21 |
/ |
/login |
/login |
/ |
/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super helpful, thanks Paulo!
I know @srijanreddy98 has been promoting the usage of Angular router. Srijan, do you know of a way to protect against these "fake return URLs" using angular/routers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, I've added more test cases and a simple additional check since the approach you provided doesn't protect us from URL-encoded attacks. For now, we're at least a little more secure (I can't find an exhaustive list of test cases, so I'll consider this resolved since our current coverage seems "good enough").
|
Hi @brianrodri, it looks like some changes were requested on this pull request by @PauloASilva. PTAL. Thanks! |
* cherry-pickable copy of #13892 * add more test cases and security
|
Hi @brianrodri, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Overview
Essential Checklist
Proof that changes are correct
No proof of changes needed because this does not change the UX of the app.
PR Pointers