-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Adds plugins audit. #122
Adds plugins audit. #122
Conversation
} | ||
|
||
// If there are sandboxed iframes, and there are no objects in the page, this should mean | ||
// the page is good and we can early exit with success. |
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.
Does this mean we don't enforce the check on the page itself?
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 open to change here, but the general idea was that if you've got no iframes, or iframes with sandboxes, and no objects on your page, then you're good. Does that fit what you'd want to check?
Historically, this audit falls into the UX bucket of "Is Mobile Friendly", as it's an indication mobile users will/won't have problems with inaccessible content. So I'd favor this audit falling into the existing aggregator with that title. (though the path ( |
|
||
// Get the network requests and get the one that matches the tested URL. | ||
const pageNetworkRequests = | ||
artifacts.networkRecords.filter(record => record.data._url === artifacts.url); |
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.
use the public getter on record.data.url
Overall I'm having bad feels on this, which is to say it feels like we should have a more accurate "is this page using a plugin?" test. What we have is inference-heavy on CSP and, as I look over the code, I'm not feeling all that great that it does what it says on the tin. |
@paullewis I think this is the right approach. The key thing about CSP is that it sets a policy which disallows use of plugins.. That is, we're not simply auditing a page snapshot, where the page could inject a plugin at a later point. Yes, it would be nice if CSP has a clean API to query this, but in the meantime, I think this works and is a valuable UX check. |
const noObject = /object-src\s+'none'/gim; | ||
|
||
// Get the network requests and get the one that matches the tested URL. | ||
const pageNetworkRequests = |
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.
.find() instead of filter since we are only using the [0]
Closing this for now. It's a tricky audit to get right (see PSI's for all the corner cases they handle) and it's not a top priority for lighthouse detection right now. Ilya, if you want to carry the torch on this, we'd definitely review pull requests. |
For testing against #101; something of a strawman.
PTAL @igrigorik @samccone