Closed Bug 832193 Opened 11 years ago Closed 11 years ago

Content Security Policy: a source of *.something.com is mistakenly interpreted as a source of http://*:80

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
17 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21
Tracking Flags:
Tracking Status
firefox18 - wontfix
firefox19 + wontfix
firefox20 + fixed
firefox21 + fixed
firefox-esr17 20+ fixed
b2g18 20+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: alice0775, Assigned: imelven)

References

Details

(Keywords: regression, sec-moderate)

Attachments

(2 files, 4 obsolete files)

patch v3
6.04 KB, patch
geekboy
: review+
lsblakk
: approval-mozilla-aurora+
akeybl
: approval-mozilla-b2g18+
Details | Diff | Splinter Review
esr17 backport of the patch that landed in mozilla21
5.86 KB, patch
akeybl
: approval-mozilla-esr17+
Details | Diff | Splinter Review 
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/712eca11a04e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130117 Firefox/21.0 ID:20130117030933

Content-Security-Policy header does not work as expected

httpd.conf of Apache:

Header set X-Content-Security-Policy "allow 'self'; img-src *.mozillazine.org"
Header set Content-Security-Policy "default-source 'self'; img-src  *.mozillazine.org"

test.html:

<!DOCTYPE HTML>
<html>
<head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<head/>
<body>
<img src="http://i.yimg.jp/images/mh/mail.gif" >
<img src="http://forums.mozillazine.org/static/common/images/blimp.png">
<img src="http://tenki.jp/images/header/logo/logo.gif" >
</body>
</html>



Steps to reproduce:
  1. Configure Apache httpd.conf to send Content-Security-Policy Response Header
  2. Open web page

Actual results:
  All images are loaded and displayed.


Response Headers of test:html:
  X-Content-Security-Policy:allow 'self'; img-src *.mozillazine.org
  Server:Apache/2.2.22 (Win32)
  Last-Modified:Fri, 18 Jan 2013 08:12:54 GMT
  Keep-Alive:timeout=5, max=100
  Etag:"330000000d89e5-13e-4d38bada5a16a"
  Date:Fri, 18 Jan 2013 08:14:00 GMT
  Content-Type:text/html
  Content-Security-Policy:default-source 'self'; img-src  *.mozillazine.org
  Content-Length:318
  Connection:Keep-Alive
  Accept-Ranges:bytes


Error console(with setting security.csp.debug = true):

  CSP debug: CSP CREATED

 ----------
  CSP debug: CSP POLICY INITED TO 'default-src *'

 ----------
  Warning: The X-Content-Security-Policy and X-Content-Security-Report-Only headers
  will be deprecated in the future. Please use the Content-Security-Policy and
  Content-Security-Report-Only headers with CSP spec compliant syntax instead.
  Source file: http://localhost/test.html
 ----------
  CSP debug: REFINE POLICY: allow 'self'; img-src *.mozillazine.org

 ----------
  CSP debug:          SELF: http://localhost/test.html

 ----------
  CSP debug: CSP 1.0 COMPLIANT : false

 ----------
  Warning: CSP WARN:  allow directive is deprecated, use the equivalent
  default-source directive instead

 ----------
  CSP debug:  in permitsAncestry(), docShell = [xpconnect wrapped nsIDocShell]

 ----------
  CSP debug: shouldLoad location = http://i.yimg.jp/images/mh/mail.gif

 ----------
  CSP debug: shouldLoad content type = 3

 ----------
  CSP debug: policy is pre-1.0

 ----------
  CSP debug: shouldLoad cspContext = img-src

 ----------
  CSP debug: shouldLoad location = http://forums.mozillazine.org/static/common/images/blimp.png

 ----------
  CSP debug: shouldLoad content type = 3

 ----------
  CSP debug: policy is pre-1.0

 ----------
  CSP debug: shouldLoad cspContext = img-src

 ----------
  CSP debug: shouldLoad location = http://tenki.jp/images/header/logo/logo.gif

 ----------
  CSP debug: shouldLoad content type = 3

 ----------
  CSP debug: policy is pre-1.0

 ----------
  CSP debug: shouldLoad cspContext = img-src



