fix(core): bound credential label and stop demos rendering it as HTML - #147
Merged
Conversation
wolpert
force-pushed
the
fix/credential-label-xss
branch
from
August 5, 2026 15:11
af2161c to
0eba441
Compare
wolpert
enabled auto-merge (rebase)
August 5, 2026 15:19
wolpert
force-pushed
the
fix/credential-label-xss
branch
2 times, most recently
from
August 5, 2026 15:26
d3cf7dd to
9b89b33
Compare
The credential label is the one free-text, host-visible field a caller controls:
set at register/finish, changed via PATCH /auth/admin/credentials/{id}, stored
verbatim, and echoed back by AdminService.listCredentials. It was validated only
for blankness — no length bound, no charset — and all three demos interpolated it
straight into innerHTML:
li.innerHTML = `<b>${cred.label}</b> ...`;
so `<img src=x onerror=...>` stored in a label executed on render, and the demos
hold the access JWT in localStorage. In the demo itself that is self-XSS, which is
not why it matters: this is the reference rendering of listCredentials output, and
that same call backs any real admin or support console — where an attacker-set
label on their own credential fires in a staff member's session.
Both ends are fixed. The demos build the row from DOM nodes and assign the label
through textContent (buttons carry their ids via dataset, so there is no attribute
sink either). The library caps the label at CredentialRecord.MAX_LABEL_LENGTH (64
— it is a nickname like "MacBook Touch ID"), surfacing an over-long value as
RegistrationResult.InvalidPayload and AdminResult.ValidationFailed so it lands as
a clean 400 rather than an exception crossing the sealed-result boundary.
The registration check runs ahead of the challenge preflight deliberately:
takeOnce is single-use, so validating first means a rejected label doesn't burn
the challenge and force a full ceremony restart. A test pins that ordering.
The cap is a storage/DoS bound, not an escaping mechanism — the constant's javadoc
says so explicitly, because a length limit must not be mistaken for output safety.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wolpert
force-pushed
the
fix/credential-label-xss
branch
from
August 5, 2026 15:30
9b89b33 to
1034a26
Compare
|
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.
Closes finding 2 of the full-project security audit.
The problem
The credential label is the one free-text, host-visible field a caller controls — set at
register/finish, changed viaPATCH /auth/admin/credentials/{id}, stored verbatim, echoed back byAdminService.listCredentials. It was validated only for blankness (no length bound, no charset), and all three demos interpolated it straight intoinnerHTML:So
<img src=x onerror=…>stored in a label executes on render — and the demos keep the access JWT inlocalStorage.In the demo itself this is self-XSS, which is not why it matters. This is the reference rendering of
listCredentialsoutput, and that same call backs any real admin or support console — where an attacker-set label on their own credential fires in a staff member's session.The fix
Demos build the row from DOM nodes and assign the label through
textContent. Buttons carry their ids viadataset, so there's no attribute sink either.Library caps the label at
CredentialRecord.MAX_LABEL_LENGTH(64 — it's a nickname like "MacBook Touch ID"), surfacing an over-long value asRegistrationResult.InvalidPayloadandAdminResult.ValidationFailed, so it lands as a clean 400 rather than an exception crossing the sealed-result boundary.The registration check runs ahead of the challenge preflight deliberately:
takeOnceis single-use, so validating first means a rejected label doesn't burn the challenge and force a full ceremony restart.overlongLabelIsInvalidPayloadAndDoesNotConsumeTheChallenge()pins that ordering.Note on the cap
The length bound is a storage/DoS control, not an escaping mechanism — the constant's javadoc says so explicitly. A label is untrusted text and renderers must still escape for their output context; conflating the two is how the demo bug happens again.
Testing
overlongLabelIsInvalidPayloadAndDoesNotConsumeTheChallenge(),labelExactlyAtTheBoundIsAccepted(),renameCredentialOverlongLabelRejected()(including the at-limit boundary)../gradlew :pk-auth-core:check :pk-auth-admin-api:checkpasses.🤖 Generated with Claude Code