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.

Sign up for GitHub

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

Jump to bottom

core(audit): Ignore href=javascript:.* for rel=noopener audit #3574

Merged
merged 6 commits into from
Nov 17, 2017
Merged

core(audit): Ignore href=javascript:.* for rel=noopener audit #3574

merged 6 commits into from
Nov 17, 2017

Conversation

karanjthakkar
Copy link
Contributor

@karanjthakkar karanjthakkar commented Oct 16, 2017 •
edited
Loading

Fixes #3079

PS: Happy to close this if @akonchady has another implementation in place already. #3079 (comment)

@karanjthakkar karanjthakkar force-pushed the fix-noopener-test branch 2 times, most recently from d9b2a37 to d05f19c Compare October 16, 2017 10:12
@karanjthakkar
Copy link
Contributor Author

@patrickhulce I'm not sure how to fix the commitlint error. It doesn't seem to consider rebased commits. Everything else seems to be passing.

@wardpeet wardpeet changed the title fix: Ignore href=javascript:.* for rel=noopener audit misc(external-anchors-use-rel-noopener): Ignore href=javascript:.* for rel=noopener audit Oct 16, 2017
wardpeet
wardpeet reviewed Oct 16, 2017
View reviewed changes
lighthouse-core/audits/dobetterweb/external-anchors-use-rel-noopener.js Outdated
@@ -47,6 +47,10 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
return true;
}
})
.filter(anchor => {
// Ignore href's that do not redirect to a new url
return !/javascript:.*/.test(anchor.href);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't this be better?
return !/^javascript:/.test(anchor.href); just to make sure the string starts with javascript:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wardpeet Thats a good idea. Makes it much more robust. Done!

@patrickhulce patrickhulce changed the title misc(external-anchors-use-rel-noopener): Ignore href=javascript:.* for rel=noopener audit core(audit): Ignore href=javascript:.* for rel=noopener audit Oct 16, 2017
wardpeet
wardpeet approved these changes Oct 16, 2017
View reviewed changes
Copy link
Collaborator

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paulirish paulirish changed the title core(audit): Ignore href=javascript:.* for rel=noopener audit misc(external-anchors-use-rel-noopener): Ignore href=javascript:.* for rel=noopener audit Oct 16, 2017
@paulirish paulirish changed the title misc(external-anchors-use-rel-noopener): Ignore href=javascript:.* for rel=noopener audit misc(external-anchors-use-rel-noopener): Ignore href=javascript:.* for rel=noopener audit. Oct 16, 2017
@paulirish paulirish changed the title misc(external-anchors-use-rel-noopener): Ignore href=javascript:.* for rel=noopener audit. misc(external-anchors-use-rel-noopener): Ignore href=javascript:.* for rel=noopener audit Oct 16, 2017
@paulirish paulirish changed the title misc(external-anchors-use-rel-noopener): Ignore href=javascript:.* for rel=noopener audit core(audit): Ignore href=javascript:.* for rel=noopener audit Oct 16, 2017
paulirish
paulirish requested changes Oct 16, 2017
View reviewed changes
lighthouse-core/audits/dobetterweb/external-anchors-use-rel-noopener.js Outdated
@@ -47,6 +47,10 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
return true;
}
})
.filter(anchor => {
// Ignore href's that do not redirect to a new url
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore href's that are not real links

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

lighthouse-core/audits/dobetterweb/external-anchors-use-rel-noopener.js Outdated
@@ -47,6 +47,10 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
return true;
}
})
.filter(anchor => {
// Ignore href's that do not redirect to a new url
return !/^javascript:/.test(anchor.href);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use startsWith instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@karanjthakkar
Copy link
Contributor Author

@paulirish I was going through #3490 and based on the tests (https://github.com/GoogleChrome/lighthouse/pull/3490/files#diff-8f19451b89a25fa6b1259f337761d2cbR59) I think we should be doing a case insensitive check of javascript:, unless i'm mistaken. Thoughts?

brendankenny
brendankenny reviewed Oct 20, 2017
View reviewed changes
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be doing a case insensitive check of javascript:, unless i'm mistaken. Thoughts?

looks like it is case insensitive, so sounds good

@karanjthakkar
Copy link
Contributor Author

@brendankenny I meant that startsWith does not do a case sensitive check. So in cases where the href is JAVASCRIPT:void(0), even though the browser may accept it (as mentioned in the spec linked by you), this will fail the check that I do here: dd8e472.

@karanjthakkar
Copy link
Contributor Author

Bump.

@wardpeet
Copy link
Collaborator

we could do .toLowerCase().startsWith just to make sure can you add a test that includes
'JAVASCRIPT:' which will fail atm.

@karanjthakkar Chrome Dev Summit just happend so the team had some other priorities. Thanks for being so patient!

@karanjthakkar
Copy link
Contributor Author

@wardpeet So sorry about that. It totally slipped my mind. I appreciate you circling back with a reply. I've made the changes 👍

patrickhulce
patrickhulce approved these changes Oct 26, 2017
View reviewed changes
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm thanks @karanjthakkar!

lighthouse-core/audits/dobetterweb/external-anchors-use-rel-noopener.js Outdated
@@ -47,6 +47,14 @@ class ExternalAnchorsUseRelNoopenerAudit extends Audit {
return true;
}
})
.filter(anchor => {
// Ignore href's that are not real links
return (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could probably simplify this to !anchor.href || !anchor.href.toLowerCase().startsWith('javascript:')

@paulirish paulirish merged commit b67601b into GoogleChrome:master Nov 17, 2017
@dependencies dependencies bot mentioned this pull request Dec 17, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brendankenny brendankenny brendankenny left review comments

@patrickhulce patrickhulce patrickhulce approved these changes

@wardpeet wardpeet wardpeet approved these changes

@paulirish paulirish paulirish approved these changes

Assignees

@paulirish paulirish

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rel=noopener audit should ignore href=javascript:.*
5 participants
@karanjthakkar @wardpeet @paulirish @brendankenny @patrickhulce