search

thieman / github-selfies

Everything is better with selfies. Yes, even pull requests. Trust me.
MIT License
1.14k stars 76 forks source link

Feedback from Mozilla reviewer #70

Open thieman opened 8 years ago

thieman commented 8 years ago

1) Remove data/background.js. This file is only relevant for a Chrome extension.

2) data/selfie.js: When using the message event, always verify that the source is okay, otherwise you may accidentally expose your add-ons functionality to arbitrary pages. See https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage For same-page communications, I recommend CustomEvent over postMessage, because the above issue will not occur, and your message event will not conflict with other (badly written) add-ons that do not expect your message events. See https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Creating_and_triggering_events and https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent

3) Overwriting pushState / replaceState and modifying the CSP is nasty. Try to avoid it if possible. There are several alternatives, e.g. usinga MutationObserver to watch an element for changes. If you really want to overwrite history.pushState, then use exportFunction, see https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts/Interacting_with_page_scripts#Expose_functions_to_page_scripts and https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.exportFunction

bhollis commented 8 years ago

OK. I'll have to see whether I can force myself to care enough about Firefox to fix those...

bhollis commented 8 years ago

Part of the problem is me wanting to have the same code on Chrome and FF.

thieman commented 8 years ago

OK. I'll have to see whether I can force myself to care enough about Firefox to fix those...

bhollis commented 8 years ago

I removed background.js. Haven't done the rest yet.