Jorge Villalobos [:jorgev] (he/him) Reporter
	Description • 8 years ago

Bug 1295324 requires WebExtensions content scripts to be blocked for certain domains, but does so silently. We need to inform developers that Firefox blocked them intentionally, so they aren't confused about the cause of the problem. We should show a Browser Console message for this.

	Daniel Veditz [:dveditz]
	Comment 1 • 8 years ago

Not a security bug needing hiding by itself, but related to bug 1295324 which is.

	Andy McKay
	Comment 2 • 7 years ago

Can we clear the security flag on this please?

	gupta.shreya1203
	Comment 3 • 7 years ago

Hi, 

I am a beginner. Would like to work on this. Could someone please help me in getting started with it.

	Andrew Swan [:aswan]
	Comment 4 • 7 years ago

Hi, since this bug was marked as a good first bug, the code that needs to be changed moved from javascript to c++.  Are you still interested in working on it?

	gupta.shreya1203
	Comment 5 • 7 years ago

@An(In reply to Andrew Swan [:aswan] from comment #4)
> Hi, since this bug was marked as a good first bug, the code that needs to be
> changed moved from javascript to c++.  Are you still interested in working
> on it?

Umm, sorry I would take up some other good-first-bug in Javascript. Thanks :)

	sharu
	Comment 6 • 7 years ago

Hi,
I am beginner and my first and want to take up this bug.Can you please guide me.I have just built up firefox.

	Andrew Swan [:aswan]
	Comment 7 • 7 years ago

Hi, are you comfortable working in C++?
Assuming you are, the relevant code is here:
http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/toolkit/components/extensions/WebExtensionPolicy.cpp#400-402

Then you could do a simplified version of this to log a message at that point:
http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/toolkit/mozapps/extensions/AddonContentPolicy.cpp#68-91

	tumpio
	Comment 8 • 7 years ago

Does this bug cover webRequest API regarding the behaviour of unprivileged requests? Requests which the extension does not have access to, "some sensitive requests" (see bug 1380729) and requests not fulfilled by the permissions granted to the extension, should result in a warning in browser console. Otherwise the behaviour of webRequest API is misleading (see bug 1380739).

I've created a extension for testing this covering only one test case. (Instead of an exception, a warning should be logged.)
https://bugzilla.mozilla.org/show_bug.cgi?id=1380739#c1

	Andrew Swan [:aswan]
	Comment 9 • 7 years ago

Bug 1380739 now covers this for webRequest

	sharu
	Comment 10 • 7 years ago

sorry,
i think my level of technical is not enough.sorry for wasting your time.

	Caitlin Neiman [:caitmuenster]
	Comment 11 • 7 years ago

Sharu, no worries at all! We have a list of other good-first-bugs you might want to look into here: https://waffle.io/mozilla/addons?label=contrib:%20good%20first%20bug

Let me know if we can be of any further help finding a good bug to work on!

	Nicklas Boman [:smurfd]
	Comment 12 • 7 years ago

Hi, how would one go about reproducing this issue and what is the expected results? 

If i understand things correctly:
http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/toolkit/components/extensions/WebExtensionPolicy.cpp#400-402  

You would like to call a function like LogMessage() that spits out a error before returning false?

Would it if so be ok to have the LogMessage() function in WebExtensionPolicy.cpp ?

	Andrew Swan [:aswan]
	Comment 13 • 7 years ago

> Would it if so be ok to have the LogMessage() function in
> WebExtensionPolicy.cpp ?

That sounds right, so long as MatchesURI() isn't called in some other case where we wouldn't want to emit the message.  I don't think it is but Kris can confirm.

	Nicklas Boman [:smurfd]
	Comment 14 • 7 years ago

Hi, i went ahead and added what i Think is a patch that will fix this. 
It would write something when a extension is blocked. i hope..

Bare with me, 1st patch :)

	Nicklas Boman [:smurfd]
	Comment 15 • 7 years ago

... and it seems like MatchesURI() only is called as a return to Matches()
Its declared in extensions/cookie/nsPermission.cpp but that shouldn't matter since its a completely different function.

	Andrew Swan [:aswan]
	Comment 16 • 7 years ago

I was set at the mentor for this bug before Kris rewrote it all in C++, which I think makes him better qualified to handle it.  Redirecting to him.

	Kris Maglione [:kmag]
	Comment 17 • 7 years ago

Comment on attachment 8914119 [details] [diff] [review]
1309033.patch

Review of attachment 8914119 [details] [diff] [review]:
-----------------------------------------------------------------

I think we probably need to pass an additional argument when we want the failures to be reported. There are a lot of places we use match patterns, and this will almost certainly generate a huge amount of console spam for, e.g., webRequest matching.

Also, obviously, this needs an actual message :)

	Nicklas Boman [:smurfd]
	Comment 18 • 7 years ago

Okidoki I can fix.

Ahh i thought the console->LogMessage() took care of that. but i guess if i do :
  console->LogStringMessage(NS_LITERAL_STRING( "WebExtension Script Blocked" ).get());  yes?

then to the $0.1 question. how do i get a Webcontent script blocked? so i can test that it actually spits out a message to the Webconsole? :)

	Nicklas Boman [:smurfd]
	Comment 19 • 7 years ago

Fixed so that there is a message :)

Is it so that there should be giving a message whenever Any of the IF cases are true? <-- i think this one, right?
or is it only for when the IsValidSite() is true or when no IF cases are true?!

