994322 - CSP in C++: implement violation reporting

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Sid Stamm [:geekboy or :sstamm]]

AsyncReportViolation, console logging, etc.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: violationReporting

This is a first patch.  I'm going to wait on reviews since it will likely need to be rebased/modified when the patches on bug 951457 are updated.

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

I need to update this patch to not use mPrincipal for the content policy check in sendReports, and tweak a couple other things before review.  Hopefully I'll get that done soon and can upload a new patch for review today.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: violationReporting

Spoke too soon.  Patch in Bug 994466 fixes the mPrincipal thing, no reason to roll it into this (we can just land them both at the same time).  Should be ready for review.

This patch corresponds to the one in cset 858f1352912b in the queue repository (http://hg.mozilla.org/users/sstamm_mozilla.com/csp-rewrite-patches/file/858f1352912b)

Comment 4

[Garrett Robinson [:grobinson]]

Comment on attachment 8418734 [details] [diff] [review]
violationReporting

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

::: content/base/src/nsCSPContext.cpp
@@ +191,5 @@
> +        this->AsyncReportViolation(aContentLocation,
> +                                   aRequestOrigin,
> +                                   violatedDirective,
> +                                   p,             /* policy index        */
> +                                   EmptyString(), /* no observer subject */

Can we consolidate the observer reporting above into this function? It doesn't make sense to have separate functions that do the same thing for external vs inline checks.

@@ +430,5 @@
>                                    int32_t aLineNum,
>                                    const nsAString& aNonce,
>                                    const nsAString& aContent)
>  {
> +  for (uint32_t p = 0; p < mPolicies.Length(); p++) {

Is there a followup bug to track experimenting with ways to do violation reporting without having to recheck the policies?

@@ +607,5 @@
> +    if (httpChannel) {
> +      httpChannel->GetReferrer(getter_AddRefs(mReferrer));
> +    }
> +    else {
> +      // probably log a warning

Resolve this TODO

@@ +703,5 @@
> +    csp_report.AppendInt(aLineNum);
> +      csp_report.AppendASCII("\"");
> +  }
> +
> +  csp_report.AppendASCII("}}\n\n");

There must be a better (more readable and extensible, less repetitive) way to assemble the report JSON.

@@ +727,5 @@
> +
> +      CSP_LogMessage(logMsg, EmptyString(), EmptyString(),
> +                     0, 0, nsIScriptError::errorFlag,
> +                     "CSP", mInnerWindowID);
> +      // 'errorWas' from csp.properties is not used anymore,

Is this a reference to the old code? Confusing here without more context, maybe unnecessary?

@@ +741,5 @@
> +    }
> +
> +    // make sure this is an anonymous request (no cookies) so in case the
> +    // policy URI is injected, it can't be abused for CSRF.
> +    //  chan.loadFlags |= Ci.nsIChannel.LOAD_ANONYMOUS;

This reference to the old code is unnecessary (now that the new code is finished)

@@ +753,5 @@
> +    // we need to set an nsIChannelEventSink on the channel object
> +    // so we can tell it to not follow redirects when posting the reports
> +    nsRefPtr<CSPReportRedirectSink> reportSink = new CSPReportRedirectSink();
> +    reportChannel->SetNotificationCallbacks(reportSink);
> +    // what about the loadgroup?

Well, what about the loadgroup? :) Do we need it?

@@ +799,5 @@
> +    nsRefPtr<CSPViolationReportListener> listener = new CSPViolationReportListener();
> +    rv = reportChannel->AsyncOpen(listener, nullptr);
> +
> +    if (NS_FAILED(rv)) {
> +      // maybe we should log the error?

Should log this, could be useful debugging failures to send reports.

@@ +834,5 @@
> +    {
> +      // the observer subject is an nsISupports: either an nsISupportsCString
> +      // from the arg passed in directly, or if that's empty, it's the blocked
> +      // source.
> +      if (aObserverSubject.Length() < 1) {

aObserverSubject.isEmpty() is a little clearer IMHO

@@ +903,5 @@
> +                            getter_Copies(logMsg));
> +
> +        CSP_LogMessage(logMsg, EmptyString(), EmptyString(),
> +                     0, 0, nsIScriptError::errorFlag,
> +                     "CSP", mInnerWindowID);

This "GetLocalizedStr, then LogMessage" pattern is common enough that it seems it should be encapsulated it in its own function.

@@ +1091,5 @@
> +                      uint32_t count,
> +                      uint32_t* writeCount)
> +{
> +  nsCString* decodedData = static_cast<nsCString*>(closure);
> +  decodedData->Append(rawSegment, count);

Can this fail? (this code gives me the heebie jeebies)

::: content/base/src/nsCSPContext.h
@@ +69,5 @@
>      nsTArray<nsCSPPolicy*> mPolicies;
>      nsCOMPtr<nsIURI>       mSelfURI;
> +    nsCOMPtr<nsIURI>       mReferrer;
> +    nsCOMPtr<nsIPrincipal> mPrincipal;
> +    uint64_t               mInnerWindowID; /* used for console logging */

clarify: web console logging. You don't need the inner window ID to log to the Browser Console, but you do need it to direct reports to a specific window's Web Console.

@@ +88,5 @@
> +};
> +
> +// The POST of the violation report (if it happens) should not follow
> +// redirects, per the spec. hence, we implement an nsIChannelEventSink
> +// with an object so we can tell XHR to abort if a redirect happens.

Great comment!

::: content/base/src/nsCSPUtils.cpp
@@ +674,5 @@
> +
> +  // append uris
> +  nsString tmpReportURI;
> +  for (uint32_t i = 0; i < mSrcs.Length(); i++) {
> +    tmpReportURI = NS_LITERAL_STRING("");

Nit: you use .Truncate() elsewhere to achieve the same goal. Would be nice to be consistent.

::: content/base/test/csp/test_CSP_bug916446.html
@@ +34,5 @@
>        if (!testpat.test(uri)) return;
>        var testid = testpat.exec(uri)[1];
>        if (testid === "img_bad") {
>          // img_bad should be *allowed* because the policy is report-only
> +        ok(true, "External loads should be allowed because the policy is report-only");

Good catch!

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8418734 [details] [diff] [review]
violationReporting

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

Even though I marked mostly minor nitpicks, I would like to see this one more time, mostly to see if we can simplify LogViolationDetails and the generation of the report-JSON. Otherwise  this is almost ready to land!

::: content/base/src/nsCSPContext.cpp
@@ +33,5 @@
>  #include "nsIWritablePropertyBag2.h"
>  #include "nsString.h"
>  #include "prlog.h"
> +#include "nsSupportsPrimitives.h"
> +#include "nsThreadUtils.h"

I can't remember how often I alphabetized the includes in nsCSPContext, for some reason they are always out of order. Maybe you can try and fix this again.

@@ +430,5 @@
>                                    int32_t aLineNum,
>                                    const nsAString& aNonce,
>                                    const nsAString& aContent)
>  {
> +  for (uint32_t p = 0; p < mPolicies.Length(); p++) {

Agreed, it would be great if we could fix the problem of finding the violated policy twice. Please file a follow-up, if not already happened.

@@ +526,5 @@
> +        default:
> +          NS_ASSERTION(false, "LogViolationDetails with invalied type");
> +          break;
> +      }
> +  }

This looks like a lot of duplicate code. Can we simplify that a little bit? At least we do not have to define nsAutoString violatedDirective in every case statement.

@@ +536,5 @@
> +   // Get the innerWindowID associated with the XMLHTTPRequest
> +  uint64_t id = 0;
> +  if (!aRequest)
> +    return id;
> +

I know this is not your fault actually, but since it's your patch, you gotta fix it. Can you add {} for all the if-statements?

@@ +539,5 @@
> +    return id;
> +
> +  nsCOMPtr<nsILoadGroup> loadGroup;
> +  aRequest->GetLoadGroup(getter_AddRefs(loadGroup));
> +  if (!loadGroup)

Maybe you want to add NS_ENSURE_SUCCESS to all of the these computations.

@@ +595,5 @@
> +    nsContentUtils::GetSecurityManager()->
> +      GetChannelPrincipal(aChannel, getter_AddRefs(mPrincipal));
> +  }
> +  else {
> +    // CSPdebug("No principal or channel for document context; violation reports may not work.");

Make this a PR_LOG or remove.

@@ +607,5 @@
> +    if (httpChannel) {
> +      httpChannel->GetReferrer(getter_AddRefs(mReferrer));
> +    }
> +    else {
> +      // probably log a warning

Yep, but I guess we do not need to log a warning because of the referrer.

@@ +631,5 @@
> +
> +  // assemble the JSON for reporting
> +  // TODO: include effective-directive, status-code, column-number
> +  // (http://www.w3.org/TR/CSP11/#reporting)
> +

Do we want to include those missing parts, otherwise delete that comment.

@@ +646,5 @@
> +    if (uri) {
> +      uri->GetSpec(reportBlockedURI);
> +    } else if (cstr) {
> +      cstr->GetData(reportBlockedURI);
> +    }

Only query the interface for aBlockedContentSource if the reportBlockedContentSource is still empty after GetSpec(reportBlockedURI).

@@ +665,5 @@
> +  csp_report.AppendASCII("\"original-policy\": \"");
> +  nsAutoString originalPolicy;
> +  rv = this->GetPolicy(aViolatedPolicyIndex, originalPolicy);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  csp_report.AppendASCII(NS_ConvertUTF16toUTF8(originalPolicy).get());

You can save one roundtrip of converting forth and back between UTF16 and UTF8 by calling csp_report.Append(originalPolicy);

@@ +700,5 @@
> +  // line-number
> +  if (aLineNum != 0) {
> +    csp_report.AppendASCII(", \"script-sample\": \"");
> +    csp_report.AppendInt(aLineNum);
> +      csp_report.AppendASCII("\"");

Seems like an indentation problem.

@@ +709,5 @@
> +  // ---------- Assembled, now send it to all the report URIs ----------- //
> +
> +  nsTArray<nsString> reportURIs;
> +  mPolicies[aViolatedPolicyIndex]->getReportURIs(reportURIs);
> +

Are we running in danger of indexing out of bounds? probably worth an assertion on top.

@@ +728,5 @@
> +      CSP_LogMessage(logMsg, EmptyString(), EmptyString(),
> +                     0, 0, nsIScriptError::errorFlag,
> +                     "CSP", mInnerWindowID);
> +      // 'errorWas' from csp.properties is not used anymore,
> +      // since we do not have an exception msg anymore.

Indeed, seems like part of the old code. We can delete that comment.

@@ +729,5 @@
> +                     0, 0, nsIScriptError::errorFlag,
> +                     "CSP", mInnerWindowID);
> +      // 'errorWas' from csp.properties is not used anymore,
> +      // since we do not have an exception msg anymore.
> +      return rv;

When returning here, aren't we possibly missing to send reports to correct URIs?

@@ +736,5 @@
> +    // try to create a new channel for every report-uri
> +    rv = NS_NewChannel(getter_AddRefs(reportChannel), reportURI);
> +    if (NS_FAILED(rv)) {
> +      // probably report error
> +      return rv;

Same here, should we really return or just skip over this one malformed URI?

@@ +758,5 @@
> +    // chan.notificationCallbacks = new CSPReportRedirectSink(policy);
> +    // if (this._weakDocRequest.get()) {
> +    //   chan.loadGroup = this._weakDocRequest.get().loadGroup;
> +    // }
> +

does the comment still describe a valid concern?

@@ +775,5 @@
> +
> +    if (shouldLoad != nsIContentPolicy::ACCEPT) {
> +      // skip unauthorized URIs
> +      // probably should report error
> +      return NS_OK;

Should we report an error here? If so, please do.

@@ +780,5 @@
> +    }
> +
> +    // wire in the string input stream to send the report
> +    nsCOMPtr<nsIStringInputStream> sis(do_CreateInstance(NS_STRINGINPUTSTREAM_CONTRACTID));
> +    NS_ASSERTION(sis, "must support nsIStringInputStream");

Please make the assertion text more descriptive.

@@ +785,5 @@
> +    rv = sis->SetData(NS_ConvertUTF16toUTF8(csp_report).get(), csp_report.Length());
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsIUploadChannel> uploadChannel(do_QueryInterface(reportChannel));
> +    NS_ASSERTION(uploadChannel, "must support uploadChannel");

same here.

@@ +849,5 @@
> +    NS_IMETHOD Run()
> +    {
> +      //TODO(sid): may have to deal with redirects, see also:
> +      //http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#1024
> +

Is that still a valid concern or legacy comment?

@@ +851,5 @@
> +      //TODO(sid): may have to deal with redirects, see also:
> +      //http://mxr.mozilla.org/mozilla-central/source/content/base/src/contentSecurityPolicy.js#1024
> +
> +      MOZ_ASSERT(NS_IsMainThread());
> +      nsresult rv;

Nit: Please always init variables; nsresult rv = NS_OK;

@@ +876,5 @@
> +        nsAutoCString blockedURIStr;
> +        blockedURI->GetSpec(blockedURIStr);
> +        nsString blockedURIChar16 = NS_ConvertUTF8toUTF16(blockedURIStr);
> +        const char16_t* params[] = { mViolatedDirective.get(),
> +                                     blockedURIChar16.get() };

No need to define blockedURIChar16, you can do the conversion inside the {}.

@@ +877,5 @@
> +        blockedURI->GetSpec(blockedURIStr);
> +        nsString blockedURIChar16 = NS_ConvertUTF8toUTF16(blockedURIStr);
> +        const char16_t* params[] = { mViolatedDirective.get(),
> +                                     blockedURIChar16.get() };
> +        CSP_GetLocalizedStr(NS_LITERAL_STRING("CSPViolationWithURI").get(),

Nit: I guess you can use NS_LITERAL_CSTRING.

@@ +884,5 @@
> +                            getter_Copies(logMsg));
> +
> +        CSP_LogMessage(logMsg, EmptyString(), EmptyString(),
> +                     0, 0, nsIScriptError::errorFlag,
> +                     "CSP", mInnerWindowID);

Nit: Indentation.

@@ +895,5 @@
> +        nsAutoCString data;
> +        blockedString->GetData(data);
> +        nsString blockedDataChar16 = NS_ConvertUTF8toUTF16(data);
> +        const char16_t* params[] = { mViolatedDirective.get(),
> +                                     blockedDataChar16.get() };

Same here, you can do the conversion inside {} without the need of blockedDataChar16.

@@ +896,5 @@
> +        blockedString->GetData(data);
> +        nsString blockedDataChar16 = NS_ConvertUTF8toUTF16(data);
> +        const char16_t* params[] = { mViolatedDirective.get(),
> +                                     blockedDataChar16.get() };
> +        CSP_GetLocalizedStr(NS_LITERAL_STRING("CSPViolationWithURI").get(),

Nit: Again, you can probably use NS_LITERAL_CSTRING

@@ +1088,5 @@
> +                      void* closure,
> +                      const char* rawSegment,
> +                      uint32_t toOffset,
> +                      uint32_t count,
> +                      uint32_t* writeCount)

Nit: please append 'a' and 'out' prefixes for arguments.

@@ +1101,5 @@
> +CSPViolationReportListener::OnDataAvailable(nsIRequest *aRequest,
> +                                            nsISupports *aContext,
> +                                            nsIInputStream *aInputStream,
> +                                            uint64_t aOffset,
> +                                            uint32_t aCount)

Nit: pointer belongs to the type.

@@ +1121,5 @@
> +}
> +
> +NS_IMETHODIMP
> +CSPViolationReportListener::OnStartRequest(nsIRequest *aRequest,
> +                                           nsISupports *aContext)

Nit: again, pointer belongs to the type.

@@ +1142,5 @@
> +NS_IMETHODIMP
> +CSPReportRedirectSink::AsyncOnChannelRedirect(nsIChannel* oldChannel,
> +                                              nsIChannel* newChannel,
> +                                              uint32_t redirFlags,
> +                                              nsIAsyncVerifyRedirectCallback *cb)

Nit: please use prefixes for arguments.

@@ +1145,5 @@
> +                                              uint32_t redirFlags,
> +                                              nsIAsyncVerifyRedirectCallback *cb)
> +{
> +  // probably log to console
> +  // this._policy.log(WARN_FLAG, CSPLocalizer.getFormatStr("reportPostRedirect", [oldChannel.URI.asciiSpec]));

you can remove this comment.

@@ +1148,5 @@
> +  // probably log to console
> +  // this._policy.log(WARN_FLAG, CSPLocalizer.getFormatStr("reportPostRedirect", [oldChannel.URI.asciiSpec]));
> +
> +  // cancel the old channel so XHR failure callback happens
> +  nsresult rv;

Nit: Please init rv; or better, move the next line up.

@@ +1159,5 @@
> +  rv = oldChannel->GetURI(getter_AddRefs(uri));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +  NS_ASSERTION(observerService, "no observer service");

Please update comment in the assertion to be more descriptive.

@@ +1163,5 @@
> +  NS_ASSERTION(observerService, "no observer service");
> +  observerService->NotifyObservers(uri,
> +                                   CSP_VIOLATION_TOPIC,
> +                                   NS_LITERAL_STRING("denied redirect while sending violation report").get());
> +

I guess you can use NS_LITERAL_CSTRING.

@@ +1168,5 @@
> +  return NS_BINDING_REDIRECTED;
> +}
> +
> +NS_IMETHODIMP
> +CSPReportRedirectSink::GetInterface(const nsIID & aIID, void **aResult)

& and ** goes to the type.

::: content/base/src/nsCSPContext.h
@@ +29,5 @@
> +#define SCRIPT_NONCE_VIOLATION_OBSERVER_TOPIC   "Inline Script had invalid nonce"
> +#define STYLE_NONCE_VIOLATION_OBSERVER_TOPIC    "Inline Style had invalid nonce"
> +#define SCRIPT_HASH_VIOLATION_OBSERVER_TOPIC    "Inline Script had invalid hash"
> +#define STYLE_HASH_VIOLATION_OBSERVER_TOPIC     "Inline Style had invalid hash"
> +

Can those be static const chars instead of defines? I guess we can even move them to the *.cpp file, right?

@@ +37,5 @@
>      NS_DECL_ISUPPORTS
>      NS_DECL_NSICONTENTSECURITYPOLICY
>      NS_DECL_NSISERIALIZABLE
>  
> +  public:

Remove that public: because we are already setting it above.

@@ +74,1 @@
>      nsDataHashtable<nsCStringHashKey, int16_t> mShouldLoadCache;

Nit: Can you align members with mShouldLoadCache?

@@ +81,5 @@
> +    NS_DECL_NSISTREAMLISTENER
> +    NS_DECL_NSIREQUESTOBSERVER
> +    NS_DECL_ISUPPORTS
> +
> +  public:

Same here, probably a merge issue, but you can remove this public:

@@ +97,5 @@
> +    NS_DECL_NSICHANNELEVENTSINK
> +    NS_DECL_NSIINTERFACEREQUESTOR
> +    NS_DECL_ISUPPORTS
> +
> +  public:

One public qualifier is enough :-)

::: content/base/src/nsCSPUtils.cpp
@@ +840,5 @@
> +                                              nsAString& outDirective) const
> +{
> +  for (uint32_t i = 0; i < mDirectives.Length(); i++) {
> +    if (mDirectives[i]->directiveNameEqualsContentType(aContentType)) {
> +      mDirectives[i]->toString(outDirective);

Can't we return early here?

@@ +846,5 @@
> +  }
> +}
> +
> +void
> +nsCSPPolicy::getReportURIs(nsTArray<nsString> &outReportURIs) const

Nit: Move & to the type.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Supplemental note: Please make sure to log warnings, or even better return errors in nsCSPContext::SendReports, whenever sending a report fails. Just found a problem with creating channels, which only occurs on B2G ICS Emulator.
Right now, sendReports returned NS_OK, even though AsyncOpen of the channel failed.

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8418734 [details] [diff] [review]
violationReporting

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

::: content/base/src/nsCSPContext.cpp
@@ +185,5 @@
>          NS_ASSERTION(observerService, "CSP requires observer service.");
>  
>          observerService->NotifyObservers(aContentLocation,
>                                           CSP_VIOLATION_TOPIC,
>                                           violatedDirective.get());

Self-review-note: remove this call to NotifyObservers (the following call to AsyncReportViolation will call it).

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

We are not incorporating the sourcefile, scriptsample and linenumber correctly when logging to the console. Please also update, the following codesnippet should help.

+++ b/content/base/src/nsCSPContext.cpp
@@ -871,19 +871,19 @@ class CSPReportSenderRunnable MOZ_FINAL 
         nsString blockedURIChar16 = NS_ConvertUTF8toUTF16(blockedURIStr);
         const char16_t* params[] = { mViolatedDirective.get(),
                                      blockedURIChar16.get() };
         CSP_GetLocalizedStr(NS_LITERAL_STRING("CSPViolationWithURI").get(),
                             params,
                             ArrayLength(params),
                             getter_Copies(logMsg));
 
-        CSP_LogMessage(logMsg, EmptyString(), EmptyString(),
-                     0, 0, nsIScriptError::errorFlag,
-                     "CSP", mInnerWindowID);
+        CSP_LogMessage(logMsg, mSourceFile, mScriptSample,
+                       mLineNum, 0, nsIScriptError::errorFlag,
+                       "CSP", mInnerWindowID);
         return NS_OK;
       }

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Actually, I did the change and pushed it to the queue-repo for you. You are welcome :-)

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> We are not incorporating the sourcefile, scriptsample and linenumber
> correctly when logging to the console.

Thanks, good catch.  I will also refactor a bit of the reportsender near that code.

And thanks for the change in the repo.  :)

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #4)
> Well, what about the loadgroup? :) Do we need it?

