Stop Base64url.parse from leaking its reverse map across urlSafe calls - #540
Open
afonsojanu wants to merge 1 commit into
Open
Stop Base64url.parse from leaking its reverse map across urlSafe calls#540afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
parse() memoized its reverse-lookup table on the shared Base64url object the first time it ran, but the table only holds entries for whichever map (safe or non-safe) built it. A later call with a different urlSafe value kept reusing that stale table instead of building its own, so decoding a url-safe string right after a non-safe call (or the other way around) silently produced the wrong bytes for any character that only exists in one of the two maps. Cached the reverse map per variant instead (_safeReverseMap and _reverseMap) so each urlSafe value gets its own table, same as stringify() already keeps _safe_map and _map separate. Added enc-base64url-test.js, which didn't exist yet, covering both directions of the original bug plus the existing stringify/parse round trip for both map variants.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #531.
parse() memoizes its reverse-lookup table directly on the shared Base64url object the first time it runs, but that table only has entries for whichever map (safe or non-safe) built it. Once cached, every later call reused the same table regardless of the urlSafe argument, so a non-safe parse() followed by a safe one (or vice versa) silently decoded any character unique to the other map into garbage instead of throwing or failing loudly.
I confirmed this with a small standalone repro before touching anything: encoding a byte whose base64url form uses
-, callingparse('AAAA', false)first to prime the cache with the non-safe table, then parsing the--containing string back. Unpatched, it decodes to the wrong bytes (re-encoding givesAwinstead of the original-w). With the fix applied, it round-trips correctly.The fix keeps two separate cached tables (
_safeReverseMapand_reverseMap) instead of one, mirroring how stringify() already keeps_safe_mapand_mapapart, so nothing about the memoization behavior changes other than making it correct per variant.There wasn't an existing test file for Base64url at all (only enc-base64-test.js for the non-url variant), so I added test/enc-base64url-test.js in the same YUI Test style, covering stringify/parse for both map variants plus the specific cross-contamination scenario from the issue in both directions.
On verification: this repo's
npm testonly runs jsonlint + jshint (no automated functional test runner is wired up here, and there's no test/index.html in this checkout to drive the YUI tests in a browser), so I couldn't run the added test file through the project's own harness. I did verify the fix and the added test's assertions manually against both the unpatched and patched source with a small Node script requiring src/core.js and src/enc-base64url.js directly, and ran jshint against the changed source file with the repo's own .jshintrc, which passes clean.