-
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
core(third-party-summary): check main domain against all third party domains #10006
core(third-party-summary): check main domain against all third party domains #10006
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
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.
thanks very much @micdijkstra! excited to get this in :)
- Use computed `MainResource` - Filter third party entries if main domain matches any third party domain - Update tests to use `networkRecordsToDevtoolsLog`
@patrickhulce This has been updated to reflect your requested changes. I didn't want to add the mainResource to |
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.
LGTM % test change
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.
hang on you'll need to add URL
to the requiredArtifacts
to fix the test issue too :)
@patrickhulce I've updated the test and included the comment.
I was going to ask about that test, thanks for the tip! Looks like it's green ✅ |
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.
LGTM, thanks so much @micdijkstra!
Summary
This PR fixes a bug where if you run an audit on a domain that's in third-party-web it records all scripts from that domain as a third party.
For example running an audit against the url
my-pr-3.netlify.com
will record all scripts retrieved frommy-pr-3.netlify.com
against the third partyNetlify
, even though they're first party scripts.The change intended to fix this issue is to compare the domain of the main resource url against all request urls and skip the ones that match.