Yeah.  Attachment 8423195 [details] [diff] in bug 1006612 shows how to hook up the correct load group.  I'll do that as part of this bug (and will be in the next iteration of the patch here).

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

New patch coming shortly.  A few things...

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> @@ +526,5 @@
> > +        default:
> > +          NS_ASSERTION(false, "LogViolationDetails with invalied type");
> > +          break;
> > +      }
> > +  }
> 
> This looks like a lot of duplicate code. Can we simplify that a little bit?
> At least we do not have to define nsAutoString violatedDirective in every
> case statement.

I will factor this out with a macro.  It'll expand to the same code, but will not be typed that many times.  There are a lot of local vars, so making it a function (and not macro) requires passing nine arguments (yuck).

> @@ +539,5 @@
> > +    return id;
> > +
> > +  nsCOMPtr<nsILoadGroup> loadGroup;
> > +  aRequest->GetLoadGroup(getter_AddRefs(loadGroup));
> > +  if (!loadGroup)
> 
> Maybe you want to add NS_ENSURE_SUCCESS to all of the these computations.

The function doesn't return an nsresult.  We could rewrite the function to do that, but then the int return val would be an outparam, which is kind of annoying.  In any case, checking for null results for things like GetLoadGroup should be sufficient since we'd end up handling any nsresult the same way at the call site (get the error, frown, then continue processing because it's not a critical error having no innerWindowID).

> @@ +709,5 @@
> > +  // ---------- Assembled, now send it to all the report URIs ----------- //
> > +
> > +  nsTArray<nsString> reportURIs;
> > +  mPolicies[aViolatedPolicyIndex]->getReportURIs(reportURIs);
> > +
> 
> Are we running in danger of indexing out of bounds? probably worth an
> assertion on top.

An excuse to use NS_ENSURE_ARG_MAX!

> Nit: Please always init variables; nsresult rv = NS_OK;

> @@ +877,5 @@
> > +        blockedURI->GetSpec(blockedURIStr);
> > +        nsString blockedURIChar16 = NS_ConvertUTF8toUTF16(blockedURIStr);
> > +        const char16_t* params[] = { mViolatedDirective.get(),
> > +                                     blockedURIChar16.get() };
> > +        CSP_GetLocalizedStr(NS_LITERAL_STRING("CSPViolationWithURI").get(),
> 
> Nit: I guess you can use NS_LITERAL_CSTRING.

Nope.  The param type is const char16_t*, needs to be UTF16 and null-terminated.

> ::: content/base/src/nsCSPContext.h
> @@ +29,5 @@
> > +#define SCRIPT_NONCE_VIOLATION_OBSERVER_TOPIC   "Inline Script had invalid nonce"
> > +#define STYLE_NONCE_VIOLATION_OBSERVER_TOPIC    "Inline Style had invalid nonce"
> > +#define SCRIPT_HASH_VIOLATION_OBSERVER_TOPIC    "Inline Script had invalid hash"
> > +#define STYLE_HASH_VIOLATION_OBSERVER_TOPIC     "Inline Style had invalid hash"
> > +
> 
> Can those be static const chars instead of defines? I guess we can even move
> them to the *.cpp file, right?

I like them as defines since many observer topics are #defined in idls or .h files.  These can be used by other consumers of CSP data types if we end up with them in the future.  I will move them to nsCSPUtils.h (and they'll only get defined once).

> @@ +37,5 @@
> >      NS_DECL_ISUPPORTS
> >      NS_DECL_NSICONTENTSECURITYPOLICY
> >      NS_DECL_NSISERIALIZABLE
> >  
> > +  public:
> 
> Remove that public: because we are already setting it above.

Actually, this is a good thing to keep.  NS_DECL_* may expand to insert a private: block after the public: block, and we don't want the ctor/dtor to be accidentally private.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #4)
> Can we consolidate the observer reporting above into this function? It
> doesn't make sense to have separate functions that do the same thing for
> external vs inline checks.

Yeah, of course, in AsyncReportViolation.  :)  Actually, this is a duplicate observer notification, so it will get deleted.

> Is there a followup bug to track experimenting with ways to do violation
> reporting without having to recheck the policies?

I filed bug 1011098.

> There must be a better (more readable and extensible, less repetitive) way
> to assemble the report JSON.

There must be.  I don't know of a better one, and this isn't super-complex JSON.  If you find one, we can consider using it.

> This "GetLocalizedStr, then LogMessage" pattern is common enough that it
> seems it should be encapsulated it in its own function.

Good idea.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #13)
> New patch coming shortly.  A few things...
> > > +    return id;
> > > +
> > > +  nsCOMPtr<nsILoadGroup> loadGroup;
> > > +  aRequest->GetLoadGroup(getter_AddRefs(loadGroup));
> > > +  if (!loadGroup)
> > 
> > Maybe you want to add NS_ENSURE_SUCCESS to all of the these computations.
> 
> The function doesn't return an nsresult.  We could rewrite the function to
> do that, but then the int return val would be an outparam, which is kind of
> annoying.  In any case, checking for null results for things like
> GetLoadGroup should be sufficient since we'd end up handling any nsresult
> the same way at the call site (get the error, frown, then continue
> processing because it's not a critical error having no innerWindowID).

Agreed, we definitely don't want an outparam, but you can still use:
NS_ENSURE_SUCCESS(rv, id);
right? http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#331
 
> > @@ +37,5 @@
> > >      NS_DECL_ISUPPORTS
> > >      NS_DECL_NSICONTENTSECURITYPOLICY
> > >      NS_DECL_NSISERIALIZABLE
> > >  
> > > +  public:
> > 
> > Remove that public: because we are already setting it above.
> 
> Actually, this is a good thing to keep.  NS_DECL_* may expand to insert a
> private: block after the public: block, and we don't want the ctor/dtor to
> be accidentally private.

Oh then that was intentional. That is definitely good to know. Thanks for clarification!

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #4)
> @@ +1091,5 @@
> > +                      uint32_t count,
> > +                      uint32_t* writeCount)
> > +{
> > +  nsCString* decodedData = static_cast<nsCString*>(closure);
> > +  decodedData->Append(rawSegment, count);
> 
> Can this fail? (this code gives me the heebie jeebies)

So "closure" is created and opaquely passed through to this AppendSegmentToString callback via call to ReadSegments in OnDataAvailable (right by the referenced code).

We create the thing (an nsCString) and pass it through as a void*, so the static_cast should never fail.  The only iffy bit is if aCount is bigger than aRawSegment.  I'd have to dig into the Append() implementation to see if we could have a buffer overread here, but my hunch is that it's safe.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #15)
> Agreed, we definitely don't want an outparam, but you can still use:
> NS_ENSURE_SUCCESS(rv, id);
> right? http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#331

Oh, sure.  Of course.  :)

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

Attachment: violationReporting

Ok, addressed comments.  Round two.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8423459 [details] [diff] [review]
violationReporting

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

Awesome, this looks really good to me, especially the refactoring in LogViolationDetails and also CSPReportSenderRunnable. However, we should investigate why aContent is not used in LogViolationDetails. Left a mark in the code review; please let me just review that part again once we figured what to do.

::: content/base/src/nsCSPContext.cpp
@@ +293,5 @@
>  
>    if (aSelfURI) {
>      // aSelfURI will be disregarded since we will remove it with bug 991474
>      NS_WARNING("aSelfURI should be a nullptr in AppendPolicy and removed in bug 991474");
> +  } 

Nit: eliminate trailing whitespace!

@@ +462,5 @@
>                                    const nsAString& aSourceFile,
>                                    const nsAString& aScriptSample,
>                                    int32_t aLineNum,
>                                    const nsAString& aNonce,
>                                    const nsAString& aContent)

