fix: decode JWT segments as Base64url (RFC 7515) - #978
Conversation
|
Thank you for the patch, looks like an interesting finding. However, I needed pr_978.patch to make it compile over here, please double check and update the PR accordingly. |
Both cleanups are from the review patch on opencloud-eu#978. Co-authored-by: Klaas Freitag <1070214+dragotin@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
82987cf to
08906e5
Compare
|
Thanks for the patch; applied in full and force-pushed. The export macro and Q_SLOTS fixes are squashed into the first commit, the enum name fix into the second, and the two cleanups (duplicated word in the log message, redundant QtTest include) are a separate third commit. The misses came from verifying only testjwt locally, on a Linux build with default symbol visibility and without QT_NO_KEYWORDS. I re-ran testjwt with QT_NO_KEYWORDS defined: 6/6 pass. |
dragotin
left a comment
There was a problem hiding this comment.
Looks now good to me, lets see what @TheOneRing thinks.
| auto parse = [](const QByteArray &part) { | ||
| const auto decoded = QByteArray::fromBase64Encoding(part, QByteArray::Base64UrlEncoding | QByteArray::AbortOnBase64DecodingErrors); | ||
| if (!decoded) { | ||
| return QJsonObject{}; |
There was a problem hiding this comment.
Please add a log line including the QByteArray::Base64DecodingStatus
There was a problem hiding this comment.
Added a qCWarning under a new sync.credentials.jwt category logging the Base64DecodingStatus on decode failure. Status only (no segment bytes) to keep token contents out of the logs. testjwt still 6/6.
JWT::JWT() decoded segments with standard-alphabet Base64; Qt's default permissive mode silently skips base64url's '-' and '_' characters, corrupting every byte after them. A corrupted payload fails JSON parsing and OIDC login aborts with a misleading audience error (opencloud-eu#977). Decode with Base64UrlEncoding and abort on invalid characters, so a header or payload segment that is not valid base64url yields an invalid JWT instead of silently corrupted claims. serialize() already used Base64UrlEncoding; only parsing was wrong. Fixes: opencloud-eu#977 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An id_token with an undecodable header or payload segment previously fell through to the audience check and failed with 'The audience of the id_token did not contain ...'. That message blames the IdP client configuration when the token never parsed. Check JWT validity first and report a distinct parse error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both cleanups are from the review patch on opencloud-eu#978. Co-authored-by: Klaas Freitag <1070214+dragotin@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds a sync.credentials.jwt logging category so a rejected Base64url segment is diagnosable instead of failing silently.
d53f81d to
f243379
Compare
TheOneRing
left a comment
There was a problem hiding this comment.
The crash in the unit test is unrelated.
Thank you for your contribution.
Both cleanups are from the review patch on #978. Co-authored-by: Klaas Freitag <1070214+dragotin@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #977
What
JWT::JWT()decoded token segments withQByteArray::fromBase64(part), which uses the standard Base64 alphabet. JWT segments are Base64url (RFC 7515 section 2). Qt's defaultIgnoreBase64DecodingErrorsmode silently skips-and_as invalid characters and corrupts every byte after them. The payload then fails JSON parsing,IdToken::aud()returns an empty string, and OIDC login fails with a misleading "The audience of the id_token did not contain …" error. See #977 for the full analysis, including why the failure depends on token content and looked IdP-specific.Changes
Commit 1:
jwt.cpp: decode segments withBase64UrlEncoding | AbortOnBase64DecodingErrorsviafromBase64Encoding. A header or payload segment that is not valid base64url now yields an empty object, and so an invalid JWT, instead of silently corrupted claims.serialize()already usedBase64UrlEncoding; only parsing was wrong.test/testjwt.cpp(new): regression tests. They cover a payload whose base64url encoding contains both-and_, a serialize/parse round-trip, structurally malformed tokens, and strict rejection of an invalid character inside an otherwise valid segment (the permissive decode wrongly accepted that case). Against the unpatched code, three of the four fail; with this change all pass.Commit 2:
oauth.cpp: an unparseable id_token previously fell through to the audience check and produced the misleading audience error. The token exchange handler now retains the parsedJWTand reports "The id_token could not be parsed" before checking the audience.test/testoauth/testoauth.cpp: newtestUnparseableIdTokencorrupts the header and payload segments in turn, and expects the parse error rather than a login or the audience message.Notes
+or/in JWT segments. RFC 7515 forbids those characters, but this is a behavior change for such non-compliant IdPs.testjwttarget standalone and ran it against both the patched and unpatched decoder; I did not run the full suite build in my environment.