From 1034a26a9a37c52a88f1ccf707e3894254702c65 Mon Sep 17 00:00:00 2001 From: Ned Wolpert Date: Wed, 5 Aug 2026 07:58:53 -0700 Subject: [PATCH] fix(core): bound credential label and stop demos rendering it as HTML MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 = `${cred.label} ...`; so `` 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) --- CHANGELOG.md | 12 ++++++ .../src/main/resources/assets/demo.js | 40 +++++++++++++++---- .../src/main/resources/public/demo.js | 40 +++++++++++++++---- .../src/main/resources/static/demo.js | 40 +++++++++++++++---- .../pkauth/admin/DefaultAdminService.java | 4 ++ .../pkauth/admin/DefaultAdminServiceTest.java | 13 ++++++ .../pkauth/credential/CredentialRecord.java | 17 ++++++++ .../DefaultPasskeyAuthenticationService.java | 12 ++++++ ...AuthenticationServiceRegistrationTest.java | 29 ++++++++++++++ 9 files changed, 186 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0447d4f..12a250c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,18 @@ uniqueness semantics — read the entry below before upgrading. by case. Choosing which row is authoritative and what becomes of the other's credentials is a business decision, not one a schema change should make silently. Resolve the listed conflicts and re-run. +- **Credential labels are bounded and no longer rendered as HTML by the demos.** + The label is the one free-text field an unauthenticated caller controls (set at + `register/finish`, changed via `PATCH /auth/admin/credentials/{id}`), it was + validated only for blankness, and all three demos interpolated it straight into + `innerHTML` when rendering the credential list — so markup stored in a label + executed on render, and the demos keep the access token in `localStorage`. The + demos now build the row from DOM nodes and assign the label via `textContent`, + and the library caps the label at `CredentialRecord.MAX_LABEL_LENGTH` (64 + chars), returning `RegistrationResult.InvalidPayload` / `AdminResult + .ValidationFailed` — a clean 400, never a thrown exception across the sealed + result boundary. The bound is checked *before* the challenge preflight, so a + rejected label does not burn the single-use challenge. ## [2.2.0] — 2026-06-27 diff --git a/examples/dropwizard-demo/src/main/resources/assets/demo.js b/examples/dropwizard-demo/src/main/resources/assets/demo.js index 69bae33..abefeea 100644 --- a/examples/dropwizard-demo/src/main/resources/assets/demo.js +++ b/examples/dropwizard-demo/src/main/resources/assets/demo.js @@ -92,18 +92,44 @@ $("btn-account").addEventListener("click", () => $("btn-register-again").addEventListener("click", () => $("btn-register").click()); +// The credential label is untrusted text: it is whatever the caller supplied at +// register/finish or via PATCH /auth/admin/credentials/{id}, stored verbatim and echoed back by +// listCredentials. Build the row from DOM nodes and set the label through textContent — +// interpolating it into innerHTML would execute markup that an attacker stored in their own label, +// which in a real admin console means it fires in a staff member's session. +function credButton(action, text, credentialId) { + const button = document.createElement("button"); + button.textContent = text; + button.dataset.action = action; + button.dataset.id = credentialId; + return button; +} + +function credentialListItem(cred) { + const li = document.createElement("li"); + const name = document.createElement("b"); + name.textContent = cred.label; + const id = document.createElement("small"); + id.textContent = `${cred.credentialId.slice(0, 16)}…`; + li.append( + name, + " ", + id, + " ", + credButton("rename", "Rename", cred.credentialId), + " ", + credButton("delete", "Delete", cred.credentialId), + ); + return li; +} + $("btn-creds").addEventListener("click", () => run("login-out", async () => { const list = await pk.admin.listCredentials(); const ul = $("cred-list"); - ul.innerHTML = ""; + ul.replaceChildren(); for (const cred of list) { - const li = document.createElement("li"); - li.innerHTML = - `${cred.label} ${cred.credentialId.slice(0, 16)}… ` + - ` ` + - ``; - ul.appendChild(li); + ul.appendChild(credentialListItem(cred)); } }), ); diff --git a/examples/micronaut-demo/src/main/resources/public/demo.js b/examples/micronaut-demo/src/main/resources/public/demo.js index 05006c7..c5f6dc7 100644 --- a/examples/micronaut-demo/src/main/resources/public/demo.js +++ b/examples/micronaut-demo/src/main/resources/public/demo.js @@ -101,18 +101,44 @@ $("btn-account").addEventListener("click", () => $("btn-register-again").addEventListener("click", () => $("btn-register").click()); +// The credential label is untrusted text: it is whatever the caller supplied at +// register/finish or via PATCH /auth/admin/credentials/{id}, stored verbatim and echoed back by +// listCredentials. Build the row from DOM nodes and set the label through textContent — +// interpolating it into innerHTML would execute markup that an attacker stored in their own label, +// which in a real admin console means it fires in a staff member's session. +function credButton(action, text, credentialId) { + const button = document.createElement("button"); + button.textContent = text; + button.dataset.action = action; + button.dataset.id = credentialId; + return button; +} + +function credentialListItem(cred) { + const li = document.createElement("li"); + const name = document.createElement("b"); + name.textContent = cred.label; + const id = document.createElement("small"); + id.textContent = `${cred.credentialId.slice(0, 16)}…`; + li.append( + name, + " ", + id, + " ", + credButton("rename", "Rename", cred.credentialId), + " ", + credButton("delete", "Delete", cred.credentialId), + ); + return li; +} + $("btn-creds").addEventListener("click", () => run("login-out", async () => { const list = await pk.admin.listCredentials(); const ul = $("cred-list"); - ul.innerHTML = ""; + ul.replaceChildren(); for (const cred of list) { - const li = document.createElement("li"); - li.innerHTML = - `${cred.label} ${cred.credentialId.slice(0, 16)}… ` + - ` ` + - ``; - ul.appendChild(li); + ul.appendChild(credentialListItem(cred)); } }), ); diff --git a/examples/spring-boot-demo/src/main/resources/static/demo.js b/examples/spring-boot-demo/src/main/resources/static/demo.js index 05006c7..c5f6dc7 100644 --- a/examples/spring-boot-demo/src/main/resources/static/demo.js +++ b/examples/spring-boot-demo/src/main/resources/static/demo.js @@ -101,18 +101,44 @@ $("btn-account").addEventListener("click", () => $("btn-register-again").addEventListener("click", () => $("btn-register").click()); +// The credential label is untrusted text: it is whatever the caller supplied at +// register/finish or via PATCH /auth/admin/credentials/{id}, stored verbatim and echoed back by +// listCredentials. Build the row from DOM nodes and set the label through textContent — +// interpolating it into innerHTML would execute markup that an attacker stored in their own label, +// which in a real admin console means it fires in a staff member's session. +function credButton(action, text, credentialId) { + const button = document.createElement("button"); + button.textContent = text; + button.dataset.action = action; + button.dataset.id = credentialId; + return button; +} + +function credentialListItem(cred) { + const li = document.createElement("li"); + const name = document.createElement("b"); + name.textContent = cred.label; + const id = document.createElement("small"); + id.textContent = `${cred.credentialId.slice(0, 16)}…`; + li.append( + name, + " ", + id, + " ", + credButton("rename", "Rename", cred.credentialId), + " ", + credButton("delete", "Delete", cred.credentialId), + ); + return li; +} + $("btn-creds").addEventListener("click", () => run("login-out", async () => { const list = await pk.admin.listCredentials(); const ul = $("cred-list"); - ul.innerHTML = ""; + ul.replaceChildren(); for (const cred of list) { - const li = document.createElement("li"); - li.innerHTML = - `${cred.label} ${cred.credentialId.slice(0, 16)}… ` + - ` ` + - ``; - ul.appendChild(li); + ul.appendChild(credentialListItem(cred)); } }), ); diff --git a/pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java b/pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java index 36e5394..e088a58 100644 --- a/pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java +++ b/pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java @@ -122,6 +122,10 @@ public AdminResult renameCredential( if (newLabel == null || newLabel.isBlank()) { return new AdminResult.ValidationFailed<>("label must be non-blank"); } + if (newLabel.length() > CredentialRecord.MAX_LABEL_LENGTH) { + return new AdminResult.ValidationFailed<>( + "label must be at most " + CredentialRecord.MAX_LABEL_LENGTH + " characters"); + } Optional cred = credentialRepository.findByCredentialId(credentialId); if (cred.isEmpty() || !cred.get().userHandle().equals(target)) { return new AdminResult.NotFound<>(); diff --git a/pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java b/pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java index a6db870..2b57bcd 100644 --- a/pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java +++ b/pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java @@ -198,6 +198,19 @@ void renameCredentialBlankLabelRejected() { .isInstanceOf(AdminResult.ValidationFailed.class); } + @Test + void renameCredentialOverlongLabelRejected() { + saveCredential(alice, new byte[] {1}); + String tooLong = "x".repeat(CredentialRecord.MAX_LABEL_LENGTH + 1); + assertThat(admin.renameCredential(alice, alice, CredentialId.of(new byte[] {1}), tooLong)) + .isInstanceOf(AdminResult.ValidationFailed.class); + + // Exactly at the bound is still accepted. + String atLimit = "x".repeat(CredentialRecord.MAX_LABEL_LENGTH); + assertThat(admin.renameCredential(alice, alice, CredentialId.of(new byte[] {1}), atLimit)) + .isInstanceOf(AdminResult.Success.class); + } + @Test void renameCredentialOfOtherUserNotFound() { UserHandle bob = users.register("bob", "Bob"); diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/credential/CredentialRecord.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/credential/CredentialRecord.java index 5057302..c385779 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/credential/CredentialRecord.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/credential/CredentialRecord.java @@ -43,6 +43,23 @@ public record CredentialRecord( Instant createdAt, @Nullable Instant lastUsedAt) { + /** + * Maximum accepted length of {@link #label}, in {@code char}s. + * + *

The label is the one free-text, host-visible field an unauthenticated caller controls (set + * at {@code register/finish}, changed via {@code PATCH /auth/admin/credentials/{id}}) and it is + * persisted and echoed back by {@code AdminService.listCredentials}. Bounding it keeps an + * arbitrarily large blob out of storage and out of every UI that renders a credential list. It is + * a nickname ("Work laptop", "MacBook Touch ID"), so 64 is generous. + * + *

This is a length bound only — it is not an escaping mechanism. A label is untrusted + * text: renderers MUST escape it for their output context (see the demos' credential list, which + * builds DOM nodes with {@code textContent} rather than interpolating into {@code innerHTML}). + * + * @since 2.3.0 + */ + public static final int MAX_LABEL_LENGTH = 64; + public CredentialRecord { Objects.requireNonNull(credentialId, "credentialId"); Objects.requireNonNull(userHandle, "userHandle"); diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java index cae68a1..13597c7 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationService.java @@ -293,6 +293,18 @@ public RegistrationResult finishRegistration( new RegistrationResult.RateLimited("ip"), start); } + // Step 0: bound the caller-supplied label before anything else. Checked ahead of the challenge + // preflight on purpose — takeOnce is single-use, so validating first means an over-long label + // doesn't burn the challenge and force a full ceremony restart. + String label = req.label(); + if (label != null && label.length() > CredentialRecord.MAX_LABEL_LENGTH) { + return outcome( + ChallengeValidator.Ceremony.REGISTRATION, + new RegistrationResult.InvalidPayload( + "label must be at most " + CredentialRecord.MAX_LABEL_LENGTH + " characters"), + start); + } + // Step 1: challenge / origin / ceremony-type preflight. ChallengeValidation validation = challengeValidator.validate( diff --git a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationServiceRegistrationTest.java b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationServiceRegistrationTest.java index 419261e..da122e5 100644 --- a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationServiceRegistrationTest.java +++ b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/internal/DefaultPasskeyAuthenticationServiceRegistrationTest.java @@ -174,6 +174,35 @@ void happyPathPersistsCredentialWithTransportsAndAaguidAndLabel() throws Excepti verify(metrics).incrementCounter("pkauth.registration.outcome", "result", "Success"); } + @Test + void overlongLabelIsInvalidPayloadAndDoesNotConsumeTheChallenge() { + String tooLong = "x".repeat(CredentialRecord.MAX_LABEL_LENGTH + 1); + + RegistrationResult result = service.finishRegistration(finishReg(cd(), tooLong)); + + assertThat(result) + .isInstanceOfSatisfying( + RegistrationResult.InvalidPayload.class, p -> assertThat(p.detail()).contains("64")); + // The label is bounded ahead of the preflight, so the single-use challenge survives a + // rejected label rather than forcing the user through a fresh ceremony. + verify(challengeStore, never()).takeOnce(any()); + verify(credentialRepository, never()).save(any()); + } + + @Test + void labelExactlyAtTheBoundIsAccepted() throws Exception { + RegistrationData regData = mockRegistrationData(AAGUID.ZERO, null, false); + when(webAuthnManager.verify( + any(com.webauthn4j.data.RegistrationRequest.class), any(RegistrationParameters.class))) + .thenReturn(regData); + String atLimit = "x".repeat(CredentialRecord.MAX_LABEL_LENGTH); + + assertThat(service.finishRegistration(finishReg(cd(), atLimit))) + .isInstanceOfSatisfying( + RegistrationResult.Success.class, + s -> assertThat(s.credential().label()).isEqualTo(atLimit)); + } + @Test void happyPathWithZeroAaguidNullTransportsAndDefaultLabel() throws Exception { // AAGUID.ZERO → stored aaguid is null; null transports → empty transport set; null label →