diff --git a/src/identity/markup.ts b/src/identity/markup.ts index dd085e5..adcdfc6 100644 --- a/src/identity/markup.ts +++ b/src/identity/markup.ts @@ -1,9 +1,31 @@ +const UNSAFE_ATTRIBUTE_VALUE = /["\r\n]|>>/; + +/** + * Emits `authorId` alongside `author` so a display name survives independently + * of the identity provider that minted the ID. Marks whose ID and name are the + * same value, or that have only one of the two, keep the single `author` + * attribute. + */ export function formatAuthoredComment( content: string, authorId?: string, + authorName?: string, ): string { - if (!authorId || /["\r\n]|>>/.test(authorId)) { - return `{>>${content}<<}`; + const id = safeAttributeValue(authorId); + const name = safeAttributeValue(authorName); + if (!id) { + return name ? `{{author="${name}">>${content}<<}}` : `{>>${content}<<}`; } - return `{{author="${authorId}">>${content}<<}}`; + if (!name || name === id) { + return `{{author="${id}">>${content}<<}}`; + } + + return `{{authorId="${id}" author="${name}">>${content}<<}}`; +} + +function safeAttributeValue(value?: string): string | undefined { + const trimmed = value?.trim(); + if (!trimmed || UNSAFE_ATTRIBUTE_VALUE.test(trimmed)) return undefined; + + return trimmed; } diff --git a/src/main.ts b/src/main.ts index 31d1503..83c817d 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1539,7 +1539,11 @@ export default class RelayCommentsPlugin identity: ReviewerIdentity, ): string { const content = sanitizeCommentText(comment); - return formatAuthoredComment(content, identity.id); + return formatAuthoredComment( + content, + identity.id, + identity.source === "fallback" ? undefined : identity.name, + ); } private formatAttachedCommentMarkup( diff --git a/tests/unit/identity/markup.test.ts b/tests/unit/identity/markup.test.ts index bbc3fc5..640c026 100644 --- a/tests/unit/identity/markup.test.ts +++ b/tests/unit/identity/markup.test.ts @@ -2,12 +2,24 @@ import { describe, expect, it } from "@jest/globals"; import { formatAuthoredComment } from "src/identity/markup"; describe("formatAuthoredComment", () => { - it("writes only the provider's service ID", () => { + it("writes the service ID and the display name", () => { + expect(formatAuthoredComment("Looks good", "123456", "Bongo Cat")).toBe( + '{{authorId="123456" author="Bongo Cat">>Looks good<<}}', + ); + }); + + it("writes only the provider's service ID when no name is available", () => { expect(formatAuthoredComment("Looks good", "123456")).toBe( '{{author="123456">>Looks good<<}}', ); }); + it("collapses a name identical to the service ID to one attribute", () => { + expect(formatAuthoredComment("Looks good", "Bongo Cat", "Bongo Cat")).toBe( + '{{author="Bongo Cat">>Looks good<<}}', + ); + }); + it("supports a literal display name as the author value", () => { expect(formatAuthoredComment("Looks good", "Bongo Cat")).toBe( '{{author="Bongo Cat">>Looks good<<}}', @@ -23,4 +35,16 @@ describe("formatAuthoredComment", () => { "{>>Looks good<<}", ); }); + + it("keeps a safe name when only the service ID is unsafe", () => { + expect(formatAuthoredComment("Looks good", 'bad"id', "Bongo Cat")).toBe( + '{{author="Bongo Cat">>Looks good<<}}', + ); + }); + + it("drops an unsafe name and keeps the service ID", () => { + expect(formatAuthoredComment("Looks good", "123456", 'bad"name')).toBe( + '{{author="123456">>Looks good<<}}', + ); + }); });