and i assume a simple bool as parameter to LogMessage() is okey?

	Nicklas Boman [:smurfd]
	Comment 21 • 7 years ago

Added new patch, that should give an errormessage when a Script is blocked.

But i cant for my life understand howto get this if case to be true:  if (AddonManagerWebAPI::IsValidSite(aURL.URI()))
LOL i mean you told me yesterday in #Webextensions to simply add a something like "matches": ["*://*.mozilla.org/*"], in a manifest.json       ( hashtag 1st bug issues :P )

So i took the Borderify example webextension, which already had that. 
Loaded Debug Addon and add temoprary addon, selectd the .json file.
opened the Browser Console (Ctrl+shift+j)
went to https://addons.mozilla.org   .... nothing

its obviously something easy that im missing :)

	Nicklas Boman [:smurfd]
	Comment 22 • 7 years ago

This one fixes the issue with not having the boolean set to a default, in the functions where it is returned
Also now works to report that it blocks webextension scripts to the Browser console.

	Nicklas Boman [:smurfd]
	Comment 23 • 7 years ago

forgot to needinfo this

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 24 • 6 years ago

Hi Nicklas,
I'm so sorry for the long time you have been waiting for a feedback on the last version of your patch, Kris has a very long queue of reviews and he hasn’t gotten to this yet, our sincere apologies about it.

Caitlin brought this bug to my attention and, even if this is a kind of change that I can't fully review and sign-off (on these parts of the WebExtensions internals Kris is the best one who could give it the last round of review and determine when it is ready for landing), I would like to provide some feedback based on what I see clearly missing in the current version of the attached patch (in case you are still interested on trying to complete this change, but if you moved on and you are not interested anymore, we totally understand it and you don't need to worry about it).

So, going back to the feedback:

## Missing test

This patch is currently missing a new test, and it is especially important for a patch like this (e.g. because it applies changes at the c++ level and we want to be sure that it works as intended and it doesn't introduce unexpected issues, and also because once it lands we want to protect the feature from regressing in the future without being noticed).

Based on what we need to test here, it looks that we could use an xpcshell test. 
The xpshell tests runs without a Firefox UI, but if we need to simulate a page loaded in a tab we have also some test helpers that allow us to do that in an xpcshell test (in a way that is similar enough to a test that runs in a complete Firefox instance).

You may take a look at some of the other xpcshell tests that are testing the "content scripts" related stuff:

- https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_WebExtensionContentScript.js, this is basically a "unit-test" for the WebExtensionContentScript component, and it may be enough to test this patch

- https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_contentscript.js, this is testing the content scripts using a test extension and a fake tab, in case a "unit-test" like the above is not enough to test this patch behaviors and possible scenarios

To test the actual message logged, we have a `promiseConsoleOutput` test helper that you may find useful (but let us know if it is not being able to catch the log emitted by this patch for some reasons):

- here is an existent test that uses that test helper: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js#49-65

## Make the logged message a bit more detailed

I think that the logged message should include some additional details, e.g. the extension id and name and the url of the document on which the content script has been blocked may be useful in that message (to allow a developer or a user to understand what is actually triggering that logged message).

I would suggest to add the new test case first (in my opinion, it is easier and more helpful to add a test before updating the patch too much further, the automated tests allow the developer to focus more on the new pieces and worry less about the other parts that were already working before the new changes), 

## Coding style issues

The attached patch currently contains some changes on white spaces and indentation that doesn't seem that should be really part of the needed changes.

Double-check your patches from time to time (and before updating the patches you attach on bugzilla) for this kind of involuntary changes (they often become part of the commit by mistake, e.g. because enforced on every file save and/or on the paste commands by some of the editor's settings), and remove them from the commit to keep it clean and reduced to the essential changes (e.g. for the simpler ones by fixing the spacing manually and then amending the commit, or using 'hg histedit" and "hg crecord" in more complex cases).

	Nicklas Boman [:smurfd]
	Comment 25 • 6 years ago

Hey thanks for the lengthy reply!
Yeah i am still interested.
Its allright, no worries. People should be busy :)

Will add more detail to the message and see if i can figure out how to test this :)
(i would also attach my test to the bug, yes?)

	Nicklas Boman [:smurfd]
	Comment 26 • 6 years ago

First of, this was marked as, a good starter bug, so i hope you bare with me with my questions :)
(or if anyone feels helpful and drop me a mail then we dont have to spam people, unless its okay? Dont want to break bugzilla policys)

About getting more info into my output message. I dont feel like re-inventing the wheel. 
So started to dig around to find something that grabbed the info i was looking for.

Found this 
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionPolicyService.cpp#188
so i guess you can use parts of that?


Started looking at the testing part. Figured out you run the xpcshell tests like :
 ./mach xpcshell-test toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js
Guess you have to modify some of the tests to make it test my function?... even though that specific test ran and Passed all checks.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 27 • 6 years ago

(In reply to Nicklas Boman from comment #26)
> First of, this was marked as, a good starter bug, so i hope you bare with me
> with my questions :)
> (or if anyone feels helpful and drop me a mail then we dont have to spam
> people, unless its okay? Dont want to break bugzilla policys)

No worries, "good first bugs" are definitely a good place to start to learn,
and learning also means "making mistakes" and "having new questions that
want their answers".