It seems like aContent is unused in this function body, are we missing something or can we eliminate that argument?

Found one call that sets aContent: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleUtil.cpp#563

@@ +481,5 @@
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_NONCE_SCRIPT,  SCRIPT,     CSP_UNSAFE_INLINE, SCRIPT_NONCE_VIOLATION_OBSERVER_TOPIC);
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_NONCE_STYLE,   STYLESHEET, CSP_UNSAFE_INLINE, STYLE_NONCE_VIOLATION_OBSERVER_TOPIC);
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_HASH_SCRIPT,   SCRIPT,     CSP_UNSAFE_INLINE, SCRIPT_HASH_VIOLATION_OBSERVER_TOPIC);
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_HASH_STYLE,    STYLESHEET, CSP_UNSAFE_INLINE, STYLE_HASH_VIOLATION_OBSERVER_TOPIC);
> +

Nit: usually we target 80 characters per line; these macro calls seem to be super long; would breaking each macro call up into 2 lines completely eliminate any sort of readability?

@@ +557,4 @@
>      nsresult rv = aChannel->GetURI(getter_AddRefs(mSelfURI));
>      NS_ENSURE_SUCCESS(rv, rv);
>    }
>  

Super-Nit: I think the following mini snippted saves a few instructions:

  // first use aSelfURI.  If that's not available get the URI from aChannel.
  mSelfURI = aSelfURI;

  if (!mSelfURI) {
    nsresult rv = aChannel->GetURI(getter_AddRefs(mSelfURI));
    NS_ENSURE_SUCCESS(rv, rv);
  }

