Closed Bug 1256488 Opened 8 years ago Closed 8 years ago

Add a Base64 URL-decoder and expose to chrome JS code

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: lina, Assigned: lina)

References

Details

(Keywords: site-compat, Whiteboard: btpp-active)

Attachments

(4 files, 4 obsolete files)

smaug suggested exposing the URL-safe Base64 encoder to chrome code back in December, after bug 1205137 landed. We have a few callers that could use these (in C++ and JS), so let's add a decoder and expose the functions on `ChromeUtils`.
Attached patch 1.patch (obsolete) — Splinter Review
The decoder is based on mt's table-based implementation in https://github.com/martinthomson/webpush-client/blob/gh-pages/webpush.js#L34-L66, except it spells the table out. It also allows padding, since Python's `base64.urlsafe_b64encode` can include it, and I didn't want to make the Push callers strip it out.

Uploading this as a Splinter patch to make smaug's life easier. :-)
Attachment #8730448 - Flags: review?(martin.thomson)
Attachment #8730448 - Flags: review?(bugs)
Attached patch WebCrypto.patch (obsolete) — Splinter Review
Web Crypto looks like it should be able to use this decoder for JWK, but the "x" coordinate for one of the test keys isn't URL-safe: https://dxr.mozilla.org/mozilla-central/rev/f0c0480732d36153e8839c7f17394d45f679f87d/dom/crypto/test/test-vectors.js#726

The odd thing is, "_" is part of the URL-safe alphabet, but "/" isn't. If I replace it with "_", like in this patch, all the crypto tests pass.

RFC 7517 section 3 implies that the "x" and "y" coordinates should be base64url-encoded, and I think this is in line with RFC 7515, section 2. Tim, you originally landed these as part of bug 1034855. Does this seem right, or does Web Crypto actually allow more lax encoding?
Attachment #8730449 - Flags: feedback?(ttaubert)
Comment on attachment 8730448 [details] [diff] [review]
1.patch

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

::: dom/base/test/unit/test_chromeutils_base64.js
@@ +43,5 @@
> +  assertEncodedString("fo", "Zm8");
> +  assertEncodedString("foo", "Zm9v");
> +  assertEncodedString("foob", "Zm9vYg");
> +  assertEncodedString("fooba", "Zm9vYmE");
> +  assertEncodedString("foobar", "Zm9vYmFy");

I think that you should be able to run the shared part of these tests with the shared part of the assertDecodedString tests below by running both in different directions.  That would save on the duplicated values.

You can pad to a multiple of 4 when decoding by doing:

if (encoded.length % 4) {
  let padded = encoded;
  do { padded += '='; } while(padded.length % 4);
  deepEqual(new Uint8Array(ChromeUtils.base64URLDecode(padded)), decoded);
}

::: toolkit/identity/IdentityCryptoService.cpp
@@ +44,5 @@
>    }
>  }
>  
>  nsresult
>  Base64UrlEncodeImpl(const nsACString & utf8Input, nsACString & result)

I would change the two call sites for this function instead.  One of them already has a buffer pointer and length and does a reinterpret_cast to massage it into a string, so you would save a back-and-forth.

