Skip to content

Commit fcd1f50

Browse files
Moses Ingersollclaude
authored and
Moses Ingersoll
committed
address PR review: security fixes, feature flag, schema cleanup, tests
Security (critical): - Add action whitelist in webauthn.cgi to prevent open proxy - Skip reserved keys (action, nt_user_session) in param merge - Sanitize session cookie via CGI::cookie() to prevent header injection - TOCTOU-safe challenge consumption (atomic UPDATE, check affected rows) Security (important): - Send userHandle in JS authentication, validate on server - Sanitize error messages (warn real error, return generic message) - Fix CSRF token escaping in user.cgi (js_escape for JS context) - Use Authen::WebAuthn at compile time, not runtime require Feature flag: - Add webauthn_enabled option (default-on, set to 0 to disable) - All 7 public WebAuthn methods check _is_enabled() first - Returns error 600 "WebAuthn is disabled by administrator" when off Schema/migration: - Remove dead nt_user_recovery_code table and recovery_login ENUM - Fix UNIQUE KEY prefix lengths to full-column (was credential_id(191)) - Use CREATE TABLE IF NOT EXISTS for partial-failure robustness - Check both tables in _sql_test_2_44 Tests: - Add T1b (feature flag toggle), T10 (registration happy path), T11 (authentication happy path), T12 (cross-user isolation) - E2E: deduplicate b64url helpers, replace waitForTimeout with proper waitForSelector/waitForFunction, fix W6 test title Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent ed3b86c commit fcd1f50

9 files changed

Lines changed: 415 additions & 158 deletions

File tree

‎client/htdocs/nt-webauthn.js‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -204,25 +204,31 @@
204204
var response = assertion.response;
205205

