Skip to content

Commit e2d56fb

Browse files
committed
fix: second review pass + remove brand references
- reject control characters in key label and display name (prevents newline injection into the plain-text authorized_keys output) - map handle race on registration to 409 instead of 500 - explicit return after the cross-origin 403 in the CSRF hook - reserved single-segment paths only return the SPA shell to HTML clients - validate ?type filter against supported key types (shared isSupportedKeyType) - extract shared noControlChars validator - drop brand references from UI and docs
1 parent 82e8e1c commit e2d56fb

6 files changed

Lines changed: 62 additions & 11 deletions

File tree

apps/server/src/lib/validation.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/**
2+
* Shared input-validation helpers.
3+
*/
4+
5+
/**
6+
* True when the string contains no control characters (C0 range or DEL),
7+
* including CR/LF. Used to keep newline-bearing input out of user-facing fields
8+
* that are later rendered into line-oriented formats such as authorized_keys.
9+
*/
10+
export function noControlChars(value: string): boolean {
11+
for (const char of value) {
12+
const code = char.charCodeAt(0);
13+
if (code < 0x20 || code === 0x7f) return false;
14+
}
15+
return true;
16+
}

apps/server/src/plugins/security.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ export async function registerSecurity(app: FastifyInstance): Promise<void> {
4747
if (!UNSAFE.has(request.method)) return;
4848
const origin = request.headers.origin;
4949
if (origin && origin !== config.publicOrigin) {
50-
await reply.code(403).send({ error: 'forbidden', message: 'Cross-origin request rejected.' });
50+
return reply.code(403).send({ error: 'forbidden', message: 'Cross-origin request rejected.' });
5151
}
5252
});
5353

apps/server/src/plugins/spa.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { config } from '../config.js';
77
import { logger } from '../logger.js';
88
import { sshKeys, users } from '../db/repositories.js';
99
import { handleSchema, isReservedHandle } from '../lib/handle.js';
10+
import { isSupportedKeyType } from '../lib/sshkey.js';
1011

1112
const here = dirname(fileURLToPath(import.meta.url));
1213

@@ -80,9 +81,12 @@ export async function registerSpa(app: FastifyInstance): Promise<void> {
8081

8182
const valid = handleSchema.safeParse(handle);
8283

83-
// Reserved single-segment paths belong to the SPA router, unless plain text
84-
// was explicitly requested (in which case there is simply no such handle).
85-
if (isReservedHandle(handle) && !forceText) return sendApp(reply);
84+
// Reserved single-segment paths belong to the SPA router — but only hand back
85+
// the HTML shell to clients that actually asked for HTML. A non-browser
86+
// client hitting a reserved path falls through to a plain 404.
87+
if (isReservedHandle(handle)) {
88+
return clientWantsHtml(request) ? sendApp(reply) : reply.code(404).type('text/plain; charset=utf-8').send('# Not found\n');
89+
}
8690

8791
const user = valid.success ? users.byHandle(valid.data) : undefined;
8892

@@ -97,8 +101,10 @@ export async function registerSpa(app: FastifyInstance): Promise<void> {
97101
return reply.code(404).send(`# No SSHID found for ${safe}\n`);
98102
}
99103

100-
// Exact (case-insensitive) key-type match, validated against supported types.
101-
const typeFilter = (request.query as { type?: string }).type?.toLowerCase();
104+
// Exact (case-insensitive) key-type match. An unrecognised type is ignored
105+
// rather than silently returning nothing.
106+
const rawType = (request.query as { type?: string }).type?.toLowerCase();
107+
const typeFilter = rawType && isSupportedKeyType(rawType) ? rawType : undefined;
102108
const lines = sshKeys
103109
.byUser(user.id)
104110
.filter((k) => !typeFilter || k.key_type.toLowerCase() === typeFilter)

apps/server/src/routes/auth.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { z } from 'zod';
33
import type { AuthenticationResponseJSON, RegistrationResponseJSON } from '@simplewebauthn/server';
44
import { credentials, users } from '../db/repositories.js';
55
import { handleSchema } from '../lib/handle.js';
6+
import { noControlChars } from '../lib/validation.js';
67
import {
78
clearChallenge,
89
clearSession,
@@ -20,7 +21,13 @@ import { loadUser } from '../plugins/auth.js';
2021

2122
const registerStartSchema = z.object({
2223
handle: handleSchema,
23-
displayName: z.string().trim().max(80).optional().default(''),
24+
displayName: z
25+
.string()
26+
.trim()
27+
.max(80)
28+
.refine(noControlChars, 'Display name cannot contain control characters.')
29+
.optional()
30+
.default(''),
2431
});
2532

2633
const loginStartSchema = z.object({ handle: handleSchema });
@@ -104,8 +111,18 @@ export async function authRoutes(app: FastifyInstance): Promise<void> {
104111
}
105112

106113
// Reuse the id encoded into the credential's userHandle so resident-key
107-
// resolution maps back to this exact account.
108-
const user = users.create(challenge.handle, challenge.displayName ?? '', challenge.userId);
114+
// resolution maps back to this exact account. Guard against a handle claimed
115+
// in the race between the two ceremony steps (UNIQUE violation -> 409).
116+
let user;
117+
try {
118+
user = users.create(challenge.handle, challenge.displayName ?? '', challenge.userId);
119+
} catch (err) {
120+
if (err instanceof Error && /UNIQUE/i.test(err.message)) {
121+
clearChallenge(reply);
122+
return reply.code(409).send({ error: 'handle_taken', message: 'That handle is already registered.' });
123+
}
124+
throw err;
125+
}
109126
credentials.create({
110127
id: verified.id,
111128
userId: user.id,

apps/server/src/routes/keys.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,20 @@ import type { FastifyInstance } from 'fastify';
22
import { z } from 'zod';
33
import { sshKeys, type SshKeyRow } from '../db/repositories.js';
44
import { InvalidPublicKeyError, parsePublicKey } from '../lib/sshkey.js';
5+
import { noControlChars } from '../lib/validation.js';
56
import { requireAuth } from '../plugins/auth.js';
67

78
const addKeySchema = z.object({
8-
label: z.string().trim().max(80).optional().default(''),
9+
// The label is emitted verbatim as a comment in the plain-text
10+
// authorized_keys output, so an embedded newline could forge a second key
11+
// line. Disallow all control characters.
12+
label: z
13+
.string()
14+
.trim()
15+
.max(80)
16+
.refine(noControlChars, 'Label cannot contain control characters.')
17+
.optional()
18+
.default(''),
919
publicKey: z.string().min(1).max(16 * 1024),
1020
});
1121

apps/server/src/routes/publicUser.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { FastifyInstance } from 'fastify';
22
import { z } from 'zod';
33
import { sshKeys, users } from '../db/repositories.js';
44
import { handleSchema } from '../lib/handle.js';
5+
import { isSupportedKeyType } from '../lib/sshkey.js';
56

67
/**
78
* Public, unauthenticated profile data consumed by the SPA to render a user's
@@ -20,7 +21,8 @@ export async function publicUserRoutes(app: FastifyInstance): Promise<void> {
2021
}
2122

2223
// Optional exact key-type filter, mirroring the plain-text resolver.
23-
const typeFilter = (request.query as { type?: string }).type?.toLowerCase();
24+
const rawType = (request.query as { type?: string }).type?.toLowerCase();
25+
const typeFilter = rawType && isSupportedKeyType(rawType) ? rawType : undefined;
2426

2527
return {
2628
handle: user.handle,

0 commit comments

Comments
 (0)