Skip to content

Commit d7da69e

Browse files
committed
fix(security): address code & security review findings
- CSRF: SameSite=Strict session cookie + Origin check on unsafe methods - WebAuthn: require user verification on registration and authentication - WebAuthn: detect authenticator sign-count regressions (clone protection) - WebAuthn: Zod-validate authenticator response payloads before verifying - auth: tie login assertion to the resolved handle's account; persist the WebAuthn user id so resident-key resolution maps to the real account - CSP: drop style-src 'unsafe-inline'; remove third-party font dependency - resolver: own rate-limit budget, exact key-type match, safe reflection, correct reserved/forced-text ordering; mirror type filter on JSON route - web: distinct error/retry state for profile load failures
1 parent 66e944e commit d7da69e

9 files changed

Lines changed: 107 additions & 27 deletions

File tree

apps/server/src/db/repositories.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,7 @@ const stmts = {
6767
const now = (): string => new Date().toISOString();
6868

6969
export const users = {
70-
create(handle: string, displayName: string): UserRow {
71-
const id = randomUUID();
70+
create(handle: string, displayName: string, id: string = randomUUID()): UserRow {
7271
const ts = now();
7372
stmts.insertUser.run(id, handle, handle.toLowerCase(), displayName, ts, ts);
7473
return stmts.userById.get(id) as UserRow;

apps/server/src/lib/session.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,9 @@ function baseCookieOptions(maxAgeSeconds: number): CookieSerializeOptions {
1313
return {
1414
path: '/',
1515
httpOnly: true,
16-
sameSite: 'lax',
16+
// Strict: the cookie is only sent for same-site requests, so it can never
17+
// ride along on a cross-site request forgery.
18+
sameSite: 'strict',
1719
secure: config.isProduction,
1820
maxAge: maxAgeSeconds,
1921
};

apps/server/src/lib/webauthn.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ export async function buildRegistrationOptions(params: {
4242
excludeCredentials: params.existing.map((c) => ({ id: c.id, transports: transportsOf(c) })),
4343
authenticatorSelection: {
4444
residentKey: 'preferred',
45-
userVerification: 'preferred',
45+
userVerification: 'required',
4646
},
4747
});
4848
}
@@ -53,7 +53,7 @@ export async function checkRegistration(params: { response: RegistrationResponse
5353
expectedChallenge: params.challenge,
5454
expectedOrigin: origin,
5555
expectedRPID: rpID,
56-
requireUserVerification: false,
56+
requireUserVerification: true,
5757
});
5858
if (!verification.verified || !verification.registrationInfo) return null;
5959

@@ -69,7 +69,7 @@ export async function checkRegistration(params: { response: RegistrationResponse
6969
export async function buildAuthenticationOptions(credentialsForUser: CredentialRow[]) {
7070
return generateAuthenticationOptions({
7171
rpID,
72-
userVerification: 'preferred',
72+
userVerification: 'required',
7373
allowCredentials: credentialsForUser.map((c) => ({ id: c.id, transports: transportsOf(c) })),
7474
});
7575
}
@@ -84,7 +84,7 @@ export async function checkAuthentication(params: {
8484
expectedChallenge: params.challenge,
8585
expectedOrigin: origin,
8686
expectedRPID: rpID,
87-
requireUserVerification: false,
87+
requireUserVerification: true,
8888
credential: {
8989
id: params.credential.id,
9090
publicKey: new Uint8Array(Buffer.from(params.credential.public_key, 'base64')),

apps/server/src/plugins/security.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,13 @@ import { config } from '../config.js';
1212
*/
1313
export async function registerSecurity(app: FastifyInstance): Promise<void> {
1414
await app.register(helmet, {
15-
// The SPA needs inline styles from the bundler and same-origin XHR/WebAuthn.
15+
// The bundler emits a linked stylesheet (no inline <style>/style attrs), so
16+
// the style policy stays strict; everything is same-origin.
1617
contentSecurityPolicy: {
1718
directives: {
1819
defaultSrc: ["'self'"],
1920
scriptSrc: ["'self'"],
20-
styleSrc: ["'self'", "'unsafe-inline'"],
21+
styleSrc: ["'self'"],
2122
imgSrc: ["'self'", 'data:'],
2223
connectSrc: ["'self'"],
2324
fontSrc: ["'self'", 'data:'],
@@ -37,6 +38,19 @@ export async function registerSecurity(app: FastifyInstance): Promise<void> {
3738

3839
await app.register(cookie, { secret: config.sessionSecret });
3940

41+
// CSRF defense: browsers always attach an Origin header to cross-origin
42+
// state-changing requests. Reject any unsafe method whose Origin is present
43+
// and does not match our own. Non-browser clients (no Origin) are unaffected,
44+
// and the SameSite=Strict session cookie provides defence in depth.
45+
const UNSAFE = new Set(['POST', 'PUT', 'PATCH', 'DELETE']);
46+
app.addHook('onRequest', async (request, reply) => {
47+
if (!UNSAFE.has(request.method)) return;
48+
const origin = request.headers.origin;
49+
if (origin && origin !== config.publicOrigin) {
50+
await reply.code(403).send({ error: 'forbidden', message: 'Cross-origin request rejected.' });
51+
}
52+
});
53+
4054
await app.register(rateLimit, {
4155
global: true,
4256
max: 240,

apps/server/src/plugins/spa.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,10 +78,12 @@ export async function registerSpa(app: FastifyInstance): Promise<void> {
7878
const handle = rawParam.endsWith('.keys') ? rawParam.slice(0, -'.keys'.length) : rawParam;
7979
const forceText = rawParam.endsWith('.keys') || (request.query as { format?: string }).format === 'txt';
8080

81-
// Reserved single-segment paths belong to the SPA router.
82-
if (isReservedHandle(handle)) return sendApp(reply);
83-
8481
const valid = handleSchema.safeParse(handle);
82+
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);
86+
8587
const user = valid.success ? users.byHandle(valid.data) : undefined;
8688

8789
// Browsers (and unrecognised handles without forced text) get the SPA, which
@@ -90,19 +92,24 @@ export async function registerSpa(app: FastifyInstance): Promise<void> {
9092

9193
reply.type('text/plain; charset=utf-8');
9294
if (!user) {
93-
return reply.code(404).send(`# No SSHID found for "@${handle}"\n`);
95+
// Only reflect the handle when it is well-formed; otherwise stay generic.
96+
const safe = valid.success ? `"@${valid.data}"` : 'that handle';
97+
return reply.code(404).send(`# No SSHID found for ${safe}\n`);
9498
}
9599

100+
// Exact (case-insensitive) key-type match, validated against supported types.
96101
const typeFilter = (request.query as { type?: string }).type?.toLowerCase();
97102
const lines = sshKeys
98103
.byUser(user.id)
99-
.filter((k) => !typeFilter || k.key_type.toLowerCase().includes(typeFilter))
104+
.filter((k) => !typeFilter || k.key_type.toLowerCase() === typeFilter)
100105
.map((k) => (k.label ? `${k.public_key} ${k.label}` : k.public_key));
101106

102107
return reply.header('cache-control', 'public, max-age=60').send(renderAuthorizedKeys(user.handle, lines));
103108
};
104109

105-
app.get('/:handle', resolver);
110+
// Public read endpoint: give it its own generous limit so curl polling does
111+
// not consume the global per-IP budget shared with the API.
112+
app.get('/:handle', { config: { rateLimit: { max: 120, timeWindow: '1 minute' } } }, resolver);
106113

107114
// Deep SPA links (e.g. /dashboard after a hard refresh) — serve the app shell.
108115
app.setNotFoundHandler({ preHandler: app.rateLimit() }, async (request, reply) => {

apps/server/src/routes/auth.ts

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,22 @@ const registerStartSchema = z.object({
2525

2626
const loginStartSchema = z.object({ handle: handleSchema });
2727

28+
/**
29+
* Minimal shape guard for an authenticator response before it is handed to the
30+
* WebAuthn verifier. This rejects malformed/oversized payloads early instead of
31+
* casting arbitrary JSON straight into the library.
32+
*/
33+
const webauthnResponseSchema = z
34+
.object({
35+
id: z.string().min(1).max(1024),
36+
rawId: z.string().min(1).max(1024),
37+
type: z.literal('public-key'),
38+
response: z.record(z.unknown()),
39+
clientExtensionResults: z.record(z.unknown()).optional(),
40+
authenticatorAttachment: z.string().optional(),
41+
})
42+
.passthrough();
43+
2844
/**
2945
* Passkey (WebAuthn) registration and authentication.
3046
*
@@ -74,15 +90,22 @@ export async function authRoutes(app: FastifyInstance): Promise<void> {
7490
return reply.code(409).send({ error: 'handle_taken', message: 'That handle is already registered.' });
7591
}
7692

93+
const body = webauthnResponseSchema.safeParse(request.body);
94+
if (!body.success) {
95+
return reply.code(400).send({ error: 'invalid_request', message: 'Malformed registration response.' });
96+
}
97+
7798
const verified = await checkRegistration({
78-
response: request.body as RegistrationResponseJSON,
99+
response: body.data as unknown as RegistrationResponseJSON,
79100
challenge: challenge.challenge,
80101
});
81102
if (!verified) {
82103
return reply.code(400).send({ error: 'verification_failed', message: 'Passkey could not be verified.' });
83104
}
84105

85-
const user = users.create(challenge.handle, challenge.displayName ?? '');
106+
// 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);
86109
credentials.create({
87110
id: verified.id,
88111
userId: user.id,
@@ -117,17 +140,34 @@ export async function authRoutes(app: FastifyInstance): Promise<void> {
117140
return reply.code(400).send({ error: 'challenge_expired', message: 'Login session expired. Try again.' });
118141
}
119142

120-
const response = request.body as AuthenticationResponseJSON;
143+
const body = webauthnResponseSchema.safeParse(request.body);
144+
if (!body.success) {
145+
return reply.code(400).send({ error: 'invalid_request', message: 'Malformed authentication response.' });
146+
}
147+
const response = body.data as unknown as AuthenticationResponseJSON;
148+
121149
const credential = credentials.byId(response.id);
122150
if (!credential) {
123151
return reply.code(401).send({ error: 'unknown_credential', message: 'Passkey not recognised.' });
124152
}
125153

154+
// The credential must belong to the account the handle resolved to. This ties
155+
// the assertion to the handle the user actually typed at login/options.
156+
if (!challenge.userId || credential.user_id !== challenge.userId) {
157+
return reply.code(401).send({ error: 'verification_failed', message: 'Passkey could not be verified.' });
158+
}
159+
126160
const result = await checkAuthentication({ response, challenge: challenge.challenge, credential });
127161
if (!result) {
128162
return reply.code(401).send({ error: 'verification_failed', message: 'Passkey could not be verified.' });
129163
}
130164

165+
// Cloned-authenticator detection: a counter that fails to advance (when the
166+
// authenticator uses one at all) indicates a possible clone or replay.
167+
if (credential.counter > 0 && result.newCounter <= credential.counter) {
168+
return reply.code(401).send({ error: 'counter_regression', message: 'Passkey rejected (possible clone).' });
169+
}
170+
131171
credentials.touch(credential.id, result.newCounter);
132172
clearChallenge(reply);
133173
issueSession(reply, credential.user_id);

apps/server/src/routes/publicUser.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,16 @@ export async function publicUserRoutes(app: FastifyInstance): Promise<void> {
1919
return reply.code(404).send({ error: 'not_found', message: 'No such handle.' });
2020
}
2121

22+
// Optional exact key-type filter, mirroring the plain-text resolver.
23+
const typeFilter = (request.query as { type?: string }).type?.toLowerCase();
24+
2225
return {
2326
handle: user.handle,
2427
displayName: user.display_name,
25-
keys: sshKeys.byUser(user.id).map((k) => ({
28+
keys: sshKeys
29+
.byUser(user.id)
30+
.filter((k) => !typeFilter || k.key_type.toLowerCase() === typeFilter)
31+
.map((k) => ({
2632
label: k.label,
2733
type: k.key_type,
2834
fingerprint: k.fingerprint,

apps/web/index.html

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,6 @@
1717
content="Fetch all your SSH public keys with a single handle. Open-source and self-hostable."
1818
/>
1919
<meta property="og:type" content="website" />
20-
<link rel="preconnect" href="https://fonts.googleapis.com" />
21-
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
22-
<link
23-
href="https://fonts.googleapis.com/css2?family=Inter:wght@400;500;600;700;800&family=JetBrains+Mono:wght@400;500;600&display=swap"
24-
rel="stylesheet"
25-
/>
2620
</head>
2721
<body>
2822
<div id="root"></div>

apps/web/src/pages/Profile.tsx

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { NotFound } from './NotFound';
99
export function Profile() {
1010
const { handle = '' } = useParams();
1111
const [profile, setProfile] = useState<PublicProfile | null>(null);
12-
const [state, setState] = useState<'loading' | 'ready' | 'missing'>('loading');
12+
const [state, setState] = useState<'loading' | 'ready' | 'missing' | 'error'>('loading');
1313

1414
useEffect(() => {
1515
let active = true;
@@ -23,7 +23,9 @@ export function Profile() {
2323
})
2424
.catch((err) => {
2525
if (!active) return;
26-
setState(err instanceof ApiError && err.status === 404 ? 'missing' : 'missing');
26+
// A genuine 404 means the handle is unclaimed; anything else (5xx,
27+
// network) is a transient error the user can retry.
28+
setState(err instanceof ApiError && err.status === 404 ? 'missing' : 'error');
2729
});
2830
return () => {
2931
active = false;
@@ -38,6 +40,22 @@ export function Profile() {
3840
);
3941
}
4042

43+
if (state === 'error') {
44+
return (
45+
<div className="container-page grid min-h-[calc(100vh-4rem)] place-items-center py-16 text-center">
46+
<div className="animate-fade-up">
47+
<h1 className="text-2xl font-bold text-white">Couldn’t load this profile</h1>
48+
<p className="mx-auto mt-3 max-w-md text-slate-400">
49+
Something went wrong reaching the server. Please try again.
50+
</p>
51+
<button onClick={() => window.location.reload()} className="btn-primary mt-8">
52+
Retry
53+
</button>
54+
</div>
55+
</div>
56+
);
57+
}
58+
4159
if (state === 'missing' || !profile) {
4260
return <NotFound handle={handle} />;
4361
}

0 commit comments

Comments
 (0)