::: xpcom/io/Base64.cpp
@@ +267,5 @@
> +  255, 255, 255, 255,
> +};
> +
> +bool Base64URLCharToValue(char aChar, uint8_t* aValue) {
> +  *aValue = kBase64URLDecodeTable[(uint8_t) aChar];

You can halve the table size by doing aChar & 0x7f

@@ +268,5 @@
> +};
> +
> +bool Base64URLCharToValue(char aChar, uint8_t* aValue) {
> +  *aValue = kBase64URLDecodeTable[(uint8_t) aChar];
> +  return *aValue != 255;

If you do the above return (aValue != 255) && (aChar & ~0x7f)

@@ +284,5 @@
> +  {}
> +
> +  ~AutoFreeBuffer()
> +  {
> +    free(mBuffer);

not delete[] mBuffer ?

@@ +442,5 @@
> +  }
> +
> +  // Allocate a buffer large enough to hold the decoded output. May be 1-2
> +  // bytes over if the input is unpadded.
> +  uint8_t* outputBegin = static_cast<uint8_t*>(malloc(sourceLength));

new?

@@ +446,5 @@
> +  uint8_t* outputBegin = static_cast<uint8_t*>(malloc(sourceLength));
> +  if (!outputBegin) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  AutoFreeBuffer autoFree(outputBegin);

Please use UniquePtr<uint8_t[]> instead.

::: xpcom/io/Base64.h
@@ +44,5 @@
>  
> +/**
> + * Decodes an optionally padded, Base64 URL-encoded |aString| into a buffer.
> + * The caller takes ownership of |aData| if decoding succeeds. |aData| points
> + * to memory allocated by |malloc|.

If it is at all possible to use new and UniquePtr<uint8_t[]>, please do that.  (I think that it is.)

Also, fix the names in the comments.
Attachment #8730448 - Flags: review?(martin.thomson)
Comment on attachment 8730448 [details] [diff] [review]
1.patch

The comment about base64URLEncode talks about RFC 4648.
Does the same RFC apply to base64URLDecode? Or how does it interpret the param?
smaug, yes, this is RFC 4648.


Kit, Peter and I just talked about padding.  I suggested that for push we might want to forbid it, since '=' is forbidden in certain forms of the header field values.  I don't mind if we allow it, but that has some negative effects.  Do we have any evidence that people are leaving padding in?  (Also, we can have the push service screen for us in most cases, can't we?)
(In reply to Martin Thomson [:mt:] from comment #3)
> If it is at all possible to use new and UniquePtr<uint8_t[]>, please do
> that.  (I think that it is.)

I forgot about UniquePtr. Thanks!

I think I'll want to use `new (mozilla::fallible) uint8_t[sourceLength]`, since the other decoder function uses fallible allocation. Does that seem right?

(In reply to Olli Pettay [:smaug] from comment #4)
> The comment about base64URLEncode talks about RFC 4648.
> Does the same RFC apply to base64URLDecode?

It does, except it permits padding, since I noticed some decoders (like Python's) will append it anyway. Should I clarify that in the comment, or remove padding support entirely and make callers normalize the string?
(In reply to Martin Thomson [:mt:] from comment #5)
> Kit, Peter and I just talked about padding.  I suggested that for push we
> might want to forbid it, since '=' is forbidden in certain forms of the
> header field values.  I don't mind if we allow it, but that has some
> negative effects.  Do we have any evidence that people are leaving padding
> in?  (Also, we can have the push service screen for us in most cases, can't
> we?)

This seems reasonable to me. Is that because padding would make it harder to parse unquoted params in `Crypto-Key` and `Encryption`? We can make this a strict implementation and have the service screen values.
> new (mozilla::fallible) uint8_t[sourceLength]

Absolutely.  Always use fallible for potentially-large values (though this probably doesn't qualify, the extra checks don't hurt).

> Is that because padding would make it harder to parse unquoted params

Stronger than that. '=' is not permitted in unquoted parameters.  I'd prefer to see padding forbidden here, at least for now.  It's less code too.
(In reply to Martin Thomson [:mt:] from comment #8)
> Stronger than that. '=' is not permitted in unquoted parameters.  I'd prefer
> to see padding forbidden here, at least for now.  It's less code too.

Will do.

(In reply to Kit Cambridge [:kitcambridge] from comment #6)
> It does, except it permits padding, since I noticed some decoders (like
> Python's) will append it anyway.

I meant "some encoders."
Attachment #8730448 - Flags: review?(bugs)
Attached patch Decoder.patch (obsolete) — Splinter Review
Instead of doing funky things UniquePtr out params, I just decided to make the second argument a FallibleTArray. As a nice side effect, CryptoBuffer can write directly into itself.

Martin, I also changed PushService{WebSocket, AndroidGCM} to strip padding from incoming payloads, until we deploy a new version of the server that can take care of this for us.
Attachment #8730448 - Attachment is obsolete: true
Attachment #8730542 - Flags: review?(martin.thomson)
Attachment #8730542 - Flags: review?(bugs)
Attached patch WebCrypto.patch (obsolete) — Splinter Review
Same issue as comment 2; just cleaned up decoding a bit.
Attachment #8730449 - Attachment is obsolete: true
Attachment #8730449 - Flags: feedback?(ttaubert)
Attachment #8730543 - Flags: feedback?(ttaubert)
Comment on attachment 8730542 [details] [diff] [review]
Decoder.patch

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

::: xpcom/io/Base64.cpp
@@ +408,5 @@
> +  uint32_t decodedLength = (sourceLength * 3) / 4;
> +  if (!aOutput.SetCapacity(decodedLength, mozilla::fallible)) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }
> +  aOutput.SetLengthAndRetainStorage(decodedLength);

I guess I could just call SetLength() here, instead of this two-step process. IIRC, that increases the capacity up to the nearest power of 2, and I'm not sure we want that here...especially since we're never calling AppendElements().
Comment on attachment 8730543 [details] [diff] [review]
WebCrypto.patch

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

LGTM, the / in the test vector is an oversight and we currently support it accidentally.
Attachment #8730543 - Flags: feedback?(ttaubert) → feedback+
(In reply to Kit Cambridge [:kitcambridge] from comment #10)
> Martin, I also changed PushService{WebSocket, AndroidGCM} to strip padding
> from incoming payloads, until we deploy a new version of the server that can
> take care of this for us.
Wait, am I misunderstanding this, or is it so that even our own servers don't really pass the kind of encoded urls that this new code is supposed to eat?
(In reply to Olli Pettay [:smaug] from comment #14)
> Wait, am I misunderstanding this, or is it so that even our own servers
> don't really pass the kind of encoded urls that this new code is supposed to
> eat?

For these two cases, yes. :-( Our Python server pads URL-encoded output. We either need the server to remove it, or add an option like `ChromeUtils.base64URLDecode(string, { ignorePadding: true })`.

The other cases decode headers, JWTs, and JWKs, so it's OK to throw on padded input.
It's my experience that the majority of base64 libraries in the wild add padding to encoded strings, (including python, perl, go, php, java, etc.) This means that unless app developers are very mindful to remove padding, they're going to be adding it and wondering why their "perfectly valid" base64 strings are getting rejected by firefox & mozilla services. 

Speaking for the code I'm responsible or, I intend on accepting the padding, stripping it where needed and sending it on it's merry way. As much as it would be nice to demand that outside parties don't include padding with their strings, I'd feel a bit like King Canute.
Comment on attachment 8730542 [details] [diff] [review]
Decoder.patch


>+  /**
>+   * Decodes an optionally padded, Base64 URL-encoded string.
>+   *
>+   * @param string The string to decode.
>+   * @returns The decoded buffer.
>+   */
>+  [Throws, NewObject]
>+  static ArrayBuffer base64URLDecode(ByteString string);
So I'm a bit lost now. I thought Decoded wasn't suppose to handle padding but base on the comment it does handle that.
And anyhow, based on the needed changes to code it is rather clear that we need to have a form of the method which
can handle padding. Either a separate method, something like base64URLDecodeWithOptionalPadding or some dictionary param to 
base64URLDecode (the dictionary should have a required property to define the behavior). 
It would be a bit silly to have a helper but then many callers would themselves need to take care of removing the padding
Attachment #8730542 - Flags: review?(bugs) → review-
JR, options are the enemy.  Optional padding isn't going to help interoperability.  It's just Postel speaking to you from beyond the grave.  If we reject padding, we don't get padding.

It's true that base64 libraries pad; but the URL and filename safe variant is more often used without padding.  See JW*.  If you have evidence that being stricter causes bustage, then let's talk about adding some lenience then.
We agreed with Kit on IRC to add a dictionary param with a required property defining whether padding is allowed or not. That way the caller must think about what kind of endoded URL is being handled.
Comment on attachment 8730542 [details] [diff] [review]
Decoder.patch

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

Splinter is terrible.  I'm assuming that you will spin this with the agreed changes.
Attachment #8730542 - Flags: review?(martin.thomson)
Attached patch Decoder.patchSplinter Review
Thanks for all your help with this, smaug. Here's a patch that implements what we discussed.

I decided to use separate functions for decoding padded and unpadded input in C++, but I'm amenable to just passing the dictionary directly. I added a matching "PaddedBase64Encode" function that we can use in Push, too, since our server expects padded input.

Also, I noticed that IdentityCryptoService actually returns *padded* output (despite the comment in nsIIdentityCryptoService.idl). The tests in toolkit/identity confirm this, so I fixed the comment.
Attachment #8730542 - Attachment is obsolete: true
Attachment #8731161 - Flags: review?(martin.thomson)
Attachment #8731161 - Flags: review?(bugs)
(In reply to Martin Thomson [:mt:] from comment #18)
> JR, options are the enemy.  Optional padding isn't going to help
> interoperability.  It's just Postel speaking to you from beyond the grave. 
> If we reject padding, we don't get padding.
> 
> It's true that base64 libraries pad; but the URL and filename safe variant
> is more often used without padding.  See JW*.  If you have evidence that
> being stricter causes bustage, then let's talk about adding some lenience
> then.

The only evidence I can offer is that libraries currently add padding and specifications such as http://tools.ietf.org/html/draft-ietf-httpbis-encryption-encoding-00 do not state that padding should be stripped from keying data. If this is the case, then the spec and all associated documentation should not only specify it, but should emphasize the point so that developers are not confused.
Comment on attachment 8731161 [details] [diff] [review]
Decoder.patch

Looks like I have so long review queue right now, that perhaps baku could help here.
Attachment #8731161 - Flags: review?(bugs) → review?(amarchesini)
Comment on attachment 8731161 [details] [diff] [review]
Decoder.patch

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

::: dom/push/PushServiceWebSocket.jsm
@@ +946,5 @@
>            update);
>          return;
>        }
> +      let message = ChromeUtils.base64URLDecode(update.data, {
> +        padded: true,

Is this because the server pads messages?  A comment might help.

::: dom/webidl/ThreadSafeChromeUtils.webidl
@@ +70,5 @@
> +  /**
> +   * Decodes a Base64 URL-encoded string per RFC 4648.
> +   *
> +   * @param string The string to decode.
> +   * @param options Specifies whether the input is padded.

You need to be careful here.  There are three states on a decode:

MUST have padding
MAY have padding
MUST NOT have padding

You have implemented the latter two.  In comparison, atob() implements the first.

If you want to be complete, you need a tri-state: padding: [require, ignore, reject].

::: xpcom/io/Base64.h
@@ +44,5 @@
> +                        nsACString& aString);
> +
> +/**
> + * Converts |aData| to a padded Base64 URL-encoded string per RFC 4648.
> + * |aString| may contain trailing "=" padding characters. The caller may free

"may" free |aData| or "must" free |aData|...
Attachment #8731161 - Flags: review?(martin.thomson) → review+
(In reply to Martin Thomson [:mt:] from comment #24)
> If you want to be complete, you need a tri-state: padding: [require, ignore,
> reject].

I'm a bit hesitant to generalize until we need it, but an enum would certainly be more descriptive: we can just restrict it to "require" or "reject". For encoding, we can use `{ pad: true }` to indicate whether the output should be padded.

> "may" free |aData| or "must" free |aData|...

I meant something like "the caller retains ownership of |aData|", or "Encode* does not take ownership of |aData|". So I'll just say that instead.
Comment on attachment 8731161 [details] [diff] [review]
Decoder.patch

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

::: dom/base/ChromeUtils.cpp
@@ +5,5 @@
>  
>  #include "ChromeUtils.h"
>  
>  #include "mozilla/BasePrincipal.h"
> +#include "mozilla/Base64.h"

alphabetic order. move it before BasePrincipal

@@ +71,5 @@
> +    const ArrayBufferView& view = aSource.GetAsArrayBufferView();
> +    view.ComputeLengthAndData();
> +    length = view.Length();
> +    data = view.Data();
> +  }

} else {
  MOZ_CRASH("something");
}

instead having the MOZ_ASSERT at the beginning of this method.

@@ +75,5 @@
> +  }
> +
> +  nsresult rv;
> +  if (aOptions.mPadded) {
> +    rv = mozilla::PaddedBase64URLEncode(length, data, aResult);

this mozilla:: should not be needed.

@@ +79,5 @@
> +    rv = mozilla::PaddedBase64URLEncode(length, data, aResult);
> +  } else {
> +    rv = mozilla::UnpaddedBase64URLEncode(length, data, aResult);
> +  }
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +96,5 @@
> +  FallibleTArray<uint8_t> data;
> +  nsresult rv;
> +
> +  if (aOptions.mPadded) {
> +    rv = mozilla::PaddedBase64URLDecode(aString, data);

no mozilla::

@@ +100,5 @@
> +    rv = mozilla::PaddedBase64URLDecode(aString, data);
> +  } else {
> +    rv = mozilla::UnpaddedBase64URLDecode(aString, data);
> +  }
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +106,5 @@
> +    return;
> +  }
> +
> +  JSContext* cx = aGlobal.Context();
> +  aRetval.set(ArrayBuffer::Create(cx, data.Length(), data.Elements()));

This can fail. Check the return value of ArrayBuffer::Create().
In case, return a NS_ERROR_FAILURE.

::: dom/webidl/ThreadSafeChromeUtils.webidl
@@ +70,5 @@
> +  /**
> +   * Decodes a Base64 URL-encoded string per RFC 4648.
> +   *
> +   * @param string The string to decode.
> +   * @param options Specifies whether the input is padded.

Are you planning to apply this comment in a follow up?
In case, file a bug.

::: xpcom/io/Base64.cpp
@@ +407,5 @@
> +
> +  // Allocate a buffer large enough to hold the decoded output. May be 1-2
> +  // bytes over, depending on the final quantum.
> +  uint32_t decodedLength = (sourceLength * 3) / 4;
> +  if (!aOutput.SetCapacity(decodedLength, mozilla::fallible)) {

NS_WARN_IF

@@ +530,5 @@
> +                      nsACString& aString)
> +{
> +  nsresult rv = UnpaddedBase64URLEncode(aLength, aData, aString);
> +
> +  if (NS_SUCCEEDED(rv)) {

do a quick return here:

if (NS_WARN_IF(NS_FAILED(rv))) {
 return rv;
}

...
Attachment #8731161 - Flags: review?(amarchesini) → review+
(In reply to Kit Cambridge [:kitcambridge] from comment #25)
> (In reply to Martin Thomson [:mt:] from comment #24)
> I'm a bit hesitant to generalize until we need it, but an enum would
> certainly be more descriptive: we can just restrict it to "require" or
> "reject". For encoding, we can use `{ pad: true }` to indicate whether the
> output should be padded.

I changed my mind. :-) Added all three for completeness. Interdiff incoming.
Attached patch inter.diffSplinter Review
This builds on attachment 8731161 [details] [diff] [review] and adds the third mode (MUST have padding) to Base64URLDecode. I'm happy to upload the full patch, or push to MozReview if that's easier.

I took smaug's original suggestion and just passed the options dict directly to Base64URL{Decode, Encode}.
Attachment #8733669 - Flags: review?(martin.thomson)
Attachment #8733669 - Flags: review?(amarchesini)
Attached patch WebCrypto.patchSplinter Review
The final CryptoBuffer conversion.
Attachment #8730543 - Attachment is obsolete: true
Attachment #8733670 - Flags: review?(ttaubert)
Comment on attachment 8733669 [details] [diff] [review]
inter.diff

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

Looks fine to me.

::: xpcom/io/Base64.cpp
@@ +427,5 @@
> +      break;
> +
> +    // If we're expecting unpadded input, no need for additional checks.
> +    // `=` isn't in the decode table, so padded strings will fail to decode.
> +    default:

I would simply omit the default.  Then if someone adds another value, this won't compile.  Unless we aren't enabling that warning as an error (which we should).
Attachment #8733669 - Flags: review?(martin.thomson) → review+
(In reply to Martin Thomson [:mt:] from comment #31)
> I would simply omit the default.  Then if someone adds another value, this
> won't compile.  Unless we aren't enabling that warning as an error (which we
> should).

I think it's still possible to hit the default if mPadding is uninitialized. Shouldn't be an issue for JS callers, but I'm concerned about C++ callers. Is that right?
Passing uninitialized memory around is a serious bug that we shouldn't worry about.  If our tools aren't catching that sort of thing, then we have bigger problems.
Whiteboard: btpp-active
Attachment #8733670 - Flags: review?(ttaubert) → review+
(In reply to Martin Thomson [:mt:] from comment #31)
> I would simply omit the default.  Then if someone adds another value, this
> won't compile.  Unless we aren't enabling that warning as an error (which we
> should).

Your concern about passing uninitialized memory makes sense. I changed this to check for `EndGuard_` and error on the warning instead, but, ironically, I then ran into this: https://dxr.mozilla.org/mozilla-central/rev/1f16d3da9280e40ada252acf8110b91ee1edbb08/security/pkix/lib/pkixutil.h#249-268

Going to leave this for now.
Attached patch ValueCasts.patchSplinter Review
Nick, I had to add a couple of casts to js/public/Value.h to squelch MSVC errors. Do these look OK to you?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab3e0b16fe5f
Attachment #8742587 - Flags: review?(nfitzgerald)
Comment on attachment 8742587 [details] [diff] [review]
ValueCasts.patch

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

Thanks!
Attachment #8742587 - Flags: review?(nfitzgerald) → review+
Attachment #8733669 - Flags: review?(amarchesini) → review+
Comment on attachment 8733669 [details] [diff] [review]
inter.diff

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

::: xpcom/io/Base64.h
@@ +8,5 @@
>  #define mozilla_Base64_h__
>  
>  #include "nsString.h"
>  
> +#include "mozilla/dom/ThreadSafeChromeUtilsBinding.h"

xpcom/ doesn't need to depend on DOM bindings or things in dom/ any more than it already does.  Can we please provide a separate xpcom-specific enum to pass to Base64URL{Encode,Decode}, and translate from the DOM version into the xpcom version wherever we need to?
(In reply to Nathan Froyd [:froydnj] from comment #38)
> xpcom/ doesn't need to depend on DOM bindings or things in dom/ any more
> than it already does.  Can we please provide a separate xpcom-specific enum
> to pass to Base64URL{Encode,Decode}, and translate from the DOM version into
> the xpcom version wherever we need to?

Sure. Would you prefer I back out, or take care of this in a follow-up bug?
Blocks: 1266569
(In reply to Kit Cambridge [:kitcambridge] from comment #39)
> (In reply to Nathan Froyd [:froydnj] from comment #38)
> > xpcom/ doesn't need to depend on DOM bindings or things in dom/ any more
> > than it already does.  Can we please provide a separate xpcom-specific enum
> > to pass to Base64URL{Encode,Decode}, and translate from the DOM version into
> > the xpcom version wherever we need to?
> 
> Sure. Would you prefer I back out, or take care of this in a follow-up bug?

A followup bug would be fine.  Thank you!
https://hg.mozilla.org/mozilla-central/rev/eb272cc767ac
https://hg.mozilla.org/mozilla-central/rev/296093225409
https://hg.mozilla.org/mozilla-central/rev/f0131a8cfd62
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1270778
I wonder if it makes sense to note this as a compat issue for Web Crypto. Padded Base64 in JWK was never allowed by the spec, and Chrome rejects with an error for padded inputs:

   crypto.subtle.importKey("jwk",
     { k: "5170d_1XwssOk8tALKYWX82_eI_uvjTvporc5ISKYDo=", kty: "oct" },
     "AES-GCM",
     true,
     ["encrypt"]
   ).then(function(cryptoKey) {
     console.log("Imported key with padding", cryptoKey);
   }, function(error) {
     // Chrome rejects with "DOMException: The JWK member "k" could not be
     // base64url decoded or contained padding".
     console.log("Error importing key", error);
   });

But Firefox <= 47 imported the key successfully, and making it more strict broke Hello in bug 1270778.
Keywords: site-compat
Testing Web Crypto in other engines...

* WebKit doesn't support passing `JsonWebKey` to `crypto.webkitSubtle.importKey` (bug 1270778, comment 9).

* Edge follows Firefox <= 47: it supports standard and URL-safe Base64, and mixing the two. "5170d_1XwssOk8tALKYWX82_eI_uvjTvporc5ISKYDo=", "5170d-1XwssOk8tALKYWX82-eI-uvjTvporc5ISKYDo", and "5170d-1XwssOk8tALKYWX82_eI-uvjTvporc5ISKYDo=====" are all valid.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.