@@ +581,5 @@
> +  }
> +
> +  if (aReferrer) {
> +    mReferrer = aReferrer;
> +  }

Same Nit: you don't necessarily need that if, right?


  mReferrer = aReferrer;

  if (!mReferrer) {...

@@ +605,5 @@
> +                          nsAString& aSourceFile,
> +                          nsAString& aScriptSample,
> +                          uint32_t aLineNum)
> +{
> +  NS_ENSURE_ARG_MAX(aViolatedPolicyIndex, mPolicies.Length() - 1);

I like that a lot! thanks

@@ +608,5 @@
> +{
> +  NS_ENSURE_ARG_MAX(aViolatedPolicyIndex, mPolicies.Length() - 1);
> +
> +  // TODO(sid/ckerschb): return early if we know this will fail due to lack of
> +  // load group information (on process-split necko implementations like b2g).

Nit: you could include the bug number 1011086

@@ +754,5 @@
> +      // skip unauthorized URIs
> +      CSPCONTEXTLOG(("nsIContentPolicy blocked sending report to %s",
> +                     reportURIs[r].get()));
> +      continue; // don't return yet, there may be more URIs
> +    }

Since you wanted to have that changed when reviewing bug 994466, I am going to let you incorporate your own suggestion :-) because it's actually part of this patch and not of the on attached to bug 994466.