If you want more info than the kind (and/or amount) that you feel comfortable to ask on a bugtracker,
IRC (e.g. the #webextensions channel on irc.mozilla.org) and emails (e.g. dev-addons@mozilla.org)
are also good communication channels.

> About getting more info into my output message. I dont feel like
> re-inventing the wheel. 
> So started to dig around to find something that grabbed the info i was
> looking for.
> 
> Found this 
> https://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> ExtensionPolicyService.cpp#188
> so i guess you can use parts of that?

hehe, "strings are hard", and also... we have many string types :-)

Like I mentioned before, I don't work so often on the C++ components and so my info
on the topic may not be updated, or I may miss something and my own opinion may be wrong.

But a more precise review of the c++ code will be needed on the patch in any case, and so in my opinion you can start by picking the one that seems to better fit, based on your needs (and remember that "there is nothing wrong" if "it ends to be
the wrong approach", the code reviews are also a way to learn from your own mistakes, especially the ones for a "good first bug").

At a first glance nsPrintfCString seems useful to achieve what you need, when the patch is functionally complete and ready for the final review, the final reviewer (which should be Kris) will check if it was actually the best pick for the job in this case, and suggest a different approach if it is not.
 
> 
> Started looking at the testing part. Figured out you run the xpcshell tests
> like :
>  ./mach xpcshell-test
> toolkit/components/extensions/test/xpcshell/test_ext_eventpage_warning.js
> Guess you have to modify some of the tests to make it test my function?...
> even though that specific test ran and Passed all checks.

"Being able to run an existing test" is a step in the right direction, this way you have a practical confirmation that your development environment is working correctly and it is able to run the existing tests successfully.

In some cases you may add some additional steps and assertions, or an entire new test case, in an existing test file.

If the scenarios to test are too complex or too many (or the new tests doesn't fit well in any of the existing test files), it is usually better (and reasonably simple) to add a new test file.

The following MDN doc page should provide some additional details about how to write an xpcshell test:

- https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests

(My apologies, I should have added the above link into comment 24)

While writing the new automated tests, our goal is ensuring that the new code written and the new related scenarios (and the most important corner cases) are covered by automated tests with some test assertions, which verify that the actual behaviors are the expected ones, and that they will not break by mistake with further changes.

In other words: the most important achievement for a test is not to "pass successfully" all the time, but more to "fail (possibly with helpful error messages) when it is supposed to fail" ;-)
(well, and also to "be readable by humans" and to "do not fail intermittently", too many "false-negative" will not provide useful information when needed).

Last thing: use the "let's write some automated tests" step as a "time to re-look at this from a different angle", testing is also an important part of the writing process, it allows you to re-think about everything you wrote and the many ways it may break.
Even the "test that you may not write" is going to be helpful (thinking about it may help catch some issues that are hard to test reliably, but that you still need to take into account and fix the implementation to handle them properly).

	Nicklas Boman [:smurfd]
	Comment 28 • 6 years ago

Massive thanks Luca

Short reflection, things has changed since the bug was initially filed and i looked at it a few months ago...
This : https://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/toolkit/components/extensions/WebExtensionPolicy.cpp#400-402
Has been moved to : https://searchfox.org/mozilla-central/source/toolkit/components/extensions/WebExtensionPolicy.cpp#365

and This message was not shown in the browser console when i looked at it earlier... 
"Content Security Policy: The page’s settings blocked the loading of a resource at self (“style-src”)."

At this point i got my funktion running after this : 
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/WebExtensionPolicy.cpp#529-531
If the boolean for reporing error is True.

Then i would get a output in Browser Console like this:

This site blocked a WebExtension Script!
Site: https://www.mozilla.org/en-US/
Extension: Borderify
ExtensionID: 221c5df5-626e-4793-bbd7-69f81aa33c62


Im otherwise back to a loop of static/non-static issues getting my function to run in
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/WebExtensionPolicy.cpp#365

i think ill mail the dev-addons@mozilla.org unless i break some good ideas during the next couple of days ;)

	Nicklas Boman [:smurfd]
	Comment 29 • 6 years ago

Positive update! (on a monday, not often That happens :) )
Have it working now! 
Will tidy up, create test and attach test+patch

	Nicklas Boman [:smurfd]
	Comment 30 • 6 years ago

Hey again Luca, hopefully this is something you can advise me on, or otherwise you might know who can.

Basicly im struggling abit with creating this test/tests. If i understand things correctly i need to test these areas.

1, Wherever i have poked, needs testing, so if i have only touched WebExtensionPolicy.cpp/.h (have rewritten some stuff since last patch) something like toolkit/components/extensions/test/xpcshell/test_WebExtensionPolicy.js could be good to run and see it is still passing.

2, i need to test my functionallity, in a sortof very general way.

The way i found for this if-statement ( https://searchfox.org/mozilla-central/source/toolkit/components/extensions/WebExtensionPolicy.cpp#365 ) to ever be true was todo the following.

a. Create an Extension to match for :"matches": ["*://*.allizom.org/*"]
b. Flip the extensions.webapi.testing to true
c. Debug Addons and manually load the extension.
d. browse to : https://addons-dev.allizom.org/en-US/firefox/

I quickly realized, you can't access websites that are not local, using xpcshell tests.

so i then need to create something like toolkit/components/extensions/test/xpcshell/test_ext_contentScripts_register.js
but maby not so extensive. Just a site that will try to register a contentscript that then is blocked.. somehow 

Am i thinking correctly? :)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 31 • 6 years ago

