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 →