>> Please use NS_CP_REJECTED(shouldLoad) instead of !=.

@@ +788,5 @@
> +      // AsyncOpen should not fail, but could if there's no load group (like if
> +      // SetRequestContext is not given a channel).  This should fail quietly
> +      // since it's really ok if reports don't go out, but it's good to log the
> +      // error locally.
> +      continue;

Nit: we are at the end of the loop, no need for continue; or do you want to prevent future bugs in case someone extends the function body?

@@ +852,5 @@
> +                               mViolatedDirective, mViolatedPolicyIndex,
> +                               mSourceFile, mScriptSample, mLineNum);
> +
> +      // 3) log to console (one per policy violation)
> +      //mBlockedContentSource could be a URI or a string.

super-nit: space after //

@@ +873,5 @@
> +                            params, ArrayLength(params),
> +                            mSourceFile, mScriptSample, mLineNum, 0,
> +                            nsIScriptError::errorFlag, "CSP", mInnerWindowID);
> +      }
> +      return NS_OK;

I like that refactoring, nicely done.

@@ +1124,5 @@
> +NS_IMETHODIMP
> +CSPReportRedirectSink::AsyncOnChannelRedirect(nsIChannel* aOldChannel,
> +                                              nsIChannel* aNewChannel,
> +                                              uint32_t aRedirFlags,
> +                                              nsIAsyncVerifyRedirectCallback *aCallback)