Hi Nicklas,
do you mind to update the patch you are working on even if it is not yet completed? 
it would make it easier for me to get a full picture of your doubts and be sure that my feedback/advice are actually correct based on the new version of your patch.

In the meantime, looking at your comment 30 it seems to me that you are understanding it correctly, here is some additional pointers that you may find useful to put together the needed test case:

about (1): any components on which your patch is going to apply changes is likely to require some additional testing (it may be some addition to an existing test file or a new tests, "which one of the approaches" is better depend from the kind of change applied and the existing tests for those components)

about (2): yes, you can't connect to a non-local websites from any of the test suites (all the test has to be reproducible, and so clearly they cannot depend from external websites), but in the tests we can still use some test domains that are redirected to a local http server that is part of the test run (e.g. example.com is one of those test domains, which is also already used in a bunch of "content scripts"-related tests: https://searchfox.org/mozilla-central/search?q=example.com&case=false&regexp=false&path=extensions%2Ftest%2Fxpcshell%2Ftest_ext_contentscript)

For testing the expected behavior we also need to be sure that content scripts on "example.com" are going to be blocked, and we should be able to achieve that by changing the value of the "extensions.webextensions.restrictedDomains" preference while running the test function, e.g. something similar to the following snippet:

```
add_task(function test_blocked_content_script_warning_on_restricted_domain() {
  async function test_blocked_content_script_warning() {
    // Start to listen for the console messages.
    let {messages} = await promiseConsoleOutput(async () => {
      // Define and load a test extension that is supposed to run a content script
      // on example.com pages.
      const extension = ExtensionTestUtils.loadExtension({
        ...
      });
      await extension.startup();

      // Load a test page for the restricted domain.
      const contentPage = await ExtensionTestUtils.loadContentPage("http://example.com");

      await extension.unload();

      await contentPage.close();
    });

    // Check that the expected message has been logged
    AddonTestUtils.checkMessages(messages, {expected: [
      {message: /TODO expected warning message regexp here/},
    ]});
  }

  // Set the restrictedDomains pref while running the test_blocked_content_script_warning function.
  return runWithPrefs([
    ["extensions.webextensions.restrictedDomains", "example.com"]
  ], test_blocked_content_script_warning);
});
```

	Nicklas Boman [:smurfd]
	Comment 32 • 6 years ago

I will absolutely do that.  Will also attach the state of the test as it is now. (even though it was towards an external site so that is not working)

	Nicklas Boman [:smurfd]
	Comment 34 • 6 years ago

Thanks Luca, goldstar for long replies!

Added the patch + the test in the state they are today.
I think the patch covers the functionality, will probably need some fine tuning though... (indentation, documentation etc etc)

I also think you covered my concerns without seeing my code :)
Will look more into the testing, but please have a look, and let me know.

Should have asked earlier on what the best practices are, for something like this. Sharing parts of the patch before it being completed. Was worried it would only confuse...

	Firefox Bug Husbandry Bot
	Comment 35 • 6 years ago

Bulk move of bugs per https://bugzilla.mozilla.org/show_bug.cgi?id=1483958

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 36 • 6 years ago

