As a follow up to bug 1309219, here's a patch which does what the summary says (for the sake of better code re-use). It's a trivial patch and the tests are passing locally for me, so I'm guessing it's safe to skip a try-run this time and just skip to requesting a review.

Comment on attachment 8820017 [details] [diff] [review] use_isCharacterToken_and_isNumberToken_in_IsValidBase64Value.diff Review of attachment 8820017 [details] [diff] [review]: ----------------------------------------------------------------- Hey Thomas, since I already have you here, can you do a few quick cleanups: * Move IsValidBase64Value right after isValidHexDig (and move the return value on it's own line, similar to isValidHexDig) * It's counter intuitive that IsValidBase64Value checks for: > if (end > cur && *(end-1) == EQUALS) end--; > if (end > cur && *(end-1) == EQUALS) end--; which might be a problem in case we want to reuse that function, can we move those checks to the callsite if necessary? To be honest, I don't fully understand what that is testing, can you probably add a comment? * Potentially it's even worse renaming the function to something like isValidNonceOrHashValue(). Thanks a lot for fixing!

>I don't fully understand what that is testing, can you probably add a comment? Oh, definitely. I really should have done that from the start :) >It's counter intuitive that IsValidBase64Value checks for: Per spec, nonce-sources must be gramatically valid base64 strings[1], and those may optionally end with = or ==. As such I'd rather leave the function named as it is (or similarly) and not break it apart. Here's a fresh patch with your other requested changes; let me know if you'd like another iteration. [1] https://w3c.github.io/webappsec-csp/#grammardef-nonce-source

Comment on attachment 8820303 [details] [diff] [review] 1324542-code_quality_tweaks_for_IsValidBase64Value.diff Review of attachment 8820303 [details] [diff] [review]: ----------------------------------------------------------------- Please just fix my nits. ::: dom/security/nsCSPParser.cpp @@ +179,5 @@ > > +static bool > +isValidBase64Value(const char16_t* cur, const char16_t* end) > +{ > + // Using grammar at https://w3c.github.io/webappsec-csp/#grammardef-nonce-source Ah, I should have looked at that before, it can really end with *2( "=" ). Thanks for clarification, it that case please keep the function name. @@ +180,5 @@ > +static bool > +isValidBase64Value(const char16_t* cur, const char16_t* end) > +{ > + // Using grammar at https://w3c.github.io/webappsec-csp/#grammardef-nonce-source > + bool isValid = true; I liked your other version better were you just return true or false right away. Please remove the tmp variable isValid and go back to early returns as you had. @@ +188,5 @@ > + if (end > cur && *(end-1) == EQUALS) end--; > + > + // Must have at least one character aside from any = > + if (end == cur) { > + isValid = false; return false; @@ +189,5 @@ > + > + // Must have at least one character aside from any = > + if (end == cur) { > + isValid = false; > + } else { no need for the else @@ +195,5 @@ > + for (; cur < end; ++cur) { > + if (!(isCharacterToken(*cur) || isNumberToken(*cur) || > + *cur == PLUS || *cur == SLASH || > + *cur == DASH || *cur == UNDERLINE)) { > + isValid = false; return false; @@ +201,5 @@ > + } > + } > + } > + > + return isValid; return true;

Thanks again for fixing!

No problem, here's the final patch. Carrying over r+ and requesting check-in.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a177f6b289fb Code-quality tweaks for isValidBase64Value. r=ckerschb