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

[links] rel and target attributes #418

Merged
merged 14 commits into from May 18, 2017
Merged

[links] rel and target attributes #418

merged 14 commits into from May 18, 2017

Conversation

tmorehouse
Copy link
Member

Adds target and rel attributes to all link-based components.

If target === '_blank' and rel is null then default rel to 'noopener'

Addresses #413

@tmorehouse
Copy link
Member Author

Does the computedRel logic look OK to you @pi0 ?

@pi0
Copy link
Member

pi0 commented May 18, 2017

Yep it seems nice :)

@alexsasharegan
Copy link
Member

I think the this.target === '_blank' condition could be eliminated. It doesn't matter which target, you don't want to grant another domain context to your page, right? Or does it not matter because the window will be closed?

@pi0 pi0 merged commit ed412ea into bootstrap-vue:master May 18, 2017
@pi0
Copy link
Member

pi0 commented May 18, 2017

Sorry i was merged fast :)) So you mean adding noopener even to internal links?

@tmorehouse
Copy link
Member Author

I didn't apply the rel/target to breadcrumb as typically breadcrumbs are in-domain (plus it would mean adding rel and target properties to each item in the list of items

@tmorehouse
Copy link
Member Author

The only think I would be timid of, is if someone does want the opener property not set to noopener (which they can override by specifying an empty string for rel).

Just concerned that it could break some user use cases.

@alexsasharegan
Copy link
Member

This would allow for any falsey value to drop the rel attribute, but it would probably break anyone depending on that communication as they will be forced to specify that prop now. But how many people use it that way?

props: {
        target: {
            type: String,
            default: null
        },
        rel: {
            type: String,
            default: 'noopener noreferrer'
        },
},
computed: {
    computedRel() {
        return this.target ? this.rel : null;
    }
}

@tmorehouse
Copy link
Member Author

Maybe it should be up to the page author to specify any values, leaving security up to them?

@alexsasharegan
Copy link
Member

So, I tested the window.opener prop on a link opened in the same window, and it is null. That makes sense because you just blew away the window by navigating to that link.

@alexsasharegan
Copy link
Member

So maybe that means your computedRel is perfect the way it is?

@tmorehouse
Copy link
Member Author

tmorehouse commented May 18, 2017

Note I defaulted target to _self as well in link mixin

@alexsasharegan
Copy link
Member

As it is now, if I use the conventional _blank, I get the noopener. If I specify a rel prop, I get my rel. If I use a non-standard target, I have to specify the rel to get the security. All seems like reasonable to me.

@alexsasharegan
Copy link
Member

Ship it! @pi0, you weren't premature on the merge after all.

@tmorehouse
Copy link
Member Author

tmorehouse commented May 18, 2017

Should nofollow noreferrer also be added (for firefox)? Unless the newer firefox supports noopener?

@alexsasharegan
Copy link
Member

Hmm... that's primarily an SEO concern, right? Now that we have a rel attribute, people have the ability to specify that at least.

@tmorehouse
Copy link
Member Author

@alexsasharegan Oops, I meant noreferrer not nofollow

https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/

Update: FF does not support "noopener" so add this.

rel="noopener noreferrer"

@alexsasharegan
Copy link
Member

I know that article makes it sound necessary, but when I look at the MDN page for an anchor tag, that attribute doesn't seem necessary. It just wipes your url from the referrer header on the request. Not sure how that is a security risk.

@tmorehouse
Copy link
Member Author

True. So we'll leave it as is for now :)

@tmorehouse tmorehouse deleted the link-rel-target branch May 19, 2017 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants