Skip to content
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

Fix tab nabbing vulnerability in formatted links #2674

Open
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
3 participants
@danielw93
Copy link

commented Jul 6, 2019

  • Add attribute rel="noopener noreferrer" to tags created by formats/link.js
  • Make unit tests for links expect rel attribute

The vulnerability described in #2438 was not fully fixed with PR #2439 .
Using quill in readonly mode to display content still results in vulnerable anchor tags, because
the create function in formats/link.js sets target="_blank" but not rel="noopener noreferrer".

This pull request in combination with #2439 mitigates the tab nabbing vulnerability.

Fix tab nabbing vulnerability in formatted links
- Add attribute rel="noopener noreferrer"
  to <a> tags created by formats/link.js
- Make unit tests for links expect rel
  attribute
@Jamesking56

This comment has been minimized.

Copy link

commented Jul 17, 2019

@jhchen can we get this merged and released please?

@@ -4,6 +4,7 @@ class Link extends Inline {
static create(value) {
const node = super.create(value);
node.setAttribute('href', this.sanitize(value));
node.setAttribute('rel', 'noopener noreferrer');

This comment has been minimized.

Copy link
@jl-welch

jl-welch Jul 17, 2019

Is noreferrer required? If I'm correct this was a workaround for FF, which was fixed in FF 52. This will cause issues with analytics reporting where traffic came from

This comment has been minimized.

Copy link
@Jamesking56

Jamesking56 Jul 17, 2019

Hmm 🤔 I think it would be best as a config option to set that

This comment has been minimized.

Copy link
@jl-welch

jl-welch Jul 17, 2019

Would using a property such as Link.REL = "noopener"; be efficient?

This comment has been minimized.

Copy link
@danielw93

danielw93 Jul 17, 2019

Author

According to caniuse, the rel=noopener attribute is not supported by IE11, with rel=noreferrer at least being supported in Win 10 creators update versions of IE11. That's why I picked both attributes here.

This will cause issues with analytics reporting where traffic came from

Personally I would consider this a feature. But I could understand if you didn't want to make this decision as part of your library.

This comment has been minimized.

Copy link
@jl-welch

jl-welch Jul 17, 2019

Ah my apologies, I should have checked IE support! This is good to go then. Thanks 😃

This comment has been minimized.

Copy link
@danielw93

danielw93 Jul 17, 2019

Author

Awesome :)
Please note though, that for a release that fixes the complete vulnerability, this commit as well as #2439 have to be included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.