Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[HtmlSanitizer] Add support for securing links#60539
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
Introduce `ensureSafeBlankTarget` to automatically add `rel="noopener noreferrer"` to `<a>` elements with `target="_blank"`, mitigating reverse tabnabbing risks. The `allowUnsafeBlankTargets` method allows opting out of this behavior if needed. Included tests validate the new functionality.
carsonbot commentedMay 25, 2025
Hey! Thanks for your PR. You are targeting branch "7.3" but it seems your PR description refers to branch "7.4". Cheers! Carsonbot |
/cc@tgalopin can you please check this 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.
I'm mixed on this one:browsers already implement the mitigation.
When people will upgrade to Symfony 7.4, even more people will have upgraded to a recent enough browser.
To me, this feels like a niche and dying concern. People that care should configure the sanitizer on their own so that we can keep the code simpler IMHO.
| @@ -58,10 +58,7 @@ public function getAttribute(string $name): ?string | |||
| public function setAttribute(string $name, ?string $value): void | |||
| { | |||
| // Always use only the first declaration (ease sanitization) | |||
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'd like we really understand what this means before removing.
I agree this isn't really needed, considering doing this only requires this (or similar): Of course this will apply to all links and not just target=_blank ones, but I think this is probably safer. Why should other targets not have the noopener flag? They open in a new window too, so IMO this PR introduces a vulnerability. |
OK noted. Many thanks for your feedback. |
Uh oh!
There was an error while loading.Please reload this page.
Introduce
ensureSafeBlankTargetto automatically addrel="noopener noreferrer"to<a>elements withtarget="_blank", mitigating reverse tabnabbing risks.The
allowUnsafeBlankTargetsmethod allows opting out of this behavior if needed.ℹ️ Info: Modern browsers already consider
noopenerevent if missing. However the presence of therelattribute is still considered as [a good practice](https://cheatsheetseries.owasp.org/cheatsheets/HTML5_Security_Cheat_Sheet.html#tabnabbing) by the OWASP.
⚠ Warning: I modified
Node::setAttributeto allow overwriting existing attributes. I might have missed the reason for the commentAlways use only the first declaration (ease sanitization).⚠ Warning: The logic is implemented in
DomVisitor. It works, however I am wondering if an abstraction is needed.