Expected results:
  Images should be blocked in accordance with Content-Security-Policy Response Header.
  I.e., Only the second image should be displayed.



Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/88ba578cbaab
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120816111151
Bad:
http://hg.mozilla.org/mozilla-central/rev/a79132ac2f05
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120816175051
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=88ba578cbaab&tochange=a79132ac2f05

Regression window(m-c)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/12c614d36e0b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120816094950
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d5fab9ef16a4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120816110150
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=12c614d36e0b&tochange=d5fab9ef16a4


Triggered by: 
d5fab9ef16a4	Marshall Moutenot — Bug 737064 - sync CSP source-expression parsing with w3c spec (r=geekboy)
I'll take a look, I've had some other reports along these lines via email. Thanks Alice !
A couple of comments :

* support for Content-Security-Policy (the unprefixed header) is pref'd off until bug 763879 is finished so that header will be ignored, I will file a new bug for flipping the pref when it's ready. 

* default-source is incorrect, it should be default-src

Neither of these are relevant to the actual issue in the bug, I just make them here for future reference :)
I'm wondering if this regression in Fx17 from bug 737064 was fixed in Fx18 with bug 784315

This could be a new regression from landing bug 783049 or bug 746978, I see you were using nightly/Fx21 to test.. 

I'm also very curious why our CSP mochhitests didn't catch any new regression that occurred - I know Marshall added a test in Fx18/bug 784315 to catch the one introduced in Fx17

Also, the debug logs and headers are incredibly useful, thank you !

I'll keep digging into this.
Ok, this seems to be broken in 18, 19, 20, and 21 as well so it seems like bug 784315 is a different issue.
I'm debugging this and want to fix it asap.
Assignee: nobody → imelven
Status: NEW → ASSIGNED
Since this is not a regression in FF18, we'll wontfix there.
status-firefox18: affected → wontfix
status-firefox-esr17: --- → affected
tracking-firefox18: ? → -
tracking-firefox19: ? → +
tracking-firefox20: ? → +
tracking-firefox21: ? → +
tracking-firefox-esr17: ? → 19+
The problem looks to be with img-src *.mozillazine.org , specifically the bug seems to be when there's a source of form *.something.something
Summary: Content-Security-Policy header does not work as expected since Firefox17 → Content Security Policy: a source of *.something.com is mistakenly interpreted as a source of http://*:80
dveditz : sec-moderate?
Flags: needinfo?(dveditz)
Yeah, sounds good. Need to get this fixed on the b2g branch as well since CSP is used for app security
status-b2g18: --- → affected
tracking-b2g18: --- → 19+
Flags: needinfo?(dveditz)
Keywords: sec-moderate
An xpcshell test that would have caught this

Still working on the fix
Forgot to qref
Attachment #704155 - Attachment is obsolete: true
The problem is that this regex 

// host            = "*" / [ "*." ] 1*host-char *( "." 1*host-char )
const R_HOST       = new RegExp ("\\*|(((\\*\\.)?" + R_HOSTCHAR.source +
                                       "+)(\\." + R_HOSTCHAR.source +"+)*)",'i');

is matching *.mozillazine.org as "*", hence the source gets created as http://*:80
Attached patch patch v1 (obsolete) — — Splinter Review 
This passes all the existing CSP tests, adds a new test for the issue in this bug, and also passes Alice's original test case - thanks again for providing that !
Attachment #704157 - Attachment is obsolete: true
Attachment #704164 - Flags: review?(sstamm)
I'm also concerned about a source of ** or *a , basically anything that starts with a * that doesn't match the first part of the regexp looking for *.something - i think these sources may still be parsed as a host of * when they should be rejected  - i'm planning to test that out asap.
Comment on attachment 704164 [details] [diff] [review]
patch v1

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

Test looks good, but I think you're right, we should probably test for invalid stuff like ** and *a.

::: content/base/src/CSPUtils.jsm
@@ +44,5 @@
>  const R_HOSTCHAR   = new RegExp ("[a-zA-Z0-9\\-]", 'i');
>  
>  // host            = "*" / [ "*." ] 1*host-char *( "." 1*host-char )
> +const R_HOST       = new RegExp ("(((\\*\\.)?" + R_HOSTCHAR.source +
> +                                      "+)(\\." + R_HOSTCHAR.source +"+)*)|\\*",'i');

Nit: space before "+)*)" after + operator and also between comma-sep args (space before 'i').