206206
// Step 3: Send assertion to server
207+
var verifyPayload = {
208+
challenge_b64: bytesToBase64url(
209+
options.challenge
210+
),
211+
credential_id_b64: bytesToBase64url(
212+
assertion.rawId
213+
),
214+
client_data_json_b64: bytesToBase64url(
215+
response.clientDataJSON
216+
),
217+
authenticator_data_b64: bytesToBase64url(
218+
response.authenticatorData
219+
),
220+
signature_b64: bytesToBase64url(
221+
response.signature
222+
)
223+
};
224+
if (response.userHandle &&
225+
response.userHandle.byteLength > 0) {
226+
verifyPayload.user_handle_b64 =
227+
bytesToBase64url(response.userHandle);
228+
}
207229
webauthnPost(
208230
'webauthn_verify_auth',
209-
{
210-
challenge_b64: bytesToBase64url(
211-
options.challenge
212-
),
213-
credential_id_b64: bytesToBase64url(
214-
assertion.rawId
215-
),
216-
client_data_json_b64: bytesToBase64url(
217-
response.clientDataJSON
218-
),
219-
authenticator_data_b64: bytesToBase64url(
220-
response.authenticatorData
221-
),
222-
signature_b64: bytesToBase64url(
223-
response.signature
224-
)
225-
},
231+
verifyPayload,
226232
csrfToken
227233
)
228234
.done(function (verifyResp) {

‎client/htdocs/user.cgi‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ sub display_passkeys {
314314
document.getElementById('nt-add-passkey').style.display = '';
315315
316316
function loadPasskeys() {
317-
NtWebAuthn.listCredentials('] . $nt_obj->esc($csrf_token) . qq[', ] . int($uid) . qq[)
317+
NtWebAuthn.listCredentials('] . $nt_obj->js_escape($csrf_token) . qq[', ] . int($uid) . qq[)
318318
.done(function(resp) {
319319
var creds = resp.credentials || [];
320320
if (creds.length === 0) {
@@ -357,7 +357,7 @@ sub display_passkeys {
357357
window.ntAddPasskey = function() {
358358
var name = prompt('Enter a name for this passkey:', 'My Passkey');
359359
if (!name) return;
360-
NtWebAuthn.register('] . $nt_obj->esc($csrf_token) . qq[', ] . int($uid) . qq[, name)
360+
NtWebAuthn.register('] . $nt_obj->js_escape($csrf_token) . qq[', ] . int($uid) . qq[, name)
361361
.done(function() { loadPasskeys(); })
362362
.fail(function(err) { alert('Passkey registration failed: ' + err); });
363363
};

‎client/htdocs/webauthn.cgi‎

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,22 @@ sub main {
5353
return;
5454
}
5555

56+
# Whitelist of allowed actions — reject anything else
57+
my %allowed = map { $_ => 1 } qw(
58+
webauthn_get_registration_options
59+
webauthn_verify_registration
60+
webauthn_get_user_credentials
61+
webauthn_revoke_credential
62+
webauthn_rename_credential
63+
webauthn_get_auth_options
64+
webauthn_verify_auth
65+
);
66+
67+
if ( !$allowed{$action} ) {
68+
send_json( 400, { error_code => 400, error_msg => 'Unknown action' } );
69+
return;
70+
}
71+
5672
# Pre-session actions (no session cookie required)
5773
my %no_session = map { $_ => 1 } qw(
5874
webauthn_get_auth_options
@@ -72,8 +88,9 @@ sub main {
7288
$params{nt_user_session} = $cookie;
7389
}
7490

75-
# Merge request data into params
91+
# Merge request data into params, skipping reserved keys
7692
for my $key ( keys %$data ) {
93+
next if $key eq 'action' || $key eq 'nt_user_session';
7794
$params{$key} = $data->{$key};
7895
}
7996

@@ -89,16 +106,33 @@ sub main {
89106
&& $response->{nt_user_session} )
90107
{
91108
my $session = $response->{nt_user_session};
92-
my $secure = ( $ENV{HTTPS} || '' ) eq 'on' ? '; Secure' : '';
93-
print "Set-Cookie: NicTool=$session; Path=/; HttpOnly; SameSite=Strict$secure\n";
109+
$session =~ s/[^\x20-\x7E]//g; # strip control chars
110+
my $secure = ( $ENV{HTTPS} || '' ) eq 'on' ? 1 : 0;
111+
print $q->header(
112+
-type => 'application/json',
113+
-charset => 'utf-8',
114+
-cookie => CGI::cookie(
115+
-name => 'NicTool',
116+
-value => $session,
117+
-path => '/',
118+
-httponly => 1,
119+
-secure => $secure,
120+
-samesite => 'Strict',
121+
),
122+
);
123+
print encode_json($response);
124+
return;
94125
}
95126

96127
send_json( 200, $response );
97128
}
98129

99130
sub send_json {
100131
my ( $status, $data ) = @_;
101-
print "Content-Type: application/json\r\n\r\n";
132+
print CGI->new->header(
133+
-type => 'application/json',
134+
-status => $status,
135+
);
102136
print encode_json($data);
103137
}
104138

‎client/t/e2e/webauthn.spec.ts‎

Lines changed: 65 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,52 @@ import {
66
browserLogin, setupVirtualAuthenticator, teardownVirtualAuthenticator,
77
} from './helpers';
88

9+
// Base64url helpers injected into browser context via page.addScriptTag.
10+
// page.evaluate callbacks run in an isolated browser scope and cannot import
11+
// Node modules, so we inject these once per page instead of duplicating them
12+
// inside every evaluate call.
13+
const B64U_HELPERS = `
14+
function b64u2buf(s) {
15+
var b = s.replace(/-/g, '+').replace(/_/g, '/');
16+
while (b.length % 4) b += '=';
17+
var r = atob(b);
18+
var a = new Uint8Array(r.length);
19+
for (var i = 0; i < r.length; i++) a[i] = r.charCodeAt(i);
20+
return a.buffer;
21+
}
22+
function buf2b64u(b) {
23+
var u = new Uint8Array(b);
24+
var s = '';
25+
for (var i = 0; i < u.length; i++) s += String.fromCharCode(u[i]);
26+
return btoa(s).replace(/\\+/g, '-').replace(/\\//g, '_').replace(/=+$/, '');
27+
}
28+
`;
29+
30+
/** Inject b64u helpers into every frame of the page. */
31+
async function injectB64UHelpers(page: import('@playwright/test').Page) {
32+
await page.addScriptTag({ content: B64U_HELPERS });
33+
for (const frame of page.frames()) {
34+
try { await frame.addScriptTag({ content: B64U_HELPERS }); }
35+
catch { /* frame may not accept scripts */ }
36+
}
37+
}
38+
39+
/** Wait for the NicTool frameset to finish loading after login. */
40+
async function waitForFrameset(page: import('@playwright/test').Page) {
41+
await page.waitForFunction(
42+
() => window.frames.length > 0,
43+
{ timeout: 10000 },
44+
);
45+
}
46+
47+
/** Wait for the login page to be ready after navigation. */
48+
async function waitForLoginPage(page: import('@playwright/test').Page) {
49+
await page.waitForSelector(
50+
'input[name="username"]',
51+
{ timeout: 10000 },
52+
);
53+
}
54+
955
// -------------------------------------------------------------------------
1056
// W1: CSRF protection on webauthn.cgi
1157
// -------------------------------------------------------------------------
@@ -128,30 +174,16 @@ test.describe('W4-W7: WebAuthn ceremonies', () => {
128174
const { cdpSession, authenticatorId } = await setupVirtualAuthenticator(page);
129175
try {
130176
await browserLogin(page);
131-
await page.waitForTimeout(2000);
177+
await waitForFrameset(page);
132178

133179
const bodyFrame = page.frames().find(f =>
134180
f.url().includes('group.cgi'));
135181
expect(bodyFrame).toBeTruthy();
136182

183+
await injectB64UHelpers(page);
184+
137185
// Run registration ceremony inside the frame's browser context
138186
const result = await bodyFrame!.evaluate(async (baseUrl: string) => {
139-
function b64u2buf(s: string): ArrayBuffer {
140-
let b = s.replace(/-/g, '+').replace(/_/g, '/');
141-
while (b.length % 4) b += '=';
142-
const r = atob(b);
143-
const a = new Uint8Array(r.length);
144-
for (let i = 0; i < r.length; i++) a[i] = r.charCodeAt(i);
145-
return a.buffer;
146-
}
147-
function buf2b64u(b: ArrayBuffer): string {
148-
const u = new Uint8Array(b);
149-
let s = '';
150-
for (let i = 0; i < u.length; i++)
151-
s += String.fromCharCode(u[i]);
152-
return btoa(s).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '');
153-
}
154-
155187
const csrf = document.cookie.match(/NicTool_csrf=([^;]+)/)?.[1] || '';
156188

157189
// Step 1: get options
@@ -234,28 +266,15 @@ test.describe('W4-W7: WebAuthn ceremonies', () => {
234266
try {
235267
// --- Phase 1: Register a passkey while logged in ---
236268
await browserLogin(page);
237-
await page.waitForTimeout(2000);
269+
await waitForFrameset(page);
238270

239271
const bodyFrame = page.frames().find(f =>
240272
f.url().includes('group.cgi'));
241273
expect(bodyFrame).toBeTruthy();
242274

275+
await injectB64UHelpers(page);
276+
243277
const regResult = await bodyFrame!.evaluate(async (baseUrl: string) => {
244-
function b64u2buf(s: string): ArrayBuffer {
245-
let b = s.replace(/-/g, '+').replace(/_/g, '/');
246-
while (b.length % 4) b += '=';
247-
const r = atob(b);
248-
const a = new Uint8Array(r.length);
249-
for (let i = 0; i < r.length; i++) a[i] = r.charCodeAt(i);
250-
return a.buffer;
251-
}
252-
function buf2b64u(b: ArrayBuffer): string {
253-
const u = new Uint8Array(b);
254-
let s = '';
255-
for (let i = 0; i < u.length; i++)
256-
s += String.fromCharCode(u[i]);
257-
return btoa(s).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '');
258-
}
259278
const csrf = document.cookie.match(/NicTool_csrf=([^;]+)/)?.[1] || '';
260279
const optRes = await fetch(`${baseUrl}/webauthn.cgi`, {
261280
method: 'POST',
@@ -317,28 +336,15 @@ test.describe('W4-W7: WebAuthn ceremonies', () => {
317336
await page.goto(`${BASE}/index.cgi?logout=1`, {
318337
headers: { Cookie: `NicTool=${sessionCk}; NicTool_csrf=${csrfCk}` },
319338
});
320-
await page.waitForTimeout(1000);
339+
await waitForLoginPage(page);
321340

322341
// --- Phase 3: Passkey login without username ---
323342
await page.goto(`${BASE}/index.cgi`);
324-
await page.waitForTimeout(500);
343+
await waitForLoginPage(page);
344+
345+
await injectB64UHelpers(page);
325346

326347
const loginResult = await page.evaluate(async (baseUrl: string) => {
327-
function b64u2buf(s: string): ArrayBuffer {
328-
let b = s.replace(/-/g, '+').replace(/_/g, '/');
329-
while (b.length % 4) b += '=';
330-
const r = atob(b);
331-
const a = new Uint8Array(r.length);
332-
for (let i = 0; i < r.length; i++) a[i] = r.charCodeAt(i);
333-
return a.buffer;
334-
}
335-
function buf2b64u(b: ArrayBuffer): string {
336-
const u = new Uint8Array(b);
337-
let s = '';
338-
for (let i = 0; i < u.length; i++)
339-
s += String.fromCharCode(u[i]);
340-
return btoa(s).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '');
341-
}
342348
const csrf = document.cookie.match(/NicTool_csrf=([^;]+)/)?.[1]
343349
|| document.querySelector<HTMLInputElement>('input[name="csrf_token"]')?.value
344350
|| '';
@@ -413,7 +419,7 @@ test.describe('W4-W7: WebAuthn ceremonies', () => {
413419
}
414420
});
415421

416-
test('W6: credential list and rename', async ({ playwright }) => {
422+
test('W6: credential list', async ({ playwright }) => {
417423
test.skip(!webauthnConfigured, 'WebAuthn not configured on server');
418424

419425
const { sessionCookie, csrfCookie } = await apiLogin(playwright);
@@ -431,27 +437,14 @@ test.describe('W4-W7: WebAuthn ceremonies', () => {
431437
try {
432438
// Register a passkey
433439
await browserLogin(page);
434-
await page.waitForTimeout(2000);
440+
await waitForFrameset(page);
435441
const bodyFrame = page.frames().find(f =>
436442
f.url().includes('group.cgi'));
437443
expect(bodyFrame).toBeTruthy();
438444

445+
await injectB64UHelpers(page);
446+
439447
const regResult = await bodyFrame!.evaluate(async (baseUrl: string) => {
440-
function b64u2buf(s: string): ArrayBuffer {
441-
let b = s.replace(/-/g, '+').replace(/_/g, '/');
442-
while (b.length % 4) b += '=';
443-
const r = atob(b);
444-
const a = new Uint8Array(r.length);
445-
for (let i = 0; i < r.length; i++) a[i] = r.charCodeAt(i);
446-
return a.buffer;
447-
}
448-
function buf2b64u(b: ArrayBuffer): string {
449-
const u = new Uint8Array(b);
450-
let s = '';
451-
for (let i = 0; i < u.length; i++)
452-
s += String.fromCharCode(u[i]);
453-
return btoa(s).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '');
454-
}
455448
const csrf = document.cookie.match(/NicTool_csrf=([^;]+)/)?.[1] || '';
456449
const optRes = await fetch(`${baseUrl}/webauthn.cgi`, {
457450
method: 'POST',
@@ -509,29 +502,16 @@ test.describe('W4-W7: WebAuthn ceremonies', () => {
509502

510503
// Logout
511504
await page.goto(`${BASE}/index.cgi?logout=1`);
512-
await page.waitForTimeout(1000);
505+
await waitForLoginPage(page);
513506

514-
// Try passkey login — should fail because credential is revoked
507+
// Try passkey login -- should fail because credential is revoked
515508
// (The virtual authenticator still has the key, but server rejects it)
516509
await page.goto(`${BASE}/index.cgi`);
517-
await page.waitForTimeout(500);
510+
await waitForLoginPage(page);
511+
512+
await injectB64UHelpers(page);
518513

519514
const loginResult = await page.evaluate(async (baseUrl: string) => {
520-
function b64u2buf(s: string): ArrayBuffer {
521-
let b = s.replace(/-/g, '+').replace(/_/g, '/');
522-
while (b.length % 4) b += '=';
523-
const r = atob(b);
524-
const a = new Uint8Array(r.length);
525-
for (let i = 0; i < r.length; i++) a[i] = r.charCodeAt(i);
526-
return a.buffer;
527-
}
528-
function buf2b64u(b: ArrayBuffer): string {
529-
const u = new Uint8Array(b);
530-
let s = '';
531-
for (let i = 0; i < u.length; i++)
532-
s += String.fromCharCode(u[i]);
533-
return btoa(s).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '');
534-
}
535515
const csrf = document.cookie.match(/NicTool_csrf=([^;]+)/)?.[1]
536516
|| document.querySelector<HTMLInputElement>('input[name="csrf_token"]')?.value
537517
|| '';

0 commit comments

Comments
 (0)