We should allow putting a "reportOnlyCSP: true" switch in the app manifest. When this is set, we would switch the CSP policy to be in "report only" mode. (report-only mode is supported by our current CSP implementation, but can't be triggered for manifest-declared CSP policies and app-default policies right now). We need this for two reasons: 1. Right now it's very hard for us to fix bug 797657. The problem is that we need to fix all apps to follow that policy before we can flip the switch. And make sure to not accidentally regress any app to break the policy while we are doing that. This is extra hard since there is absolutely no indication of breaking the desired policy. 2. Allow developers that want to develop a "privileged" app to have a report-only policy while they are doing development. We should of course never let the marketplace sign an app which still has the "reportOnlyCSP" flag set to true. This is for developers only. Ideally we should even reject installing an app which has "reportOnlyCSP" set to true unless the phone is in developer mode.

CSP violations generate errors (and warnings) that show up in the developer console in Firefox. Do we not have an equivalent in B2G?

(In reply to Lucas Adamski from comment #1) > CSP violations generate errors (and warnings) that show up in the developer > console in Firefox. Do we not have an equivalent in B2G? Yes, they do generate errors on the console also. At least on the B2G desktop they do. While I agree with Jonas on the difficulty of just flipping the global switch for bug 797657, that's why Carmen implemented a solution for bug 773891. As far as I'm concerned, a better solution for this problem is: * Land the patch for bug 773891. * Indicate to Gaia app developers that they should add a line saying: "csp": "default-src *; script-src 'self'; object-src 'none'; style-src 'self'", to their manifests And that's all. Once they do that, everything that can fail on their app (and only their app) will fail, they can fix it, and land the changes. Once the changes are landed they can't regress unintentionally since any regression will make their app stop working unless they remove the csp from the manifest. That's actually what we're doing at TEF, but it's a pain currently since we have to make a custom image including the patch from bug 773891.

(In reply to Lucas Adamski from comment #1) > CSP violations generate errors (and warnings) that show up in the developer > console in Firefox. Do we not have an equivalent in B2G? We do, but the problem is that we can't flip the switch without completely breaking gaia. So we're currently running with a very permissive policy. As long as we have that permissive policy, adding more code which isn't following the policy that we want to have doesn't result in any warnings anywhere.

(In reply to Jonas Sicking (:sicking) from comment #3) > (In reply to Lucas Adamski from comment #1) > > CSP violations generate errors (and warnings) that show up in the developer > > console in Firefox. Do we not have an equivalent in B2G? > > We do, but the problem is that we can't flip the switch without completely > breaking gaia. So we're currently running with a very permissive policy. As > long as we have that permissive policy, adding more code which isn't > following the policy that we want to have doesn't result in any warnings > anywhere. As I said before, we can get the same benefits for incremental adaptation of Gaia to the definitive policy by landing bug 773891, which has the advantage of being already implemented :).

This will help devs, so on the nice-to-have fence.

If this is needed, we can take this, but I would still like to land the patch for bug 773891 before tackling this, because I think it'll be more useful on the short run to modify the apps, and because I think we're going to have to modify the same files (if not the same lines even) and it would be nice to modify over something that's definitive :). That said, I see the value on having a report only policy for installed apps, but not so much for certified and privileged, since I take that on end user devices the default policy should take precedence over anything a manifest say, shouldn't it? What is the way this should work in relation to the default policies? Should it be something only for development devices or for end user devices too? Should it be controlled by another preference (something like allowCSPOverride)?

I don't think we need a new setting for this. Using the normal "developer mode" setting would be fine. When reportOnlyCSP is set it would make both default policies as well as the policies declared using the bug 773891 mechanism report-only.

Taking this one

I was going to start working on this, but I would prefer to use the changes of bug 773891 as base for this one. That's why I'm marking this as dependent of 773891.

I have a first version working, but I've run into a little hitch that I don't know if I should fix or not. As CSP is currently implemented, when the CSP is marked reportOnly it does report the violations that are raised from trying to load a forbidden resource. Thus, if the policy states: script: 'self' and the page tries to load an script from other source, the error is logged on both the console and the report-uri if a report-uri is specified. But that doesn't work for inline scripts and evals. Inline scripts and evals violations aren't reported from contentSecurityPolicy.js but they're reported from nsScriptLoader::ProcessScriptElement(nsIScriptElement *aElement) and nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction(JSContext *cx) respectively. And to do that they call a function from contentSecurityPolicy.js that returns true always if reportOnlyMode is set. In short that means that as it currently stands, when a policy is marked as report only, the inline scripts violation (and evals) aren't reported, ever. So... do I fix that too? Should that be another bug? Or is it working as intended?

This WIP patch implements the required change, but inline reporting doesn't work, check comment 10

(In reply to Carmen Jiménez Cabezas from comment #10) > > So... do I fix that too? Should that be another bug? Or is it working as > intended? sounds like bug 687086 ? (which is pretty close to landing...)

(In reply to Ian Melven :imelven from comment #12) > (In reply to Carmen Jiménez Cabezas from comment #10) > > > > So... do I fix that too? Should that be another bug? Or is it working as > > intended? > > sounds like bug 687086 ? (which is pretty close to landing...) Cool, that's exactly it. Thank you! I'll set this one as dependent of bug 687086 then.

Comment on attachment 675511 [details] [diff] [review] WIP: Implemented the report only mode for apps CSPs Review of attachment 675511 [details] [diff] [review]: ----------------------------------------------------------------- I don't think I should be the one reviewing this.

This version is complete and includes the automatic tests

Comment on attachment 676681 [details] [diff] [review] Initial patch version Review of attachment 676681 [details] [diff] [review]: ----------------------------------------------------------------- This patch, plus the stuff from bug 773891 makes InitCSP() hard to read. Could you do a little restructuring so that there's an additional section for pulling CSP stuff out of the manifest? This would require a new local var to keep the manifest-specified CSP separate from the header so it's clearer that the "cspHeaderValue" is actually from the header and not from the manifest. ::: build/automation.py.in @@ +305,5 @@ > "manifestURL": "$manifestURL", > "localId": $localId, > "appStatus": $appStatus, > +"csp": "$csp", > +"cspReportOnly": "$cspReportOnly" Since this is a boolean, please make the manifest name it more obviously named, such as "cspIsReportOnly" or "makeCspReportOnly". (change in other files too please). ::: content/base/src/nsDocument.cpp @@ +2297,5 @@ > NS_SUCCEEDED(principal->GetAppStatus(&appStatus))) { > applyAppDefaultCSP = ( appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || > appStatus == nsIPrincipal::APP_STATUS_CERTIFIED); > > // Bug 773981. Allow a per-app policy from the manifest. While you're editing this function, please fix this bug number in the comment. Should be 773891. Sorry I didn't catch this earlier. @@ +2313,5 @@ > + appsService->GetCSPReportOnlyByLocalId(appId, &cspReportOnly); > + csp->SetReportOnlyMode(cspReportOnly && > + (appStatus == nsIPrincipal::APP_STATUS_INSTALLED || > + (applyAppDefaultCSP && > + Preferences::GetBool("dom.mozApps.dev_mode",false)))); This logic is hard to parse... seems to me that report only mode is set to true in two cases: 1. when the manifest says so for an INSTALLED app 2. the document is privileged/certified and running in dev_mode. This feels a bit forced, and is awkward to read... can you restructure this function to have a separate section for the manifest-based CSP added by bug 773891? (Suggested in the overview note in this review). Nit: trailing whitespace.

(In reply to Sid Stamm [:geekboy] from comment #16) > Comment on attachment 676681 [details] [diff] [review] > Initial patch version > > Review of attachment 676681 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch, plus the stuff from bug 773891 makes InitCSP() hard to read. > Could you do a little restructuring so that there's an additional section > for pulling CSP stuff out of the manifest? This would require a new local > var to keep the manifest-specified CSP separate from the header so it's > clearer that the "cspHeaderValue" is actually from the header and not from > the manifest. > Ok, I'm not sure I understand what you want me to do here. I tried to keep it as simple as possible and do only the minimum necessary changes. That's why I reused the variable, so I wouldn't have to touch where it was used. What I could do is to add a new variable for the variable CSP and another variable to hold the actual value that will be used. Then I could used initialized the second variable with the manifest value or the header value depending if one or the other should be used. Is this what you were referring to? > ::: build/automation.py.in > @@ +305,5 @@ > > "manifestURL": "$manifestURL", > > "localId": $localId, > > "appStatus": $appStatus, > > +"csp": "$csp", > > +"cspReportOnly": "$cspReportOnly" > > Since this is a boolean, please make the manifest name it more obviously > named, such as "cspIsReportOnly" or "makeCspReportOnly". (change in other > files too please). > You're right, I'll change this. > ::: content/base/src/nsDocument.cpp > @@ +2297,5 @@ > > NS_SUCCEEDED(principal->GetAppStatus(&appStatus))) { > > applyAppDefaultCSP = ( appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || > > appStatus == nsIPrincipal::APP_STATUS_CERTIFIED); > > > > // Bug 773981. Allow a per-app policy from the manifest. > > While you're editing this function, please fix this bug number in the > comment. Should be 773891. Sorry I didn't catch this earlier. > Ok, sorry I didn't notice that > @@ +2313,5 @@ > > + appsService->GetCSPReportOnlyByLocalId(appId, &cspReportOnly); > > + csp->SetReportOnlyMode(cspReportOnly && > > + (appStatus == nsIPrincipal::APP_STATUS_INSTALLED || > > + (applyAppDefaultCSP && > > + Preferences::GetBool("dom.mozApps.dev_mode",false)))); > > This logic is hard to parse... seems to me that report only mode is set to > true in two cases: > 1. when the manifest says so for an INSTALLED app > 2. the document is privileged/certified and running in dev_mode. > Yes, exactly that. Well, in fact the second condition is when the default CSP should be applied and the device is running in dev_mode. That's why I used the applyAppDefaultCSP on the expression. > This feels a bit forced, and is awkward to read... can you restructure this > function to have a separate section for the manifest-based CSP added by bug > 773891? (Suggested in the overview note in this review). > As I said before I'm not sure about what exactly you want me to restructure. As it stands now there is no duplicate code. In fact, that was one of my targets when I wrote the changes: no duplicate calls of management CSP if it was possible. This way, for example, if the condition to set reportOnlyMode changes, it will only be necessary to change it in one place. > Nit: trailing whitespace. ok

(In reply to Sid Stamm [:geekboy] from comment #16) > Comment on attachment 676681 [details] [diff] [review] > Initial patch version > > Review of attachment 676681 [details] [diff] [review]: > ----------------------------------------------------------------- > > This patch, plus the stuff from bug 773891 makes InitCSP() hard to read. > Could you do a little restructuring so that there's an additional section > for pulling CSP stuff out of the manifest? This would require a new local > var to keep the manifest-specified CSP separate from the header so it's > clearer that the "cspHeaderValue" is actually from the header and not from > the manifest. > Ok, I'm not sure I understand what you want me to do here. I tried to keep it as simple as possible and do only the minimum necessary changes. That's why I reused the variable, so I wouldn't have to touch where it was used. What I could do is to add a new variable for the variable CSP and another variable to hold the actual value that will be used. Then I could used initialized the second variable with the manifest value or the header value depending if one or the other should be used. Is this what you were referring to? > ::: build/automation.py.in > @@ +305,5 @@ > > "manifestURL": "$manifestURL", > > "localId": $localId, > > "appStatus": $appStatus, > > +"csp": "$csp", > > +"cspReportOnly": "$cspReportOnly" > > Since this is a boolean, please make the manifest name it more obviously > named, such as "cspIsReportOnly" or "makeCspReportOnly". (change in other > files too please). > You're right, I'll change this. > ::: content/base/src/nsDocument.cpp > @@ +2297,5 @@ > > NS_SUCCEEDED(principal->GetAppStatus(&appStatus))) { > > applyAppDefaultCSP = ( appStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || > > appStatus == nsIPrincipal::APP_STATUS_CERTIFIED); > > > > // Bug 773981. Allow a per-app policy from the manifest. > > While you're editing this function, please fix this bug number in the > comment. Should be 773891. Sorry I didn't catch this earlier. > Ok, sorry I didn't notice that > @@ +2313,5 @@ > > + appsService->GetCSPReportOnlyByLocalId(appId, &cspReportOnly); > > + csp->SetReportOnlyMode(cspReportOnly && > > + (appStatus == nsIPrincipal::APP_STATUS_INSTALLED || > > + (applyAppDefaultCSP && > > + Preferences::GetBool("dom.mozApps.dev_mode",false)))); > > This logic is hard to parse... seems to me that report only mode is set to > true in two cases: > 1. when the manifest says so for an INSTALLED app > 2. the document is privileged/certified and running in dev_mode. > Yes, exactly that. Well, in fact the second condition is when the default CSP should be applied and the device is running in dev_mode. That's why I used the applyAppDefaultCSP on the expression. > This feels a bit forced, and is awkward to read... can you restructure this > function to have a separate section for the manifest-based CSP added by bug > 773891? (Suggested in the overview note in this review). > As I said before I'm not sure about what exactly you want me to restructure. As it stands now there is no duplicate code. In fact, that was one of my targets when I wrote the changes: no duplicate calls of management CSP if it was possible. This way, for example, if the condition to set reportOnlyMode changes, it will only be necessary to change it in one place. > Nit: trailing whitespace. ok

Sorry for the delay, I've been out of the office a bit since the 15th. (In reply to Carmen Jiménez Cabezas from comment #18) > As I said before I'm not sure about what exactly you want me to restructure. > As it stands now there is no duplicate code. In fact, that was one of my > targets when I wrote the changes: no duplicate calls of management CSP if it > was possible. This way, for example, if the condition to set reportOnlyMode > changes, it will only be necessary to change it in one place. I think I understand your reasons here, but the code is still hard to read (and will be hard to change if we need to change it). The way I was thinking we could make it easier to read would be: 1. make a new variable for the manifest-specified csp (maybe cspFromAppManifest) and one for the "reportOnly" flag in the manifest (maybe appIsReportOnly) 2. in the "return-early" if test, add a check for the cspFromAppManifest.IsEmpty() 3. after processing the default policy, add a section that processes cspFromAppManifest (and uses appIsReportOnly flag with csp->SetReportOnlyMode() if necessary). This way, there would be four distinct if blocks and yeah, some duplicated code but it makes it much easier to read because the logic would be: 1. Get all the data from manifest and headers 2. If there's no policy to apply, return early 3. Apply any CSP that's app-default 4. Apply any CSP data (policy & reportOnly flag) from the app manifests 5. Apply any CSP from the headers 6. Apply any CSP report-only from the headers It makes it tremendously easier to read and also easier to write-in warnings for conflicts (such as when there are HTTP headers *and* an app default policy, or like the case when there's a report-only header and a regular one at the same time). Another approach would be to create two branches of InitCSP: one for apps and one for other documents. This might be cleaner (and you could have two separate functions) but would cause more duplication of code.

(In reply to Sid Stamm [:geekboy] from comment #19) > Another approach would be to create two branches of InitCSP: one for apps > and one for other documents. This might be cleaner (and you could have two > separate functions) but would cause more duplication of code. I like this approach, I think it's much cleaner to untangle the app-only and other document code in InitCSP and would lead to easier to understand code, especially with the forthcoming changes in bug 783049

Bug 797657, one of the rationales for implementing this has since been fixed. Does this still need to block?

That would allow us to make UITest app to work again (I hoped that this bug will eventually be fixed someday while landing bug 797657), and ease app development (2nd point of the original bug description).

(In reply to Alexandre Poirot (:ochameau) from comment #22) > That would allow us to make UITest app to work again (I hoped that this bug > will eventually be fixed someday while landing bug 797657), > and ease app development (2nd point of the original bug description). I'd imagine this could also be useful for third-party app developers building privileged apps, right?

I'm working on making the changes requested on the review now. I hope to have another version for review this week.

Minusing per comment 5. We wouldn't hold the release if we didn't have this.

As requested, I've separated the code in two blocks (of course the rest of comments are done too). Nonetheless, although I've done the changes, I still think the original version was better than current one. Now there is too much duplicated code and this will make harder to support the code, IMO.

Comment on attachment 690490 [details] [diff] [review] V2 including requested changes Drive-by review comments: +void +nsDocument::RefinePolicy(nsIContentSecurityPolicy* aCsp, + const NS_ConvertASCIItoUTF16& aCspValue, nsIURI* aChanURI + #ifdef PR_LOGGING + , const char * logMsg + #endif + ){ Please leave the logMsg argument present even if PR_LOGGING is not defined. Just have the callers of this method pass in null if PR_LOGGING is not defined, and just don't use the argument in that case as well, and comment to that effect above this method. Also, please put the opening '{' on its own line per the surrounding style in this file. - In nsDocument::InitCSPFromHttp(): // ----- if there's a full-strength CSP header, apply it. if (!cspHeaderValue.IsEmpty()) { - // Need to tokenize the header value since multiple headers could be - // concatenated into one comma-separated list of policies. - // See RFC2616 section 4.2 (last paragraph) - nsCharSeparatedTokenizer tokenizer(cspHeaderValue, ','); - while (tokenizer.hasMoreTokens()) { - const nsSubstring& policy = tokenizer.nextToken(); - csp->RefinePolicy(policy, chanURI); + RefinePolicy(csp, cspHeaderValue, chanURI #ifdef PR_LOGGING - { - PR_LOG(gCspPRLog, PR_LOG_DEBUG, - ("CSP refined with policy: \"%s\"", - NS_ConvertUTF16toUTF8(policy).get())); - } + , "CSP refined with policy: \"%s\"" #endif - } + ); } Please define a new macro in this file and call it something like REFINE_POLICY_LOG_MSG, and have that macro take the log message as an argument and evaluate to that if PR_LOGGING is defined, and nullptr if PR_LOGGING is not defined. Use the same macro in all callers of nsDocument::RefinePolicy(). r- to have that cleaned up, but leaving r? for sid to look at the rest of this patch...

Comment on attachment 690490 [details] [diff] [review] V2 including requested changes Review of attachment 690490 [details] [diff] [review]: ----------------------------------------------------------------- Carmen - thanks for your hard work on this. I'm giving you lots of detailed comments here in hopes of making it more readable so the duplicated code is (as you were concerned) easier to maintain. It will get there. ::: content/base/src/nsDocument.cpp @@ +2305,3 @@ > #endif > return NS_OK; > } Please move this return-early check before the initialization of the csp variable, so we only create a csp instance when necessary. @@ +2338,5 @@ > "ReportOnlyCSPIgnored"); > #ifdef PR_LOGGING > PR_LOG(gCspPRLog, PR_LOG_DEBUG, > ("Skipped report-only CSP init for document %p because another, enforced policy is set", this)); > #endif I don't think I needed the #ifdef around this since there's no extra work done before/after this macro in opt builds. Would you please remove it while you're editing this block? @@ +2350,4 @@ > #endif > + ); > + > + } Nit: add a space of indentation here, please. @@ +2390,5 @@ > +{ > + > + nsCOMPtr<nsIContentSecurityPolicy> csp; > + > + NS_ConvertASCIItoUTF16 cspValue(""); I think you can just use nsAutoString as a type for cspValue here, avoiding this converter function. @@ +2405,5 @@ > +#ifdef PR_LOGGING > + PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("Failed to create CSP object: %x", rv)); > +#endif > + return rv; > + } I believe if you instead use: NS_ASSERTION(csp, "Failed to create CSP object"); NS_ENSURE_SUCCESS(rv, rv); here, the string won't be created in opt builds and you don't need the #ifdef directive. @@ +2411,5 @@ > + applyAppDefaultCSP = ( aAppStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || > + aAppStatus == nsIPrincipal::APP_STATUS_CERTIFIED); > + // Bug 773891. Allow a per-app policy from the manifest. > + // Just read the CSP from the manifest into cspValue > + if (applyAppDefaultCSP || aAppStatus == nsIPrincipal::APP_STATUS_INSTALLED) { Can we assume, that if this InitCSPFromApp() function was called that aAppStatus is privileged, certified or installed? This function is basically a nop if the app status isn't one of those three. @@ +2416,5 @@ > + > + nsCOMPtr<nsIAppsService> appsService = do_GetService(APPS_SERVICE_CONTRACTID); > + if (appsService) { > + > + uint32_t appId; Nit: trailing whitespace (please check the rest of the patch for it too). @@ +2422,5 @@ > + bool cspIsReportOnly = false; > + appsService->GetCSPIsReportOnlyByLocalId(appId, &cspIsReportOnly); > + csp->SetReportOnlyMode(cspIsReportOnly && > + (aAppStatus == nsIPrincipal::APP_STATUS_INSTALLED || > + (applyAppDefaultCSP && Please move all the report-only mode stuff to the bottom of this function, after the return-early block and after refining the app's CSP with both the app default and any manifest policy. This way we don't have to create an instance of a nsIContentSecurityPolicy and don't even have to look for report-only info if there's no policy to apply. @@ +2446,5 @@ > + } > + > +#ifdef PR_LOGGING > + PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("Document is an app %p", this)); > +#endif I don't think you need the #ifdef around this since there's no extra work done before/after this macro in opt builds. @@ +2467,5 @@ > + if (appCSP) > + csp->RefinePolicy(appCSP, chanURI); > + } > + > + // ----- if there's a full-strength CSP header, apply it. Comment is probably wrong here -- it's not a header, it's a policy in the manifest, right? @@ +2493,5 @@ > + aChannel->Cancel(NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION); > + } > + } > + > + if (csp) { If we get to this point, csp won't be null (or we'll likely crash with a nullptr deref some lines earlier). If you add the assertion above, you don't need this check. @@ +2524,5 @@ > + uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; > + > + if (NS_SUCCEEDED(principal->GetUnknownAppId(&unknownAppId)) && > + !unknownAppId && > + NS_SUCCEEDED(principal->GetAppStatus(&appStatus))) { What should we do for apps that are _NOT_INSTALLED and have a HTTP-header-based CSP? I think we should follow the app path (like this code currently does), but I want to make sure we don't do this by accident. Do you agree, Carmen? @@ +2527,5 @@ > + !unknownAppId && > + NS_SUCCEEDED(principal->GetAppStatus(&appStatus))) { > + > + return InitCSPFromApp(aChannel, appStatus); > + }else{ Nit: please put spaces around else.

(In reply to Sid Stamm [:geekboy] from comment #28) > @@ +2338,5 @@ > > "ReportOnlyCSPIgnored"); > > #ifdef PR_LOGGING > > PR_LOG(gCspPRLog, PR_LOG_DEBUG, > > ("Skipped report-only CSP init for document %p because another, enforced policy is set", this)); > > #endif > > I don't think I needed the #ifdef around this since there's no extra work > done before/after this macro in opt builds. Would you please remove it > while you're editing this block? > @@ +2446,5 @@ > > + } > > + > > +#ifdef PR_LOGGING > > + PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("Document is an app %p", this)); > > +#endif > > I don't think you need the #ifdef around this since there's no extra work > done before/after this macro in opt builds. Yeah, ignore these two comments. We want the #ifdefs there 'cause logging is enabled in opt builds of Firefox.

Sorry for the late answer, I've just come back from my holidays. I'm modifying this, but there has been a patch for nsDocument.cpp that has broken my patch so I'm rebasing the changes as they were first. I hope to have a new version on a couple of days

:: content/base/src/nsDocument.cpp @@ +2390,5 @@ >> +{ >> + >> + nsCOMPtr<nsIContentSecurityPolicy> csp; >> + >> + NS_ConvertASCIItoUTF16 cspValue(""); >I think you can just use nsAutoString as a type for cspValue here, avoiding this converter function. I've left this unchanged since this was mostly copied from existing code. I don't know if I use a nsAutoString if it'll automatically create an UTF16 string or not. If you think it should work that way, I can change it later @@ +2405,5 @@ >> +#ifdef PR_LOGGING >> + PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("Failed to create CSP object: %x", rv)); >> +#endif >> + return rv; >> + } >I believe if you instead use: > NS_ASSERTION(csp, "Failed to create CSP object"); > NS_ENSURE_SUCCESS(rv, rv); >here, the string won't be created in opt builds and you don't need the #ifdef directive. Since by your comment 29 logging is enabled in opt builds of Firefox, I've left this untouched also @@ +2411,5 @@ >> + applyAppDefaultCSP = ( aAppStatus == nsIPrincipal::APP_STATUS_PRIVILEGED || >> + aAppStatus == nsIPrincipal::APP_STATUS_CERTIFIED); >> + // Bug 773891. Allow a per-app policy from the manifest. >> + // Just read the CSP from the manifest into cspValue >> + if (applyAppDefaultCSP || aAppStatus == nsIPrincipal::APP_STATUS_INSTALLED) { >Can we assume, that if this InitCSPFromApp() function was called that aAppStatus is privileged, certified or installed? This function is basically a nop if the app status isn't one of those three. Currently only privileged, certified and installed apps can have manifests and as such a manifest-defined CSP. But you're right, I've restructured the function so everything is inside the if. The function just returns NS_OK if it the principal isn't an app. I could move the if outside the function but since the application type could change tomorrow, it seems cleaner to me this way. The function is the one that knows the types of apps and the logic for each of them @@ +2524,5 @@ >> + uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; >> + >> + if (NS_SUCCEEDED(principal->GetUnknownAppId(&unknownAppId)) && >> + !unknownAppId && >> + NS_SUCCEEDED(principal->GetAppStatus(&appStatus))) { >What should we do for apps that are _NOT_INSTALLED and have a HTTP-header-based CSP? I think we should follow the app path (like this code currently does), but I want to make sure we don't do this by accident. Do you agree, Carmen? Hmm... if the app isn't installed then it'll be loaded by http, not by app, and thus it should enter the InitCSPFromHttp path, not the app path. I've changed the if to take this into account

So I haven't forgotten about this bug. I'm waiting for the 1.0 version of the CSP parser to land (bug 663566) before we make more changes to the parser entry points. Should be soon.

Just updating this bug again... still haven't forgotten. 1.0 of the CSP parser landed, so this might need to be un-bitrotted. Lets hold off a little longer to implement multi-policy support (I want to get rid of refinePolicy) first. That will make this patch simpler, and will also de-obfuscate nsDocument::InitCSP().

Bug 836922 calls for supporting multiple policies (instead of combining them via intersection). We're making forward progress on that and it will make this work much much simpler. This bug is still on my radar and I'll follow up when we land bug 836922 and try to get this done quickly thereafter.

Now that 836922 landed, the patch for this one will be simpler, especially the bits in nsDocument. Carmen, do you want to rewrite the relevant bits of the patch here or should I take it?

After this time, to be honest I would have to start from scratch, I don't remember anything about it :). So if you can/want to take it by all means go ahead!

Thanks, Carmen. Sorry it took so long; the new multi-policy backend fixed a ton of bugs and was badly needed.

Comment on attachment 704696 [details] [diff] [review] V3 including the review requested changes Clearing the review flags for now. We'll tackle this again after we catch up on some other CSP stuff that needs doing.

jonas, paul, do we still want this?

No