Fix ArrayIndexOutOfBoundsException in Base64Url on padding-only input - #51217
Open
sergioperezcheco wants to merge 2 commits into
Open
Fix ArrayIndexOutOfBoundsException in Base64Url on padding-only input#51217sergioperezcheco wants to merge 2 commits into
sergioperezcheco wants to merge 2 commits into
Conversation
encodeBase64ToBase64Url used base64.split("=")[0] to strip trailing
padding. When the input consists entirely of '=' characters (e.g. "="
or "=="), String.split discards trailing empty strings and returns a
zero-length array, so the [0] access throws ArrayIndexOutOfBoundsException.
This propagated through Base64Url.decode and JOSEParser.parse when
processing malformed JOSE compact tokens with padding-only segments.
Guard the array access: take the first segment only when the array is
non-empty, otherwise return an empty string (the correct Base64Url
representation of content that was all padding).
Fixes keycloak#51212
Signed-off-by: sergioperezcheco <checo520@outlook.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes an ArrayIndexOutOfBoundsException in Base64Url.encodeBase64ToBase64Url when the input is padding-only (e.g. "=", "=="), which can surface when parsing malformed JOSE compact tokens.
Changes:
- Guarded access to the first element of
base64.split("=")to handle padding-only inputs safely. - Added unit tests covering normal encoding/decoding, base64 charset conversion, padding-only inputs, and empty strings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| common/src/main/java/org/keycloak/common/util/Base64Url.java | Protects against split returning a zero-length array for padding-only inputs. |
| common/src/test/java/org/keycloak/common/util/Base64UrlTest.java | Adds regression and behavioral tests for encoding/decoding and padding-only edge cases. |
Comment on lines
+47
to
+52
| // Remove any trailing '='s. String.split discards trailing empty strings, | ||
| // so a padding-only input (e.g. "=" or "==") produces an empty array whose | ||
| // first element cannot be accessed. Take the first segment only when the | ||
| // array is non-empty; otherwise there is no content before the padding. | ||
| String[] parts = base64.split("="); | ||
| String s = parts.length > 0 ? parts[0] : ""; |
| @Test | ||
| public void decode_normalInput() { | ||
| byte[] result = Base64Url.decode("dGVzdA"); | ||
| assertThat(new String(result), equalTo("test")); |
| // decode() routes through encodeBase64ToBase64Url, so padded Base64 | ||
| // input must still work. | ||
| byte[] result = Base64Url.decode("dGVzdA=="); | ||
| assertThat(new String(result), equalTo("test")); |
…in tests
Switch encodeBase64ToBase64Url to indexOf('=') + substring as suggested in
review — avoids the regex engine and array allocation of String.split and
still handles padding-only input cleanly. Also pass StandardCharsets.UTF_8
to String constructors in the test assertions instead of relying on the
platform default charset.
Signed-off-by: sergioperezcheco <checo520@outlook.com>
Author
|
Updated both points: switched to indexOf('=') + substring so we skip the regex/array allocation entirely, and the test assertions now pass StandardCharsets.UTF_8 explicitly. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
common/src/main/java/org/keycloak/common/util/Base64Url.java:53
- The explanation about padding-only input is incorrect/misleading: for
"="/"==",indexOf('=')returns0(not-1) andsubstring(0, 0)yields""(there is no trimming involved). Please adjust the comment to describe theidx == 0behavior (or remove the-1/trimming explanation) so future readers don’t misunderstand why this fixes the exception.
// Strip trailing Base64 padding ('=') by cutting at the first '='.
// indexOf avoids the regex + array allocation of String.split and also
// handles padding-only input (e.g. "=" or "==") without an
// ArrayIndexOutOfBoundsException — indexOf returns -1, so we keep the full string,
// which is empty after trimming when the input was all '=' characters.
int idx = base64.indexOf('=');
String s = idx >= 0 ? base64.substring(0, idx) : base64;
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
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.
Base64Url.encodeBase64ToBase64Url strips trailing '=' padding with base64.split("=")[0]. When the input is entirely padding (e.g. "=" or "=="), String.split discards trailing empty strings and returns a zero-length array, so the [0] access throws ArrayIndexOutOfBoundsException. This surfaces through Base64Url.decode and JOSEParser.parse when processing malformed JOSE compact tokens that contain padding-only segments.
The fix guards the array access — the first segment is taken only when the array is non-empty, otherwise an empty string is returned, which is the correct Base64Url representation of content that was all padding.
Added Base64UrlTest covering normal inputs, charset conversion, the padding-only edge cases, empty strings, and the decode path.
Fixes #51212