(In reply to Nicklas Boman from comment #34)
> Thanks Luca, goldstar for long replies!
> 
> Added the patch + the test in the state they are today.
> I think the patch covers the functionality, will probably need some fine
> tuning though... (indentation, documentation etc etc)

Hi Nicklas,
I took a look at the attached patch (attachment 9000024 [details] [diff] [review]) and I have some feedback about the C++ pieces from your patch (like I anticipated
any change to the C++ components will need an additional review and sign-off by someone who actually works on the C++ layer, nevertheless the following comments may still be useful in the meantime).

### Make `LogMessageToBrowserConsole` an instance method and remove the sWebExtensionPolicy StaticRefPtr (and the GetSingleton / WEP static methods)

I'm pretty sure that using a global StaticRefPtr is not a reasonable approach and it wouldn't pass a final review on the patch. 

Based on what I saw in the last version of this patch, I think that the reason why you are going down this path is that you are calling the `LogMessageToBrowserConsole` from the static method `IsRestrictedURI`, and that is likely what pushed you to define  `LogMessageToBrowserConsole` as a static method.

In my opinion `LogMessageToBrowserConsole` should just be a `WebExtensionPolicy` instance method,
and by doing that `LogMessageToBrowserConsole` can call the `Name()` and `MozExtensionHostname()` instance methods without any need for the `GetSingleton` / `WEP()` static methods and the `sWebExtensionPolicy StaticRefPtr`.

### Log the message for content script blocked on any restricted site

The patch is currently logging the message only when the content script is restricted on websites that have access to the Addon Manager webAPI (e.g. addons.mozilla.org) (basically the if block with the condition: `if (AddonManagerWebAPI::IsValidSite(aURI.URI())) {...}`), but that message should also be logged for the other restricted sites (the ones checked in the if block above that one: `if (domains->Contains(aURI.HostAtom())) {...}`).

### Log the message only for the extensions content scripts

To be honest, `IsRestrictedURI` doesn't actually block anything, it is just an helper which is used where we want to check if an URI is restricted, which clearly includes the methods that are actually checking if a content script matches a given url, doc or window, but we also use isRestrictedURI in other places that are actually printing their own warning message:

- https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/devtools/server/actors/addon/webextension-inspected-window.js#517
- https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/toolkit/modules/addons/WebRequest.jsm#155-157

And so, it seems to be that calling this `LogMessageToBrowserConsole` method from MozDocumentMatcher::Matches could be more reasonable:

https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/toolkit/components/extensions/WebExtensionPolicy.cpp#518

### Some additional notes about the `LogMessageToBrowserConsole` function name, parameters and return values

- How about renaming the function to `LogBlockedContentScript`?

- The second boolean parameter doesn't seem really needed, it seems to be always passed as true, let's remove it

- This method doesn't seem to actually need to result any value, let's just make it void, and we should just return earlier if the error can't be logged for any reasons, e.g. because we can't get the consoleService or we can't allocate the nsIScriptError instance, like we do here:
  
  https://searchfox.org/mozilla-central/rev/3fa761ade83ed0b8ab463acb057c2cf0b104689e/dom/security/nsCSPUtils.cpp#136-142

### Make the message visible in the webconsole related to the tab where the script has been blocked
  
The tab's webconsole (the one available from the tab developer toolbox) filters the messages to make it visible there using the Window IDs, and so to make the message visible in the tab's webconsole we have to init an `nsIScriptError` instance with the related inner Window ID.

We should be able to retrieve the inner Window ID from the DocInfo object, I think that we could change this method to get a single `DocInfo` parameter and retrieve the inner Window ID inside the method itself, by using something like:

```
void
WebExtensionPolicy::LogMessageToBrowserConsole(const DocInfo& aDoc)
{
  nsPIDOMWindowOuter* pwin = aDoc.GetWindow();
  if (!pwin) {
    // Return earlier if the DocInfo are not related to a window object.
    return;
  }

  nsPIDOMWindowInner* inner = pwin->GetCurrentInnerWindow();
  if (!inner) {
    // Return earlier if we can't retrieve the innerWindowID to associate the
    // warning message with.
    return;
  }

  const URLInfo aURI = aDoc.PrincipalURL();
  uint64_t aWindowID = inner->WindowID();

  ...
```

Then we need to create init the error object, e.g. using something like:

```
  URLInfo url(aDoc.PrincipalURL());
  URLInfo extUrl(mBaseURI);

  nsString errorMsg;
  errorMsg.AppendLiteral("WebExtension Script from \"");
  errorMsg.Append(Name());
  errorMsg.AppendLiteral("\" blocked on ");
  errorMsg.Append(url.Spec());

  nsCString category("javascript");

  // Associate the console message to the extension base URL.
  nsString fileName;
  fileName.Append(extUrl.Spec());

  error->InitWithWindowID(errorMsg,
                          /* aFilename */ fileName,
                          /* aSourceLine */ EmptyString(),
                          /* aLineNumber */ 1,
                          /* aColumnNumber */ 1,
                          /* aSeverityFlag */ 8, // nsIScriptError::infoFlag
                          category,
                          aWindowID);

  console->LogMessage(error);
```

### Only log the message when the extension content script was actually matching the url

It will be confusing if we are going to log this "blocked content script" message for a content script that wasn't going to match that URL based on its definition (besides the fact that the url was related to a restricted url or document), and so we should probably be sure to log the message only if the content script would have matched the URI.

I guess that one way of doing it could be to extract part of
`bool MozDocumentMatcher::MatchesURI(const URLInfo& aURL) const` into a separate method that only check if the content script would match the URL without checking also if it is restricted, and then to use the extracted method to check if we should log a message for a blocked content script, e.g. something like:

```
bool
MozDocumentMatcher::Matches(const DocInfo& aDoc) const
{
   ...
   auto& urlinfo = aDoc.PrincipalURL();

    if (mRestricted && mExtension->IsRestrictedDoc(aDoc)) {
      if (WantsToMatchURI(urlinfo)) {
        mExtension->LogBlockedContentScriptWarning(aDoc);
      }
      return false;
    }
   ...
}

bool
MozDocumentMatcher::WantsToMatchURI(const URLInfo& aURL) const
{
  if (!mMatches->Matches(aURL)) {
    return false;
  }

  if (mExcludeMatches && mExcludeMatches->Matches(aURL)) {
    return false;
  }

  if (!mIncludeGlobs.IsNull() && !mIncludeGlobs.Value().Matches(aURL.Spec())) {
    return false;
  }

  if (!mExcludeGlobs.IsNull() && mExcludeGlobs.Value().Matches(aURL.Spec())) {
    return false;
  }

  return true;
}

bool
MozDocumentMatcher::MatchesURI(const URLInfo& aURL) const
{
  if (!WantsToMatchURI(aURL)) {
    return false;
  }

  if (mRestricted && mExtension->IsRestrictedURI(aURL)) {
    return false;
  }

  return true;
}
```

	Nicklas Boman [:smurfd]
	Comment 37 • 6 years ago

Luca!

Should have re-read your last reply, in more of a condensed matter (not read the whole thing and kindof understand)
Once i read a few lines and followed the steps, then repeated until done...
Have also backtracked through the earlier replies. Hopefully i have dotted all the i's
 
Its working! :) :)
So does testing! 
(or atleast i assume its working as it should)

If the restrictedDomains pref is set, it fails.
If i remove the part when restrictedDomains pref is set, it passes.

Adding 2 patches, one for the code and one for the testing.

Thanks!

	Nicklas Boman [:smurfd]
	Comment 38 • 6 years ago

This is the actual patch, which adds the functionallity

	Nicklas Boman [:smurfd]
	Comment 39 • 6 years ago

This patch regards the testing part

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 40 • 6 years ago