Why did moving "(\\*\\.)?" fix this?  If it's because we want to rule out the cases where it's *. first, then this regex isn't greedy/specific enough...  

Thanks for fixing this indent, but what do you think about splitting the literals up a bit to balance the parens:

= new RegExp("(" + "((\\*\\.)?" + R_HOSTCHAR.source + "+)" +
                   "(\\." + R_HOSTCHAR.source +"+)*)|\\*", 'i');
Attachment #704164 - Flags: review?(sstamm)
> Why did moving "(\\*\\.)?" fix this?  If it's because we want to rule out
> the cases where it's *. first, then this regex isn't greedy/specific
> enough...  

I also agree. I think the correct fix is to surround R_HOST with "^(" and a ")$" at both ends. The current fix assumes that the regex engine will use the left part of the alternation first. IIRC, there can be an engine that doesn't (ofcourse, the mozilla one follows the standard pattern).

This same issue is already solved in the code for R_SCHEME with the R_GETSCHEME regex. The latter uses a look-ahead in addition to ^ and $. The invariant that the code wants to maintain is that only R_GETSCHEME should be used to do a .exec, not R_SCHEME. R_SCHEME should only be used to create new regexes. Unfortunately, there was no R_GETHOST. You can create one and replace the use of R_HOST.exec with R_GETHOST.exec

I personally also don't like the naming scheme, which seems to be "R_SCHEME is for internal use, R_GETSCHEME is ok to use externally". I would rather see R_SCHEME_INTERNAL and R_SCHEME respectively (and similarly for R_HOST). Better yet, private vars!


Also, in a related note, the definition of R_EXTHOSTSRC seems wrong. If I am not wrong, it will end up with multiple $ in the regex (one from the R_HOSTSRC, and one in the declaration).
Comment on attachment 704164 [details] [diff] [review]
patch v1

When adding more tests, I found this patch is incorrect.

I think we should add tests like :

      //"* permits all"
      src = CSPSource.create("*", undefined, "https://foobar.com:443");
      do_check_true(src.permits("http://barbaz.com"));
      //"** doesn't permit all"
      src = CSPSource.create("**", undefined, "https://foobar.com:443");
      do_check_false(src.permits("http://barbaz.com"));
      //"*a doesn't permit all"
      src = CSPSource.create("*a", undefined, "https://foobar.com:443");
      do_check_false(src.permits("http://barbaz.com"));
      //"*.foo.com doesn't permit all"
      src = CSPSource.create("*.foo.com", undefined, "https://foobar.com:443");
      do_check_false(src.permits("http://barbaz.com"));

(In reply to Devdatta Akhawe [:devd] from comment #16)
>
> I also agree. I think the correct fix is to surround R_HOST with "^(" and a
> ")$" at both ends. 

Sid and I discussed this very approach yesterday :)

