Add Base64-URL-encoding and Base64-URL-decoding to XPCOM

RESOLVED WORKSFORME

Status

()

enhancement
RESOLVED WORKSFORME
7 years ago
7 months ago

People

(Reporter: briansmith, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:-)

Details

Attachments

(1 attachment)

+++ This bug was initially created as a clone of Bug #769519 +++
+++ This bug was initially created as a clone of Bug #753238 +++

toolkit/identity needs the ability to base64-URL-encode a raw byte array of arbitrary length as a string. For testing it will probably be eventually useful to be able to base64-URL-decode too. toolkit/identity/IdentityCryptoService.cpp already contains a function to do base64-URL-encoding built on top of XPCOM's Base64 encoder; we should move that functionality to XPCOM so it is available for every module.

Note that Identity needs base64-url-encoding to be exposed to JS, so perhaps we should also expose the base64 functionality through Components.utils?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> What's wrong with atob/btoa?

It must be callable from native code and it must be base64-URL-encoding, not regular base64.

Also, I don't understand the "Character out of range" problem mentioned in the MDN documentation for atob and btoa.
This seems to have carried the blocking basecamp forward - is it really blocking basecamp?
blocking-basecamp: + → ?
Sorry, I should really stop using the clone feature of Bugzilla. It is not blocking Basecamp.
basecamp-ing based on Brian's comment.  Thanks.
blocking-basecamp: ? → -
Is 
  https://mxr.mozilla.org/mozilla-central/source/xpcom/io/Base64.cpp#239
the best place for the new functions Base64UrlEncode and Base64UrlDecode?

Write PL_Base64UrlEncode and PL_Base64UrlDecode first?
There's no need to write a PL_Base64Url[En|De]code.  Just implement this in xpcom directly.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #7)
> There's no need to write a PL_Base64Url[En|De]code.  Just implement this in
> xpcom directly.

I don't follow you. What does "implement directly in xpcom" mean here?
Put the implementation in xpcom/io/Base64.h/cpp.  There's no need to implement functions in NSPR and then write wrappers around them in xpcom/.
Four new functions to base64url encode and decode.

I don't understand how the base64 tests work. So this is untested. Have fun.
Axel
(In reply to Axel Nennker from comment #10)
> Created attachment 658446 [details] [diff] [review]
> 4 new functions implementing base64url (en|de)coding
> 
> Four new functions to base64url encode and decode.
> 
> I don't understand how the base64 tests work. So this is untested. Have fun.
> Axel

This patch may not get review without a test. Apparently, khuey is the test author. khuey: Do we need a new test function that consumes base64url-encoded strings?  http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestBase64.cpp ?
xpcom/tests/TestBase64.cpp tests the base64 stream encoder only.
I think that TestBase64.cpp is not the right place to test base64url.

Maybe we should add base64url encoding and decoding to
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#617 and test it there?

nsresult
nsContentUtils::Base64UrlEncode(const nsAString& aBinaryData,
                     nsAString& aAsciiBase64String)
{
  if (!Is8bit(aBinaryData)) {
    aAsciiBase64String.Truncate();
    return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
  }

  return Base64UrlEncode(aBinaryData, aAsciiBase64String);
}

nsresult
nsContentUtils::Base64UrlDecode(const nsAString& aAsciiBase64String,
                     nsAString& aBinaryData)
{
  if (!Is8bit(aAsciiBase64String)) {
    aBinaryData.Truncate();
    return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
  }

  nsresult rv = Base64UrlDecode(aAsciiBase64String, aBinaryData);
  if (NS_FAILED(rv) && rv == NS_ERROR_INVALID_ARG) {
    return NS_ERROR_DOM_INVALID_CHARACTER_ERR;
  }
  return rv;
}

It might make sense to prefix the function first like...
mozBase64UrlEncode
An encoder function was added in https://hg.mozilla.org/mozilla-central/rev/dda92c46bb23 so we are halfway there.
(In reply to Martin Thomson [:mt:] from comment #13)
> An encoder function was added in
> https://hg.mozilla.org/mozilla-central/rev/dda92c46bb23 so we are halfway
> there.

And then the decoder function was added in bug 1256488, so I think we are done here.
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.