Comment on attachment 9004043 [details] [diff] [review]
Bug1309033_180826_0040.patch

Review of attachment 9004043 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Nicklas,
follows some additional review comments.
Besides those, the patch should have a summary message that follows the conventions used in the other commits, e.g.

Bug 1309033 - Log a console message when a WebExtensions content script is blocked on a restricted site.

::: toolkit/components/extensions/WebExtensionPolicy.cpp
@@ +500,5 @@
> + * Function used for Logging a message to the Browser Console if a website
> + * blocks a WebExtension script
> + *
> + */
> +void 

This line seems to contain an unnecessary white space 

(as a side note, it is usually worth to double-check the diff to be sure that the patch doesn't contain any unnecessary whitespace, as well as to fix any indentation mistakes).

@@ +517,5 @@
> +    // warning message with.
> +    return;
> +  }
> +
> +  const URLInfo aURI = aDoc.PrincipalURL();

This `aURI` local variable doesn't seem to be used anymore 
(keep an eye on the warning that the compiler may report on the files changed in the patch, it may provide useful suggestions e.g. it should log a warning if there is any unused variable that can be removed).

@@ +529,5 @@
> +  if (!console || !error) {
> +    return;
> +  }
> +
> +  URLInfo url(aDoc.PrincipalURL());

`./mach static-analysis check toolkit/components/extensions/WebExtensionPolicy.cpp` says that this can be a "const URLInfo &url(...);"

@@ +554,5 @@
> +                          category,
> +                          aWindowID);
> +
> +  console->LogMessage(error);
> +} 

Unnecessary whitespace.

@@ +727,4 @@
>  
>    if (!mExcludeGlobs.IsNull() && mExcludeGlobs.Value().Matches(aURL.Spec())) {
>      return false;
> +  } 

Unnecessary whitespace.

::: toolkit/components/extensions/WebExtensionPolicy.h
@@ +165,4 @@
>  
>    nsISupports* GetParentObject() const { return mParent; }
>  
> +	void LogBlockedContentScript(const DocInfo& aDoc); 

This line needs some indentation and whitespace fixes.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 41 • 6 years ago

Comment on attachment 9004044 [details] [diff] [review]
Bug1309033_180826_0212.patch

Review of attachment 9004044 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Nicklas,
Follows some review comments on the test case.

Besides the changes described below, I would suggest to merge the two patches into a single one 
(don't worry of the current two patches, we can mark them as obsolete after you have attached
a new version in a single attachment).

Don't forget to run the test locally, you can run it using:

./mach xpcshell-test toolkit/components/extension/test/xpcshell/test_name.js

(and to lint all the changed files ;-))

Once the test passes, it is also a good idea to run it in --verify mode to double-check it for intermittency ;-)

./mach xpcshell-test --verify toolkit/components/extension/test/xpcshell/test_name.js

