Fix #109: Disable extraction outside of allowlisted sites. #138
Conversation
There was a problem hiding this comment.
R+(WC?)!
Made one suggestion about syncing the allowlist in config with the domains listed in fallback_extraction_selectors.js and asked a couple questions.
Other than that I would recommend:
- Filing a follow-up issue (backlog for now) for your comment:
In the future we should consider making message sending between different contexts more consistent.
| 'www.walmart.com', | ||
| 'mkelly.me', | ||
| 'www.mkelly.me', | ||
| ]), |
There was a problem hiding this comment.
Instead of this being hardcoded in, can we generate this list from fallback_extraction_selectors.js (available after a rebase)?
There was a problem hiding this comment.
Doing that would essentially require fallback extraction support for all sites that we ever support, which isn't really maintainable. Keeping them separate means more typing for sites that are supported, but also allows for sites that we only support via Fathom extraction.
| // being tracked no matter what. | ||
| const url = new URL(document.location.href); | ||
| const allowList = await config.get('extractionAllowlist'); | ||
| const allowAll = allowList.length === 1 && allowList[0] === '*'; |
There was a problem hiding this comment.
I guess you're building this in for post-MVP where we remove the 5 site limit? Why wouldn't we just remove the allowList check and list from config if we were opening up extraction to all sites?
There was a problem hiding this comment.
This is more for non-developers who want to test the add-on out on many sites without hardcoding them into the pref and can't make their own build with the config changed.
|
|
||
| // Only perform extraction on allowlisted sites. Background updates get a | ||
| // pass; we don't want to accidentally freeze updates for products that are | ||
| // being tracked no matter what. |
There was a problem hiding this comment.
I'm confused by this -- if users can only track products on allowlisted sites in the first place, then how would a background update ever load a page that wasn't an allowlisted site?
There was a problem hiding this comment.
If they tracked a product on a webpage that was previously allowed, but is no longer allowed (whether due to an update, testing the demo add-on, or something else). It's a bit of an edge case but could happen between add-on updates.
| * | ||
| * Content scripts cannot access the preference API, and thus cannot use this | ||
| * module to get config values. Use commerce/config/content instead to use | ||
| * message passing to fetch the config values from the background script. |
There was a problem hiding this comment.
I was very confused by the new config folder until I read this.
There was a problem hiding this comment.
Yup, it's kinda confusing.
Osmose
force-pushed
the
restricted-extraction
branch
from
8aaa224 to
6cbb552
Compare
I'm not entirely a fan of how the message passing is working here, but I don't think refactoring it would be wise in this particular PR. In the future we should consider making message sending between different contexts more consistent.