Nit: move the pointer to the type.

::: content/base/test/TestCSPParser.cpp
@@ +92,5 @@
>    nsresult rv = NS_NewURI(getter_AddRefs(selfURI), "http://www.selfuri.com");
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // we also init the csp with a dummyChannel, which is unused
> +  // for the parser tests but surpresses assertiosn in SetRequestContext.

nit: assertions - thanks for fixing :-)

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

> >                                    const nsAString& aSourceFile,
> >                                    const nsAString& aScriptSample,
> >                                    int32_t aLineNum,
> >                                    const nsAString& aNonce,
> >                                    const nsAString& aContent)
> 
> It seems like aContent is unused in this function body, are we missing
> something or can we eliminate that argument?

I looked into this problem again, it seems you just have to pass aNonce, aContent, or a null/emptystring/whatever to the macro as an additonal argument, because ::allows takes a second argument which is aNonceOrHash and defaults to the emptystring if not explicityl provided. Compare function def:

> ::allows(enum CSPKeyword aKeyword, const nsAString& aHashOrNonce) const

Comment 21

[Garrett Robinson [:grobinson]]

Comment on attachment 8423459 [details] [diff] [review]
violationReporting

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

Just started reviewing this, but when I pulled the latest from the patch queue it's very different from this patch. Looks like a lot of the issues I'm commenting on have been addressed. Ending the review here, please post the up to date patch when it's ready (and, in the future, clear review flags for such obsoleted patches).

::: content/base/src/nsCSPContext.cpp
@@ +460,2 @@
>  NS_IMETHODIMP
>  nsCSPContext::LogViolationDetails(uint16_t aViolationType,

It seems LogViolationDetails is only used to detect "inline" violations, ShouldLoad calls AsyncReportViolation directly. Can we change the name of the function to reflect that detail?

@@ +462,5 @@
>                                    const nsAString& aSourceFile,
>                                    const nsAString& aScriptSample,
>                                    int32_t aLineNum,
>                                    const nsAString& aNonce,
>                                    const nsAString& aContent)

aContent is used to re-check hash violations. It is also set in nsScriptLoader::CSPAllowsInlineScript.

@@ +480,5 @@
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_INLINE_SCRIPT, SCRIPT,     CSP_UNSAFE_INLINE, INLINE_SCRIPT_VIOLATION_OBSERVER_TOPIC);
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_NONCE_SCRIPT,  SCRIPT,     CSP_UNSAFE_INLINE, SCRIPT_NONCE_VIOLATION_OBSERVER_TOPIC);
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_NONCE_STYLE,   STYLESHEET, CSP_UNSAFE_INLINE, STYLE_NONCE_VIOLATION_OBSERVER_TOPIC);
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_HASH_SCRIPT,   SCRIPT,     CSP_UNSAFE_INLINE, SCRIPT_HASH_VIOLATION_OBSERVER_TOPIC);
> +      CASE_CHECK_AND_REPORT(VIOLATION_TYPE_HASH_STYLE,    STYLESHEET, CSP_UNSAFE_INLINE, STYLE_HASH_VIOLATION_OBSERVER_TOPIC);

This macro isn't doing the right thing for nonce or hash. You're not passing aNonce or aContent, so they can't be re-checked. Best way to resolve this might be to change nsCSPPolicy::allows so it doesn't always pass NS_LITERAL_STRING("") for that argument.

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #21)
> Just started reviewing this, but when I pulled the latest from the patch
> queue it's very different from this patch. Looks like a lot of the issues
> I'm commenting on have been addressed. Ending the review here, please post
> the up to date patch when it's ready (and, in the future, clear review flags
> for such obsoleted patches).

Sorry, I had just addressed chris's comments and updated the patch repo in the last few hours and didn't want to clear the flag until I had a new patch up.  I figured any comments you had in addition to chris's comments would be relevant to the updated patch since I didn't intend to fix more than he commented on and also figured you may have already started to review while I was updating the patch.

> ::: content/base/src/nsCSPContext.cpp
> @@ +460,2 @@
> >  NS_IMETHODIMP
> >  nsCSPContext::LogViolationDetails(uint16_t aViolationType,
> 
> It seems LogViolationDetails is only used to detect "inline" violations,
> ShouldLoad calls AsyncReportViolation directly. Can we change the name of
> the function to reflect that detail?

Unfortunately, not easily.  It's part of the IDL which means the old implementation also uses it this way.  We could file a follow-up bug to make this change (either before or after decommissioning the old backend), but it's out of scope for this work.  (BTW, it also is used for eval-block reporting, not just the "inline" stuff).

> This macro isn't doing the right thing for nonce or hash. You're not passing
> aNonce or aContent, so they can't be re-checked. Best way to resolve this
> might be to change nsCSPPolicy::allows so it doesn't always pass
> NS_LITERAL_STRING("") for that argument.

There's another nsCSPPolicy::allows that takes a third argument.  I'll just use that.  You're right, that macro is borked and I'm embarassed that I uploaded it in that way.  I'll post a better patch shortly.

Sorry for the confusion, Garrett.

Comment 23

[Sid Stamm [:geekboy or :sstamm]]

Attachment: violationReporting

New patch with Chris's comments addressed (and the other comments from him and Garrett in this bug).

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8424220 [details] [diff] [review]
violationReporting

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

Ready to go! Please consider putting an assertion when assembling the JSON. Just in case!

::: content/base/src/nsCSPContext.cpp
@@ +643,5 @@
> +      nsCOMPtr<nsISupportsCString> cstr = do_QueryInterface(aBlockedContentSource);
> +      if (cstr) {
> +        cstr->GetData(reportBlockedURI);
> +      }
> +    }

Usually that should never happen, but what if a 'uri' is null and then also 'cstr' is null. we would end up with a json not having a blocked-uri set, which would be confusing to users. Maybe we should add an assertion here.

@@ +846,5 @@
> +          do_CreateInstance(NS_SUPPORTS_CSTRING_CONTRACTID);
> +        NS_ASSERTION(supportscstr, "Couldn't allocate nsISupportsCString");
> +        supportscstr->SetData(NS_ConvertUTF16toUTF8(aObserverSubject));
> +        mObserverSubject = do_QueryInterface(supportscstr);
> +      }

Super Nit: the comments indicates how to flow should be; you could switch the if-statement around and do what's currently in the else-branch first. Makes more sense semantically.

Comment 25

[Garrett Robinson [:grobinson]]

Comment on attachment 8424220 [details] [diff] [review]
violationReporting

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