::: toolkit/components/extensions/test/xpcshell/test_bug1309033.js
@@ +1,3 @@
> +/* -*-         Mode: indent-tabs-mode: nil; js-indent-level: 2           -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";

This test doesn't pass yet, and it require a number of changes to be complete and green.

e.g. to be able to use "example.com" in this test without trying to contact an external server
(which would make the test to file immediately), this test needs a preamble similar to the one
used in the other similar tests (e.g. here https://searchfox.org/mozilla-central/rev/05d91d3e02a0780f44599371005591d7988e2809/toolkit/components/extensions/test/xpcshell/test_ext_contentscript.js#3-10)

In this test, it may probably enough:

```
const server = createHttpServer({hosts: ["example.com"]});
server.registerDirectory("/", do_get_cwd());

// ExtensionContent.jsm needs to know when it's running from xpcshell,
// to use the right timeout for content scripts executed at document_idle.
ExtensionTestUtils.mockAppInfo();
```

@@ +8,5 @@
> +    let extensionData = {
> +      manifest: {
> +        content_scripts: [
> +          {
> +            "matches": ["*://*.example.com/*"],

"matches": ["*://example.com/*"],

@@ +9,5 @@
> +      manifest: {
> +        content_scripts: [
> +          {
> +            "matches": ["*://*.example.com/*"],
> +            "js": ["content_script.js"]

These lines are missing the leading comma "," and eslint is going to complain (it is usually better to give a lint to all the files changes in a patch, you can easily run the linter checks by running `./mach lint toolkit/components/extensions/test/xpcshell/test_bug1309033.js`).

@@ +15,5 @@
> +        ]
> +      },
> +
> +      files: {
> +        "content_script.js"() {

All these files are not really necessary for this test, the following should be enough:

```
      files: {
        "content_script.js"() {
          browser.test.fail(`Blocked script should not be executed`);
        },
      },
```

So that the test fails if the script is actually executed (because it should not actually happen).

@@ +42,5 @@
> +      await extension.startup();
> +
> +      // Load a test page for the restricted domain.
> +      const contentPage =
> +        await ExtensionTestUtils.loadContentPage("http://www.example.com/");

We should also assert that the innerWindowID is actually the expected one, e.g.:

```
 let innerWindowID;

    // Start to listen for the console messages.
    let {messages} = await promiseConsoleOutput(async () => {
      ...
      const contentPage = await ExtensionTestUtils.loadContentPage(url);

      innerWindowID = await contentPage.spawn(undefined, () => {
        return this.content.windowUtils.currentInnerWindowID;
      });
      ...
    });

        // Check that the expected message has been logged
    AddonTestUtils.checkMessages(messages, {expected: [
      {
        innerWindowID,
        message: new RegExp(`WebExtension Script from "${name}" blocked on ${url}`),
      },
    ]});
```

@@ +51,5 @@
> +    });
> +
> +    // Check that the expected message has been logged
> +    AddonTestUtils.checkMessages(messages, {expected: [
> +      {message: /TODO expected warning message regexp here/},

This regular expression would make the test to fail, the right regexp should be something like:

```
const name = "test extension name";
const url = "http://example.com/";
...
let extensionData = {
      manifest: {
        name,
...

    // Load a test page for the restricted domain.
    const contentPage = await ExtensionTestUtils.loadContentPage(url);

...
AddonTestUtils.checkMessages(messages, {expected: [
  {
    message: new RegExp(`WebExtension Script from "${name}" blocked on ${url}`),
  }
...
```

@@ +55,5 @@
> +      {message: /TODO expected warning message regexp here/},
> +    ]});
> +  }
> +
> +  // Set the restrictedDomains pref while running the 

Unnecessary whitespace.

::: toolkit/components/extensions/test/xpcshell/xpcshell.ini
@@ +49,4 @@
>  [test_locale_converter.js]
>  [test_locale_data.js]
>  [test_ext_ipcBlob.js]
> +[test_bug1309033.js]

None of the tests in this directory is named using the bug number, and so a more descriptive (but still short enough) name would be probably better.

How about "test_contentscript_blocked_message.js"?

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 42 • 6 years ago

(In reply to Nicklas Boman from comment #37)
> Luca!
> 
> Should have re-read your last reply, in more of a condensed matter (not read
> the whole thing and kindof understand)
> Once i read a few lines and followed the steps, then repeated until done...
> Have also backtracked through the earlier replies. Hopefully i have dotted
> all the i's
>  
> Its working! :) :)
> So does testing! 
> (or atleast i assume its working as it should)
> 
> If the restrictedDomains pref is set, it fails.
> If i remove the part when restrictedDomains pref is set, it passes.
> 
> Adding 2 patches, one for the code and one for the testing.
> 
> Thanks!

I'm glad that my feedback comments have been helpful!

I've just added a new round of feedback on the two attachments.

	Nicklas Boman [:smurfd]
	Comment 43 • 6 years ago

Just to keep you updated, 
I have fixed your notes on the code.
Have also fixed your notes on the test, but it seems that the test is not running LogBlockedContentScript() and therefore it fails because we dont get the Console output we expect... 

Looking more into it tomorrow.

	Nicklas Boman [:smurfd]
	Comment 44 • 6 years ago

Sorry appears i need your help again. 

The testing does not seem to make the 'if (mRestricted && mExtension->IsRestrictedDoc(aDoc))' true.
It Does so when i do a manual test running the browser and manually loading a plugin. then browsing to like http://example.com and watching in the Browser Console. That works!

  auto& urlinfo = aDoc.PrincipalURL();
  if (mRestricted && mExtension->IsRestrictedDoc(aDoc)) {
    if (WantsToMatchURI(urlinfo)) {
      mExtension->LogBlockedContentScript(aDoc);
    }
    return false;
  }

attaching the test as it is now, im thinking its something easy we are missing. 
(will remove all attachments once i add my final patch)

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 45 • 6 years ago

Hi Nicklas,

At a quick glance, it is likely due to the regexp (at least if the message didn't changed in the updated patch that actually logs the console message):

     new RegExp(`WebExtension Script from \"${name}\" blocked on \"${url}\"`)

should be

     new RegExp(`WebExtension Script from "${name}" blocked on ${url}`)

(as the regular expression included in comment 41, in particular: there is no need to escape the " in a template string, and the url was not quoted in the message logged in the patch).

Let me know if this change helped (otherwise, feel free to attach the full updated patch, even if it is incomplete or it is not working yet, so that I can give it a try locally and get a better picture of what could be missing).

	Nicklas Boman [:smurfd]
	Comment 46 • 6 years ago

Hey Luca, 

The attached progress-patch has the progress-test also in it.
There are some added printf() to the WebExtensionPolicy.cpp basicly just printing the URL we try to access in MozDocumentMatcher::Matches.

Also commented away some of the errorMsg to be sure there is no issue with string escaping-characters and therefor not matching.

1)Doing a manual ./mach run 
2)Loading a slightly modified Borderify webextension-example, to match example.com
3)Adding example.com to the pref extensions.webextensions.restrictedDomains 
4)Browsing to http://example.com 
5)Looking in Browser Console will show a message that we defined!!!
[X] works :)  (i mean, right?)

When trying to get the same done via a xpcshell-test, there seems to be some issues.

It seems this does not get true : mExtension->IsRestrictedDoc(aDoc) so we never actually run the LogBlockedContentScript() function. It never hits the printf i added in the end of LogBlockedContentScript().

Also seeing that we try to access : about:neterror?e=proxyConnectFailure&u=http%3A//example.org%3A35075/&c=UTF-8&f=regular&d=Firefox%20is%20configured%20to%20use%20a%20proxy%20server%20that%20is%20refusing%20connections.

so i asked in #developers and got a tip it might have todo with the port not beeing 80, so checked for that and added that to the address we go to.

Thought id flip swiches preventing proxies to be used. 
None seem to work ...

Please ignore whitespaces, commented code, unnecessary added stuff etc etc, will go through that once we get this working.

Im starting to wonder if this has something todo with my local setup? 
Please have a look locally

When i run my test i run : 
MOZ_NODE_PATH=/usr/bin/node10 ./mach xpcshell-test toolkit/components/extensions/test/xpcshell/test_contentscript_blocked_message.js

this is in my ~/.mozconfig

export CXX=clang++-6.0.1
export CC=clang-6.0.1
ac_add_options --enable-linker=lld
ac_add_options --disable-elf-hack
CC="clang -fcolor-diagnostics"
CXX="clang++ -fcolor-diagnostics"
ac_add_options --with-ccache=/usr/bin/ccache
mk_add_options PYTHON=/usr/bin/python2

(one good thing, i feel that i have gathered a far bigger knowledge on how to do test, patches and whatnot :))

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 47 • 6 years ago

Comment on attachment 9007624 [details] [diff] [review]
Bug1309033_180909_2020.patch

Review of attachment 9007624 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/xpcshell/test_contentscript_blocked_message.js
@@ +19,5 @@
> +    const port = server.identity.primaryPort;
> +    console.log("port="+port);
> +
> +    const name = "TestExtensionContentScript";
> +    const url = "http://example.com/";

Hi Nicklas,
this test file is starting a test HTTP server for the example.org domain, but the test then is using "http://example.com/" as the url to connect to, whereas this test should choose one test domain and use it everywhere.

This is likely to be the reason why you are able to see the expected message while testing it interactively, but fails in the tests because of the domain name mismatch.

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 48 • 6 years ago

Clearing needinfo (see comment 47).

	Nicklas Boman [:smurfd]
	Comment 49 • 6 years ago

Hey Luca, sadly that was not the case, and not my intention. But after a ton of attempts, .org and .com start to look alike 8)   

After a tip from aswan in #webextensions Im now suspecting that this is a local firewall and or router issue of mine. 

As you say when the test creates that test http server, it then uses ephemeral ports (after googling i realize its fansy pantsy wording for random ports)

Is it Way to much to ask, that you (or ask someone else to) try to apply the patch locally and then run this test, and see if it passes for you?
(Will OFCOURSE not sit here and wait, but knowing network issues, it can take 5 minutes or 5 weeks to figure that out...)

	Nicklas Boman [:smurfd]
	Comment 50 • 6 years ago

With some luck you have not seen the needinfo. It seems i have had some issue with my local build that has caused issues when it has been creating these local webservers for these tests.

hopefully tomorrow i have a test that works!

	Nicklas Boman [:smurfd]
	Comment 51 • 6 years ago

I have learned a few things the last couple of hours.
1, dont manually edit a patch file and remove lines, unless you also change the lines which it should manipulate, then it will not be appliable, like the patch i added on Thursday.
2, Bootstrapping does more than just grab the necessary packages you need (or i have been missing some packages)
3, Building firefox in a VM takes alot longer .. doh

Anywho. Good news everyone! \o/

The attached patch passes the test that it also contains!!!

It does however spit out a info when you run the test, but maby this is to be expected? 

0:14.64 INFO "CONSOLE_MESSAGE: (info) [JavaScript Error: "WebExtension Script from "TestExtensionContentScript" blocked on "http://example.org/"" {file: "jar:file:///tmp/firefox/xpcshellprofile/temp/generated-extension.xpi!/" line: 1}]"

Hopefully i have not re-introduced some white space issues

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 52 • 6 years ago

Comment on attachment 9009452 [details] [diff] [review]
Bug1309033_180916_1340.patch

Review of attachment 9009452 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Nicklas,
this version looks good to me, and so I'm setting an f+ on it \o/

Follows some additional comments related to some small nits (just one more unneeded whitespace, and some optional change to the inline comments used on the call to InitWithWindowID).

::: toolkit/components/extensions/WebExtensionPolicy.cpp
@@ +500,5 @@
> + * Function used for Logging a message to the Browser Console if a website
> + * blocks a WebExtension script
> + */
> +void
> +WebExtensionPolicy::LogBlockedContentScript(const DocInfo& aDoc) 

