feat(person-mention): reject non-UUID personIds at the renderer boundary
Nora's CWE-601 (Open Redirect) defense-in-depth concern: today the backend
emits UUIDs, but renderTranscriptionBody concatenates personId straight into
an href. If a future "external person" feature ever flows a non-UUID through
the sidecar, the renderer would happily emit `<a href="javascript:…">`.
Add a strict UUID regex check before substituting. Non-UUID entries fall
through unchanged so the @-trigger remains as plain text — no silent data
loss, no clickable redirect.
Three new failing→passing tests cover javascript: scheme, absolute URL, and
the positive case (well-formed UUID still renders). Existing tests that used
synthetic IDs ("p-short", "p-first", etc.) updated to real UUIDs.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -232,9 +232,11 @@ describe('renderTranscriptionBody', () => {
|
||||
});
|
||||
|
||||
it('processes longer displayNames first to avoid prefix shadowing', () => {
|
||||
const augusteShort: PersonMention = { personId: 'p-short', displayName: 'Auguste' };
|
||||
const SHORT_ID = '11111111-1111-4111-8111-111111111111';
|
||||
const LONG_ID = '22222222-2222-4222-8222-222222222222';
|
||||
const augusteShort: PersonMention = { personId: SHORT_ID, displayName: 'Auguste' };
|
||||
const augusteLong: PersonMention = {
|
||||
personId: 'p-long',
|
||||
personId: LONG_ID,
|
||||
displayName: 'Auguste Raddatz'
|
||||
};
|
||||
// Sidecar order is short-first; longer match must still win for the long text
|
||||
@@ -242,8 +244,8 @@ describe('renderTranscriptionBody', () => {
|
||||
augusteShort,
|
||||
augusteLong
|
||||
]);
|
||||
expect(result).toContain('href="/persons/p-long"');
|
||||
expect(result).toContain('href="/persons/p-short"');
|
||||
expect(result).toContain(`href="/persons/${LONG_ID}"`);
|
||||
expect(result).toContain(`href="/persons/${SHORT_ID}"`);
|
||||
// The "Raddatz" suffix must not leak inside the short-name anchor
|
||||
expect(result).not.toMatch(/>Auguste<\/a> Raddatz/);
|
||||
});
|
||||
@@ -257,18 +259,20 @@ describe('renderTranscriptionBody', () => {
|
||||
|
||||
it('first-sidecar-wins when two entries share the same displayName', () => {
|
||||
// Two persons named "Hans" — first sidecar entry wins for all occurrences.
|
||||
const hansFirst: PersonMention = { personId: 'p-first', displayName: 'Hans' };
|
||||
const hansSecond: PersonMention = { personId: 'p-second', displayName: 'Hans' };
|
||||
const FIRST_ID = '33333333-3333-4333-8333-333333333333';
|
||||
const SECOND_ID = '44444444-4444-4444-8444-444444444444';
|
||||
const hansFirst: PersonMention = { personId: FIRST_ID, displayName: 'Hans' };
|
||||
const hansSecond: PersonMention = { personId: SECOND_ID, displayName: 'Hans' };
|
||||
const result = renderTranscriptionBody('@Hans und @Hans', [hansFirst, hansSecond]);
|
||||
expect(result).toContain('href="/persons/p-first"');
|
||||
expect(result).not.toContain('href="/persons/p-second"');
|
||||
expect(result).toContain(`href="/persons/${FIRST_ID}"`);
|
||||
expect(result).not.toContain(`href="/persons/${SECOND_ID}"`);
|
||||
const anchorCount = (result.match(/<a /g) ?? []).length;
|
||||
expect(anchorCount).toBe(2);
|
||||
});
|
||||
|
||||
it('escapes HTML in displayName to prevent stored XSS', () => {
|
||||
const xss: PersonMention = {
|
||||
personId: 'p-xss',
|
||||
personId: '55555555-5555-4555-8555-555555555555',
|
||||
displayName: '<script>alert(1)</script>'
|
||||
};
|
||||
const result = renderTranscriptionBody('Hi @<script>alert(1)</script> there', [xss]);
|
||||
@@ -292,7 +296,7 @@ describe('renderTranscriptionBody', () => {
|
||||
|
||||
it('escapes quotes in displayName so they cannot break the href attribute', () => {
|
||||
const tricky: PersonMention = {
|
||||
personId: 'p-quote',
|
||||
personId: '66666666-6666-4666-8666-666666666666',
|
||||
displayName: 'O"Brien'
|
||||
};
|
||||
const result = renderTranscriptionBody('@O"Brien', [tricky]);
|
||||
@@ -307,4 +311,40 @@ describe('renderTranscriptionBody', () => {
|
||||
const result = renderTranscriptionBody('Plain old transcription text.', []);
|
||||
expect(result).toBe('Plain old transcription text.');
|
||||
});
|
||||
|
||||
it('skips substitution when personId is not a UUID (defense in depth)', () => {
|
||||
// Nora #5551: if personId ever flowed in from a less-sanitised source
|
||||
// (a future "external person" or a bad sidecar), the renderer must not
|
||||
// emit a clickable link. The escaped text remains as plain content.
|
||||
const evil: PersonMention = {
|
||||
personId: 'javascript:alert(1)',
|
||||
displayName: 'Evil Link'
|
||||
};
|
||||
const result = renderTranscriptionBody('Hi @Evil Link!', [evil]);
|
||||
expect(result).not.toContain('<a ');
|
||||
expect(result).not.toContain('javascript:');
|
||||
// The @-trigger and displayName are preserved as plain text
|
||||
expect(result).toContain('@Evil Link');
|
||||
});
|
||||
|
||||
it('skips substitution when personId is an absolute URL', () => {
|
||||
const evil: PersonMention = {
|
||||
personId: 'https://evil.example/persons/abc',
|
||||
displayName: 'Phisher'
|
||||
};
|
||||
const result = renderTranscriptionBody('Hi @Phisher', [evil]);
|
||||
expect(result).not.toContain('<a ');
|
||||
expect(result).not.toContain('https://evil.example');
|
||||
});
|
||||
|
||||
it('still substitutes when personId is a well-formed UUID', () => {
|
||||
// Sanity check that the validation does not over-reject valid IDs.
|
||||
const valid: PersonMention = {
|
||||
personId: '550e8400-e29b-41d4-a716-446655440000',
|
||||
displayName: 'Auguste Raddatz'
|
||||
};
|
||||
const result = renderTranscriptionBody('Brief an @Auguste Raddatz', [valid]);
|
||||
expect(result).toContain('<a ');
|
||||
expect(result).toContain('href="/persons/550e8400-e29b-41d4-a716-446655440000"');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -77,6 +77,18 @@ function escapeRegExp(str: string): string {
|
||||
return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
|
||||
}
|
||||
|
||||
/**
|
||||
* Strict UUID v1–v5 check. Used as a defensive boundary on PersonMention.personId
|
||||
* before substituting it into an `href` — even though the backend currently only
|
||||
* emits UUIDs, a future "external person" feature must not accidentally turn this
|
||||
* helper into an open-redirect surface (CWE-601).
|
||||
*/
|
||||
const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
|
||||
|
||||
function isUuid(value: string): boolean {
|
||||
return UUID_RE.test(value);
|
||||
}
|
||||
|
||||
/**
|
||||
* Renders a transcription block's text segment as safe HTML for read mode.
|
||||
*
|
||||
@@ -100,6 +112,10 @@ export function renderTranscriptionBody(text: string, mentionedPersons: PersonMe
|
||||
const unique: PersonMention[] = [];
|
||||
for (const mention of mentionedPersons) {
|
||||
if (seen.has(mention.displayName)) continue;
|
||||
// Defense in depth: refuse to render an anchor for a non-UUID personId.
|
||||
// The escaped block text falls through unchanged, so the @-trigger is
|
||||
// preserved as plain content — no silent data loss, no clickable link.
|
||||
if (!isUuid(mention.personId)) continue;
|
||||
seen.add(mention.displayName);
|
||||
unique.push(mention);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user