::: content/base/src/nsCSPContext.cpp
@@ +502,5 @@
> +      CASE_CHECK_AND_REPORT(HASH_STYLE,        STYLESHEET, aContent,
> +                            CSP_UNSAFE_INLINE, STYLE_HASH_VIOLATION_OBSERVER_TOPIC);
> +
> +      default:
> +        NS_ASSERTION(false, "LogViolationDetails with invalied type");

Nit: invalied

@@ +513,5 @@
> +#undef CASE_CHECK_AND_REPORT
> +
> +uint64_t
> +getInnerWindowID(nsIRequest* aRequest) {
> +  // Get the innerWindowID associated with the XMLHTTPRequest

This comment suggests that we only use this function on XHR's, but we appear to use it on any channel that is passed to setRequestContext.

@@ +550,5 @@
> +  if (!du) {
> +    return 0;
> +  }
> +
> +  rv = du->GetCurrentInnerWindowID(&id);

This is wrong. You want the inner window associated with aRequest, which you can get from GetAssociatedWindow on the nsILoadContext. This will give you the window ID of the current active tab, which is not necessarily the window associated with the request we are examining here.

@@ +580,4 @@
>  
> +  if (aChannel) {
> +    mInnerWindowID = getInnerWindowID(aChannel);
> +    aChannel->GetLoadGroup(getter_AddRefs(mCallingChannelLoadGroup));

I don't know enough about the Necko request life cycle to know if this is a good idea. Or is the document's load group kept alive for the duration of its existence anyway? (in which case this should be fine)

@@ +583,5 @@
> +    aChannel->GetLoadGroup(getter_AddRefs(mCallingChannelLoadGroup));
> +  }
> +  else {
> +    NS_WARNING("Channel needed (but null) in SetRequestContext.  Cannot query"
> +               "loadgroup, which means report sending may fail.");

I'd prefer to see this message all on one line. Splitting it lets you conform to the line length recommendations in the style guide, but it also makes it hard to grep for this message.

@@ +604,5 @@
> +      httpChannel->GetReferrer(getter_AddRefs(mReferrer));
> +    }
> +    else {
> +      NS_WARNING("Channel provided to SetRequestContext is not an nsIHttpChannel"
> +                 "so referrer is not available for reporting." );

Nit: here and above, you need to add a space at the end of the first line or the beginning of the second (if you're going to keep the split strings, which I do not endorse).

@@ +624,5 @@
> +  NS_ENSURE_ARG_MAX(aViolatedPolicyIndex, mPolicies.Length() - 1);
> +
> +  // TODO(sid/ckerschb): return early if we know this will fail due to lack of
> +  // load group information (on process-split necko implementations like b2g).
> +  // (fix this in bug 1011086)

You should implement this TODO before landing, since you know you won't be able to send the reports. IIRC there's a macro that checks if you're multiprocess, although I can't remember exactly what it is.

test_csp_report.html is already disabled on multiprocess because it uses the http-on-opening request observer, which we need to proxy. This issue will also break that test, so I'm adding bug 1011086 as a blocker of bug 1009632.

@@ +717,5 @@
> +    if (NS_FAILED(rv)) {
> +      const char16_t* params[] = { reportURIs[r].get() };
> +      CSPCONTEXTLOG(("Could not create nsIURI for report URI %s",
> +                     reportURIs[r].get()));
> +      CSP_LogLocalizedStr(NS_LITERAL_STRING("triedToSendReport").get(),

Why not just use a string literal here?

@@ +803,5 @@
> +
> +      // AsyncOpen should not fail, but could if there's no load group (like if
> +      // SetRequestContext is not given a channel).  This should fail quietly
> +      // since it's really ok if reports don't go out, but it's good to log the
> +      // error locally.

Nit: put the comment before the code it is commenting on

@@ +1103,5 @@
> +                                            uint32_t aCount)
> +{
> +  uint32_t read;
> +  nsCString decodedData;
> +  return aInputStream->ReadSegments(AppendSegmentToString,

Why are we doing any of this stuff? I don't see where we use it elsewhere.

::: content/base/src/nsCSPContext.h
@@ +59,5 @@
>                                      bool* outIsAllowed) const;
>  
> +    nsCOMPtr<nsIURI>                           mReferrer;
> +    nsCOMPtr<nsIPrincipal>                     mPrincipal;
> +    uint64_t                                   mInnerWindowID; /* used for web console logging */

Nit: prefer // over /* */ for single-line comments.

Comment 26

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Garrett Robinson [:grobinson] from comment #25)
> @@ +550,5 @@
> > +  if (!du) {
> > +    return 0;
> > +  }
> > +
> > +  rv = du->GetCurrentInnerWindowID(&id);
> 
> This is wrong. You want the inner window associated with aRequest, which you
> can get from GetAssociatedWindow on the nsILoadContext. This will give you
> the window ID of the current active tab, which is not necessarily the window
> associated with the request we are examining here.

I'm confused, isn't that what I'm doing? Getting the window from aRequest, and getting its inner window ID...  This code is doing almost exactly what mgoodwin implemented in bug 770099, (See http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#1931) which seemed to work.  This code is using GetAssociatedWindow on a load context.  The "get" chain is:

request
 ->loadGroup
 ->notificationCallbacks
 ->loadContext
 ->associatedWindow
 ->domwindowutils
 ->currentInnerWindowID;

How else can I get the innerwindowID from an nsIDOMWindow?


> @@ +580,4 @@
> >  
> > +  if (aChannel) {
> > +    mInnerWindowID = getInnerWindowID(aChannel);
> > +    aChannel->GetLoadGroup(getter_AddRefs(mCallingChannelLoadGroup));
> 
> I don't know enough about the Necko request life cycle to know if this is a
> good idea. Or is the document's load group kept alive for the duration of
> its existence anyway? (in which case this should be fine)

This is something else that didn't change from the previous implementation of CSP.
SetRequestContext is called synchronously during nsDocument::Init(), where the channel is alive and the loadGroup is still alive, so it should work.

> @@ +624,5 @@
> > +  NS_ENSURE_ARG_MAX(aViolatedPolicyIndex, mPolicies.Length() - 1);
> > +
> > +  // TODO(sid/ckerschb): return early if we know this will fail due to lack of
> > +  // load group information (on process-split necko implementations like b2g).
> > +  // (fix this in bug 1011086)
> 
> You should implement this TODO before landing, since you know you won't be
> able to send the reports. IIRC there's a macro that checks if you're
> multiprocess, although I can't remember exactly what it is.
> 
> test_csp_report.html is already disabled on multiprocess because it uses the
> http-on-opening request observer, which we need to proxy. This issue will
> also break that test, so I'm adding bug 1011086 as a blocker of bug 1009632.

Again, I'm confused.  If the test is disabled because it already doesn't work, why is that blocking landing of this replacement for current code?  The early return is just an optimization, and it's not trivial to implement (though it does not make the implementation *worse* than the current one).

Of course we can either implement that in this bug or we can implement it later, but this is not a new issue.

> @@ +717,5 @@
> > +      CSP_LogLocalizedStr(NS_LITERAL_STRING("triedToSendReport").get(),
> 
> Why not just use a string literal here?

Needs to be UTF16 (const char16_t*).

> @@ +1103,5 @@
> > +                                            uint32_t aCount)
> > +{
> > +  uint32_t read;
> > +  nsCString decodedData;
> > +  return aInputStream->ReadSegments(AppendSegmentToString,
> 
> Why are we doing any of this stuff? I don't see where we use it elsewhere.

We don't need to keep read data, we just have to avoid putting the stream in an unexpected state.
https://bugzilla.mozilla.org/show_bug.cgi?id=846458#c67

Comment 27

[Sid Stamm [:geekboy or :sstamm]]

Forgot to flag grobinson for needinfo about the innerWindowID issue.  Is there a better way to do it?  I think the code is already getting the window info from the request...

I'll wait to upload a new patch until we figure this out.

Comment 28

[Garrett Robinson [:grobinson]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #26)
> (In reply to Garrett Robinson [:grobinson] from comment #25)

> I'm confused, isn't that what I'm doing? Getting the window from aRequest,
> and getting its inner window ID...  This code is doing almost exactly what
> mgoodwin implemented in bug 770099, (See
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.
> jsm#1931) which seemed to work.  This code is using GetAssociatedWindow on a
> load context.  The "get" chain is:
> 
> request
>  ->loadGroup
>  ->notificationCallbacks
>  ->loadContext
>  ->associatedWindow
>  ->domwindowutils
>  ->currentInnerWindowID;

Apologies, I misunderstood the code in nsIDOMWindowUtils. This works.

> How else can I get the innerwindowID from an nsIDOMWindow?

I think you can also do associatedWindow->GetDocument()->GetInnerWindowID(), but if this code works I wouldn't bother.

> This is something else that didn't change from the previous implementation
> of CSP.
> SetRequestContext is called synchronously during nsDocument::Init(), where
> the channel is alive and the loadGroup is still alive, so it should work.

I meant, maybe it's not good to keep the calling channel's load group alive for the lifetime of the document (which we do because we're addref'ing). I just don't know. If it's been working so far, then I see no reason to change it.

> The early return is just an optimization, and it's not trivial to implement
> (though it does not make the implementation *worse* than the current one).

I thought it was trivial to implement, if it's not then I agree it should not be a blocker as it is just an optimization.

> Of course we can either implement that in this bug or we can implement it
> later, but this is not a new issue.

If you don't want to do it, remove the TODO. I guess I'd like there to be logging when this happens, so people can easily answer the question "why am I not seeing CSP reports being sent on B2G or e10s?" You could just add the logging down where you try to set the load group and it fails, if you prefer. It also just seems silly to do a bunch of work to build the report if *know* we won't be able to send it. And it would be clearer to gate the function at the top for other people looking at this code, since this is basically a precondition.

> > @@ +1103,5 @@
> > > +                                            uint32_t aCount)
> > > +{
> > > +  uint32_t read;
> > > +  nsCString decodedData;
> > > +  return aInputStream->ReadSegments(AppendSegmentToString,
> > 
> > Why are we doing any of this stuff? I don't see where we use it elsewhere.
> 
> We don't need to keep read data, we just have to avoid putting the stream in
> an unexpected state.
> https://bugzilla.mozilla.org/show_bug.cgi?id=846458#c67

Ah, I figured it was something like that. Thanks for providing the context!

Comment 29

[Sid Stamm [:geekboy or :sstamm]]

Attachment: violationReporting

Thanks for the review, grobinson.  I updated the patch with your review comments.  Chris: since I changed a bit, would you take another quick look at it?

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8425144 [details] [diff] [review]
violationReporting

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

Alright Sid, you already got an r+ for this patch last time. Looked over it again, and of course, I found some more nitpicks :-)  In general: solid work and ready to land.

::: content/base/src/nsCSPContext.cpp
@@ +445,5 @@
> +                                 aSourceFile, aScriptSample, aLineNum);                        \
> +    }                                                                                          \
> +    PR_END_MACRO;                                                                              \
> +    break
> +

Nit: No need to use so many spaces between the last char of the line and the \.

@@ +578,1 @@
>  

Nit: I guess you are missing a whitespace either at the end of the first line of the assertion or at the beginning of the second line of the assertion.

@@ +633,5 @@
> +  csp_report.AppendASCII("{\"csp-report\": {");
> +
> +  // blocked-uri
> +  csp_report.AppendASCII("\"blocked-uri\": \"");
> +  if (aBlockedContentSource) {

Do we want an NS_WARNING before that if-statement, checking that aBlockedContentSource is not null? I think it is confusing for consumers to get a report with and empty blocked-uri.

@@ +757,5 @@
> +    // check content policy
> +    int16_t shouldLoad = nsIContentPolicy::ACCEPT;
> +    rv = NS_CheckContentLoadPolicy(nsIContentPolicy::TYPE_CSP_REPORT,
> +                                   reportURI,
> +                                   mPrincipal, /* should be principal */

Nit: remove the comment:
/* should be principal */?? Isnt' it the principal?
Would rather have a comment for the handed of nullptr and EmptyCString();

Comment 31

[Sid Stamm [:geekboy or :sstamm]]

Attachment: violationReporting

Hey Garrett, looks like I didn't qref and ended up missing some of your nits.  Here's a patch without that problem (and also taking into account ckerschb's latest nits).

Comment 32

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8425608 [details] [diff] [review]
violationReporting

Carrying over r=ckerschb

Comment 33

[Garrett Robinson [:grobinson]]

Comment on attachment 8425608 [details] [diff] [review]
violationReporting

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

lgtm!

::: content/base/src/nsCSPContext.cpp
@@ +426,5 @@
> + * @param nonceOrHash: for NONCE and HASH violations, it's the nonce or content
> + *               string. For other violations, it is an empty string.
> + * @param keyword: the keyword corresponding to violation (UNSAFE_INLINE for most)
> + * @param observerTopic: the observer topic string to send with the CSP
> + *                 observer notifications.

Fantastic improvements to these comments!

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Since Sid is on a plane, he asked me to set the checkin keyword - it's ready to land. I pushed this patch  to try [1] together with the patch from bug 994466. Please apply this patch and the patch from bug 994466 and land both at the same time. Keep in mind that the new CSP implmentation lives behind a pref at the moment and is disabled by default! Hence only running Linux builds on TBPL. Thanks!

[1] https://tbpl.mozilla.org/?tree=Try&rev=955de4d3dda2

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb00a01bf9f3

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb00a01bf9f3