Unneeded whitespace at the end of this line ;-)

@@ +544,5 @@
> +  nsString fileName;
> +  fileName.Append(extUrl.Spec());
> +
> +  error->InitWithWindowID(errorMsg,
> +                          /* aFilename */ fileName,

Nits:
this parameter is called `fileName`, and so I think that we can avoid the related inline comment.

Maybe we could also reverse the order between the inline comments and the related parameter and use a single line comments? (e.g. like we do here: https://searchfox.org/mozilla-central/rev/6c82481caa506a240a626bb44a2b8cbe0eedb3a0/layout/style/FontFaceSet.cpp#1414-1421)

(but it is also fine with me if you leave it like it is right now and hear the opinion of the final reviewer of this patch,
mainly because it is still possible that the final reviewer may have a different opinion about how this should look like).

	Nicklas Boman [:smurfd]
	Comment 53 • 6 years ago

This patch addresses the last comments from Luca
It replaces attachment 9009452 [details] [diff] [review] (but didn't want to make it Obsolete to loose that F+ :) )

	Luca Greco [:rpl] [:luca] [:lgreco] Assignee
	Comment 54 • 6 years ago

Comment on attachment 9010013 [details] [diff] [review]
Bug1309033_180918_1928.patch

Review of attachment 9010013 [details] [diff] [review]:
-----------------------------------------------------------------

Marked the old version as obsolete and propagated the f+ on the new version of the attached patch.