github-selfies icon indicating copy to clipboard operation
github-selfies copied to clipboard

Feedback from Mozilla reviewer

Open thieman opened this issue 10 years ago • 4 comments

  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

thieman avatar Jan 11 '16 00:01 thieman

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

bhollis avatar Jan 11 '16 05:01 bhollis

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

bhollis avatar Jan 11 '16 05:01 bhollis

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

thieman avatar Jan 11 '16 05:01 thieman

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

bhollis avatar Jan 31 '16 03:01 bhollis