Thanks for helping look into this !
Attachment #704164 - Attachment is obsolete: true
(In reply to Ian Melven :imelven from comment #17)
> Comment on attachment 704164 [details] [diff] [review]
> patch v1
> 
> When adding more tests, I found this patch is incorrect.

actually the tests I added were wrong. I was testing CSPSource.create but the issue descibed in this bug is actually exposed via CSPSourceList.create, so I worked with these tests :

+      //"** doesn't permit all"
+      do_check_false(allDoubledHostSourceList.permits("http://barbaz.com"));
+      //"*a doesn't permit all"
+      do_check_false(allGarbageHostSourceList.permits("http://barbaz.com"));
+
+      //"*.foo.com permits somerandom.foo.com"
+      do_check_true(wildcardHostSourceList.permits("http://somerandom.foo.com")
);
+      //"*.foo.com doesn't permit all"
+      do_check_false(wildcardHostSourceList.permits("http://barbaz.com"));

the original patch of moving the * to the end passes these tests - I did try out adding ^ and $ as discussed, making an R_GETHOST, but these broke various other csputils tests.
(In reply to Ian Melven :imelven from comment #18)

> +      //"** doesn't permit all"
> +      do_check_false(allDoubledHostSourceList.permits("http://barbaz.com"));
> +      //"*a doesn't permit all"
> +      do_check_false(allGarbageHostSourceList.permits("http://barbaz.com"));
> +
> +      //"*.foo.com permits somerandom.foo.com"
> +     
> do_check_true(wildcardHostSourceList.permits("http://somerandom.foo.com")
> );
> +      //"*.foo.com doesn't permit all"
> +      do_check_false(wildcardHostSourceList.permits("http://barbaz.com"));

where

+      var wildcardHostSourceList = CSPSourceList.fromString("*.foo.com",
+                                                            undefined, URI("htt
p://self.com"));
+      var allDoubledHostSourceList = CSPSourceList.fromString("**");
+      var allGarbageHostSourceList = CSPSourceList.fromString("*a");

Also keep in mind we want to replace the CSP JS implementation with C++ code at some point likely fairly soon, see bug 820196
(In reply to Devdatta Akhawe [:devd] from comment #16)
>
> I also agree. I think the correct fix is to surround R_HOST with "^(" and a
> ")$" at both ends. The current fix assumes that the regex engine will use
> the left part of the alternation first. IIRC, there can be an engine that
> doesn't (ofcourse, the mozilla one follows the standard pattern).

this doesn't work, because R_HOST is used in R_HOSTSRC where the problem occures here : http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#1363

same problem as you pointed out with R_EXTHOSTSRC
I meant, create a new R_GETHOST that is R_HOST surrounded by ^ and $. (see R_GETSCHEME )
(In reply to Devdatta Akhawe [:devd] from comment #21)
> I meant, create a new R_GETHOST that is R_HOST surrounded by ^ and $. (see
> R_GETSCHEME )

Then R_GETHOST doesn't match a host with a scheme eg https://foo.com:443 if we use R_GETHOST.exec here : http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#1375
Attached patch patch v2 (obsolete) — — Splinter Review 
ok, this doesn't touch R_HOST at all except for some formatting including the changes Sid asked for, passes all the CSP xpcshell tests and mochitests and also Alice's original test case.
Attachment #705678 - Flags: review?(sstamm)
Comment on attachment 705678 [details] [diff] [review]
patch v2

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

::: content/base/src/CSPUtils.jsm
@@ -1384,2 @@
>      sObj._host = CSPHost.fromString(hostMatch[0]);
> -    var portMatch = R_PORT.exec(aStr);

Woah, what happened here?

::: content/base/test/unit/test_csputils.js
@@ +304,2 @@
>        do_check_true( allSourceList.permits("http://x.com:23"));
> +      //"* does permit a long host with no port"

I see you reworded these things... these strings (See above comments too) used to be messages printed when the test fails.  They should all be about the failure condition or all about the test condition.  If you tweak these two, please tweak the rest in the file.
Attachment #705678 - Flags: review?(sstamm)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #25)
> Comment on attachment 705678 [details] [diff] [review]
> patch v2
> 
> Review of attachment 705678 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/CSPUtils.jsm
> @@ -1384,2 @@
> >      sObj._host = CSPHost.fromString(hostMatch[0]);
> > -    var portMatch = R_PORT.exec(aStr);
> 
> Woah, what happened here?

it moved up a few lines so i can strip the port off if there's one before
creating the host :

--> +    var portMatch = R_PORT.exec(hostMatch);
+
+    // Host regex also gets port, so remove the port here.
+    if (portMatch)
+      hostMatch = R_HOSTSRC.exec(hostMatch[0].substring(0, hostMatch[0].length - portMatch[0].length));
+
     sObj._host = CSPHost.fromString(hostMatch[0]);
-    var portMatch = R_PORT.exec(aStr);


> ::: content/base/test/unit/test_csputils.js
> @@ +304,2 @@
> >        do_check_true( allSourceList.permits("http://x.com:23"));
> > +      //"* does permit a long host with no port"
> 
> I see you reworded these things... these strings (See above comments too)
> used to be messages printed when the test fails.  They should all be about
> the failure condition or all about the test condition.  If you tweak these
> two, please tweak the rest in the file.

ah, I misunderstood what these comments were - thanks for the clarification. I tweaked the ones I added to match the rest of the file.
Attached patch patch v3 — — Splinter Review 
Fix the test failure messages.
Attachment #705678 - Attachment is obsolete: true
Attachment #707230 - Flags: review?(sstamm)
Comment on attachment 707230 [details] [diff] [review]
patch v3

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

Ok, yeah, I missed the moving of that line before and freaked out.  Looks good.
Attachment #707230 - Flags: review?(sstamm) → review+
https://hg.mozilla.org/mozilla-central/rev/4285b55dfb2d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox21: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
If we think this is something that needs to go into FF19, we need a nomination for uplift to Aurora/Beta no later than today. Beta 5 (the last real opportunity for non-critical forward fixes) is going to build tomorrow.
(In reply to Alex Keybl [:akeybl] from comment #31)
> If we think this is something that needs to go into FF19, we need a
> nomination for uplift to Aurora/Beta no later than today. Beta 5 (the last
> real opportunity for non-critical forward fixes) is going to build tomorrow.

discussed with Sid, we think we want to uplift this to Aurora but not to Beta due to lack of bake time on m-c. We're confident enough in the tests to go to Aurora but don't see a compelling reason to take risk on Beta. This is a regression that affects sites that use a specific kind of CSP (one with a source of form *.foo.com) but doesn't affect all users of CSP.
Comment on attachment 707230 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression for Bug 737064
User impact if declined: CSP sources of *.foo.com will incorrectly allow all hosts
Testing completed (on m-c, etc.): it's been on m-c a few days, added new tests to CSP parser tests that would have caught this bug, also added some more tests around parsing of sources that include a *
Risk to taking this patch (and alternatives if risky): might cause a new regression, but we've tried to improve the tests to avoid this
String or UUID changes made by this patch: none

We'd like to get this patch to users for Fx20, but feel that Fx19 might be a bit of a rush due to low bake time on m-c.
Attachment #707230 - Flags: approval-mozilla-aurora?
Comment on attachment 707230 [details] [diff] [review]
patch v3

Approving for Aurora so we get lots of bake time before shipping.
Attachment #707230 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Considering this is "wontfix" for 19 changing the tracking esr/b2g flags to 20+
tracking-b2g18: 19+ → 20+
tracking-firefox-esr17: 19+ → 20+
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
status-b2g18-v1.0.1: --- → affected
Can we get an uplift nomination for ESR17 and B2G18? Thanks!
Comment on attachment 707230 [details] [diff] [review]
patch v3

nomination for both b2g18 and esr17 :

[Approval Request Comment]
Bug caused by (feature/regressing bug #): regression by Bug 737064
User impact if declined:  CSP sources of *.foo.com will incorrectly allow all hosts
Testing completed: has been on m-c since Feb 3rd, uplifted to aurora on Feb 4th, no new CSP bug reports as of yet, added CSP tests around this bug with the m-c landing
Risk to taking this patch (and alternatives if risky): seems pretty stable in terms of bake time, may change B2G behavior if apps use a source of *.something.com and something is dependency on content being incorrectly allowed which would now be blocked.
String or UUID changes made by this patch: none

Risk of not taking this patch: CSP will function incorrectly and won't block sources that should be blocked, reducing the protection it offers.
Attachment #707230 - Flags: approval-mozilla-esr17?
Attachment #707230 - Flags: approval-mozilla-b2g18?
(In reply to Ian Melven :imelven from comment #39)
> Comment on attachment 707230 [details] [diff] [review]
> patch v3
> 
> nomination for both b2g18 and esr17 :
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): regression by Bug 737064
> User impact if declined:  CSP sources of *.foo.com will incorrectly allow
> all hosts
> Testing completed: has been on m-c since Feb 3rd, uplifted to aurora on Feb
> 4th, no new CSP bug reports as of yet, added CSP tests around this bug with
> the m-c landing
> Risk to taking this patch (and alternatives if risky): seems pretty stable
> in terms of bake time, may change B2G behavior if apps use a source of
> *.something.com and something is dependency on content being incorrectly
> allowed which would now be blocked.
> String or UUID changes made by this patch: none
> 
> Risk of not taking this patch: CSP will function incorrectly and won't block
> sources that should be blocked, reducing the protection it offers.

Approving for both ESR17 and B2G18, given the pretty low risk of regression here. Jason, do you know who may want to get the heads up on this on the Marketplace side given above risk to 3rd party devs?
Flags: needinfo?(jsmith)
Attachment #707230 - Flags: approval-mozilla-esr17?
Attachment #707230 - Flags: approval-mozilla-esr17+
Attachment #707230 - Flags: approval-mozilla-b2g18?
Attachment #707230 - Flags: approval-mozilla-b2g18+
I just gave the heads up to Lisa (app review lead) that this bug could have potential risk for third-party app breakage.
Flags: needinfo?(jsmith)
Added some people to warn them of third-party app risks. Feel free to send questions to Ian on what things to watch out for.
And backed out on ESR17 for xpcshell failures.
https://hg.mozilla.org/releases/mozilla-esr17/rev/40514f725fe8

https://tbpl.mozilla.org/php/getParsedLog.php?id=20498349&tree=Mozilla-Esr17

TEST-PASS | /builds/slave/talos-slave/test/build/xpcshell/tests/content/base/test/unit/test_csputils.js | [test_CSPSourceList_permits : 305] true == true

TEST-PASS | /builds/slave/talos-slave/test/build/xpcshell/tests/content/base/test/unit/test_csputils.js | [test_CSPSourceList_permits : 308] true == true

TEST-PASS | /builds/slave/talos-slave/test/build/xpcshell/tests/content/base/test/unit/test_csputils.js | [test_CSPSourceList_permits : 311] false == false

TEST-PASS | /builds/slave/talos-slave/test/build/xpcshell/tests/content/base/test/unit/test_csputils.js | [test_CSPSourceList_permits : 313] false == false

TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/content/base/test/unit/test_csputils.js | false == true - See following stack:
JS frame :: /builds/slave/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 451
JS frame :: /builds/slave/talos-slave/test/build/xpcshell/head.js :: _do_check_eq :: line 545
JS frame :: /builds/slave/talos-slave/test/build/xpcshell/head.js :: do_check_eq :: line 566
JS frame :: /builds/slave/talos-slave/test/build/xpcshell/head.js :: do_check_true :: line 580
JS frame :: /builds/slave/talos-slave/test/build/xpcshell/tests/content/base/test/unit/test_csputils.js :: test_CSPSourceList_permits :: line 316
JS frame :: /builds/slave/talos-slave/test/build/xpcshell/tests/content/base/test/unit/test_csputils.js :: run_test :: line 763
JS frame :: /builds/slave/talos-slave/test/build/xpcshell/head.js :: _execute_test :: line 315
JS frame :: -e :: <TOP_LEVEL> :: line 1

TEST-INFO | (xpcshell/head.js) | exiting test
status-firefox-esr17: fixed → affected
Attachment #707230 - Flags: approval-mozilla-esr17+
(In reply to Jason Smith [:jsmith] from comment #42)
> Added some people to warn them of third-party app risks. Feel free to send
> questions to Ian on what things to watch out for.

The situation I'm concerned about here is : 

1) app specifies CSP in its manifest, with a source of form *.example.com
(not sure how many if any apps are specifying their own CSP currently)

2) app expects loading something from some other site not whitelisted in CSP to be blocked

3) the load is not blocked due to this bug 

Probably pretty unlikely overall. 

I'll try to take a look at the ESR17 xpcshell problems when I have a minute.
(In reply to Ian Melven :imelven from comment #45)
> (In reply to Jason Smith [:jsmith] from comment #42)
> > Added some people to warn them of third-party app risks. Feel free to send
> > questions to Ian on what things to watch out for.
> 
> The situation I'm concerned about here is : 
> 
> 1) app specifies CSP in its manifest, with a source of form *.example.com
> (not sure how many if any apps are specifying their own CSP currently)
> 
> 2) app expects loading something from some other site not whitelisted in CSP
> to be blocked
> 
> 3) the load is not blocked due to this bug 
> 
> Probably pretty unlikely overall. 
> 

Actually, the case I'm thinking about is better stated as :

2) app 'works' since it can load from any source (*.example.com is incorrectly parsed as *)
3) this bug gets fixed - now csp is blocking something (correctly) that it wasn't before and app 'doesn't work'
Backport the existing patch to esr17. test_csputils.js passes locally now.

[Approval Request Comment]
Please see comment 39 and comment 40 - I'm renominating with a esr17 specific patch.
Attachment #724632 - Flags: approval-mozilla-esr17?
Attachment #724632 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
You need to log in before you can comment on or make changes to this bug.