Skip to content

sec(auth): close API-key persistence primitive on password rotation (#102) #392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 2 commits into from
Sep 28, 2026
Merged

sec(auth): close API-key persistence primitive on password rotation (#102) #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
495709e
sec(auth): close API-key persistence primitive on password rotation (…
cristim Sep 28, 2026
62984e0
fix(auth): address CodeRabbit findings on #392
cristim Sep 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions frontend/src/__tests__/api-apikeys.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,14 @@ describe('API Keys API Module', () => {
json: () => Promise.resolve(mockResponse)
});

const result = await createApiKey({ name: 'New Key' });
const request = { name: 'New Key', password: 'cGFzc3dvcmQ=', expires_at: '2025-12-31T00:00:00Z' };
const result = await createApiKey(request);

expect(fetchMock).toHaveBeenCalledWith(
'/api/api-keys',
expect.objectContaining({
method: 'POST',
body: JSON.stringify({ name: 'New Key' }),
body: JSON.stringify(request),
headers: expect.objectContaining({
'Content-Type': 'application/json',
'x-amz-content-sha256': expect.stringMatching(/^[a-f0-9]{64}$/)
Expand All @@ -140,6 +141,7 @@ describe('API Keys API Module', () => {

const request = {
name: 'Admin Key',
password: 'cGFzc3dvcmQ=',
permissions: [{ action: 'read', resource: '*' }],
expires_at: '2025-12-31T00:00:00Z'
};
Expand All @@ -163,7 +165,7 @@ describe('API Keys API Module', () => {
json: () => Promise.resolve({ error: 'Invalid request' })
});

await expect(createApiKey({ name: '' })).rejects.toThrow('Invalid request');
await expect(createApiKey({ name: '', password: '', expires_at: '2025-12-31T00:00:00Z' })).rejects.toThrow('Invalid request');
});
});

Expand Down
167 changes: 77 additions & 90 deletions frontend/src/__tests__/apikeys.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ jest.mock('../api', () => ({
getApiKeysUsageStats: jest.fn(),
createApiKey: jest.fn(),
revokeApiKey: jest.fn(),
deleteApiKey: jest.fn()
deleteApiKey: jest.fn(),
// Real implementation (not mocked): createApiKey encodes the password
// field with this before building the request, so tests need the real
// encoding to assert on the resulting request shape.
base64Encode: (str: string) => Buffer.from(str, 'binary').toString('base64')
}));

// confirmDialog (introduced in P2) is used for revoke/delete in place of
Expand All @@ -43,10 +47,8 @@ describe('API Keys Module', () => {
<div id="create-apikey-modal" class="hidden">
<form id="create-apikey-form">
<input type="text" id="apikey-name" value="">
<input type="checkbox" id="apikey-expires">
<div id="apikey-expires-at-field" class="hidden">
<input type="date" id="apikey-expires-at" value="">
</div>
<input type="password" id="apikey-password" value="">
<input type="date" id="apikey-expires-at" value="">
<div id="create-apikey-error" class="hidden"></div>
<button type="submit">Create</button>
</form>
Expand Down Expand Up @@ -376,17 +378,24 @@ describe('API Keys Module', () => {
expect(errorEl?.classList.contains('hidden')).toBe(true);
});

test('resets expiration checkbox and field', () => {
const expiresCheckbox = document.getElementById('apikey-expires') as HTMLInputElement;
const expiresAtField = document.getElementById('apikey-expires-at-field');

expiresCheckbox.checked = true;
expiresAtField?.classList.remove('hidden');
test('prefills expiration date 90 days out', () => {
const expiresAtInput = document.getElementById('apikey-expires-at') as HTMLInputElement;
expiresAtInput.value = '';

showCreateKeyModal();

expect(expiresCheckbox.checked).toBe(false);
expect(expiresAtField?.classList.contains('hidden')).toBe(true);
// Compare local calendar components, not a UTC round-trip: the field
// is filled from local date parts (issue #102 CodeRabbit finding), so
// asserting via toISOString would flake near local midnight in
// timezones ahead of or behind UTC.
const expected = new Date();
expected.setDate(expected.getDate() + 90);
const expectedValue = [
expected.getFullYear(),
String(expected.getMonth() + 1).padStart(2, '0'),
String(expected.getDate()).padStart(2, '0'),
].join('-');
expect(expiresAtInput.value).toBe(expectedValue);
});

test('handles missing modal gracefully', () => {
Expand Down Expand Up @@ -416,22 +425,9 @@ describe('API Keys Module', () => {
});

describe('createApiKey', () => {
test('creates API key with name only', async () => {
const mockResponse = {
api_key: 'full-api-key-value',
key_id: 'key-1',
key: { id: 'key-1', name: 'Test Key', key_prefix: 'abc' }
};

(api.createApiKey as jest.Mock).mockResolvedValue(mockResponse);

const result = await createApiKey('Test Key');
const expiresAt = new Date('2025-12-31T00:00:00Z');

expect(api.createApiKey).toHaveBeenCalledWith({ name: 'Test Key' });
expect(result.api_key).toBe('full-api-key-value');
});

test('creates API key with permissions', async () => {
test('creates API key with name, password and expiration', async () => {
const mockResponse = {
api_key: 'full-api-key-value',
key_id: 'key-1',
Expand All @@ -440,16 +436,17 @@ describe('API Keys Module', () => {

(api.createApiKey as jest.Mock).mockResolvedValue(mockResponse);

const permissions = [{ action: 'read', resource: '*' }];
await createApiKey('Test Key', permissions);
const result = await createApiKey('Test Key', 'my-password', expiresAt);

expect(api.createApiKey).toHaveBeenCalledWith({
name: 'Test Key',
permissions: permissions
password: Buffer.from('my-password', 'binary').toString('base64'),
expires_at: expiresAt.toISOString()
});
expect(result.api_key).toBe('full-api-key-value');
});

test('creates API key with expiration', async () => {
test('creates API key with permissions', async () => {
const mockResponse = {
api_key: 'full-api-key-value',
key_id: 'key-1',
Expand All @@ -458,20 +455,22 @@ describe('API Keys Module', () => {

(api.createApiKey as jest.Mock).mockResolvedValue(mockResponse);

const expiresAt = new Date('2025-12-31T00:00:00Z');
await createApiKey('Test Key', undefined, expiresAt);
const permissions = [{ action: 'read', resource: '*' }];
await createApiKey('Test Key', 'my-password', expiresAt, permissions);

expect(api.createApiKey).toHaveBeenCalledWith({
name: 'Test Key',
expires_at: expiresAt.toISOString()
password: Buffer.from('my-password', 'binary').toString('base64'),
expires_at: expiresAt.toISOString(),
permissions: permissions
});
});

test('throws error on API failure', async () => {
const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {});
(api.createApiKey as jest.Mock).mockRejectedValue(new Error('Create failed'));

await expect(createApiKey('Test Key')).rejects.toThrow('Create failed');
await expect(createApiKey('Test Key', 'my-password', expiresAt)).rejects.toThrow('Create failed');
expect(consoleError).toHaveBeenCalled();
consoleError.mockRestore();
});
Expand Down Expand Up @@ -503,46 +502,54 @@ describe('API Keys Module', () => {
expect(errorEl?.textContent).toBe('API key name is required');
});

test('shows error when expiration date is in the past', async () => {
const pastDate = new Date(Date.now() - 24 * 60 * 60 * 1000);

// Regression coverage for issue #102: the form must not let a key be
// created with no password re-verification.
test('shows error when password is empty', async () => {
const futureDate = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000);
(document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key';
(document.getElementById('apikey-expires') as HTMLInputElement).checked = true;
(document.getElementById('apikey-expires-at') as HTMLInputElement).value = pastDate.toISOString().split('T')[0] || "";
(document.getElementById('apikey-expires-at') as HTMLInputElement).value = futureDate.toISOString().split('T')[0] || "";

const event = { preventDefault: jest.fn() } as unknown as Event;
await handleCreateApiKey(event);

const errorEl = document.getElementById('create-apikey-error');
expect(errorEl?.textContent).toBe('Expiration date must be in the future');
expect(errorEl?.textContent).toBe('Your password is required to create an API key');
expect(api.createApiKey).not.toHaveBeenCalled();
});

test('creates key and shows success modal', async () => {
const mockResponse = {
api_key: 'new-api-key-12345',
key_id: 'key-1',
key: { id: 'key-1', name: 'Test Key', key_prefix: 'new' }
};
// Regression coverage for issue #102: the form must not let a key be
// created with no expiration.
test('shows error when expiration date is empty', async () => {
(document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key';
(document.getElementById('apikey-password') as HTMLInputElement).value = 'my-password';
(document.getElementById('apikey-expires-at') as HTMLInputElement).value = '';

(api.createApiKey as jest.Mock).mockResolvedValue(mockResponse);
(api.getApiKeys as jest.Mock).mockResolvedValue({ api_keys: [] });
const event = { preventDefault: jest.fn() } as unknown as Event;
await handleCreateApiKey(event);

const errorEl = document.getElementById('create-apikey-error');
expect(errorEl?.textContent).toBe('An expiration date is required');
expect(api.createApiKey).not.toHaveBeenCalled();
});

test('shows error when expiration date is in the past', async () => {
const pastDate = new Date(Date.now() - 24 * 60 * 60 * 1000);

(document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key';
(document.getElementById('apikey-password') as HTMLInputElement).value = 'my-password';
(document.getElementById('apikey-expires-at') as HTMLInputElement).value = pastDate.toISOString().split('T')[0] || "";

const event = { preventDefault: jest.fn() } as unknown as Event;
await handleCreateApiKey(event);

// Check that the key created modal is shown
const createdModal = document.getElementById('apikey-created-modal');
expect(createdModal).not.toBeNull();
expect(createdModal?.innerHTML).toContain('new-api-key-12345');
const errorEl = document.getElementById('create-apikey-error');
expect(errorEl?.textContent).toBe('Expiration date must be in the future');
});

test('creates key with expiration date', async () => {
test('creates key and shows success modal', async () => {
const futureDate = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000);

const mockResponse = {
api_key: 'new-api-key',
api_key: 'new-api-key-12345',
key_id: 'key-1',
key: { id: 'key-1', name: 'Test Key', key_prefix: 'new' }
};
Expand All @@ -551,7 +558,7 @@ describe('API Keys Module', () => {
(api.getApiKeys as jest.Mock).mockResolvedValue({ api_keys: [] });

(document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key';
(document.getElementById('apikey-expires') as HTMLInputElement).checked = true;
(document.getElementById('apikey-password') as HTMLInputElement).value = 'my-password';
(document.getElementById('apikey-expires-at') as HTMLInputElement).value = futureDate.toISOString().split('T')[0] || "";

const event = { preventDefault: jest.fn() } as unknown as Event;
Expand All @@ -560,15 +567,24 @@ describe('API Keys Module', () => {
expect(api.createApiKey).toHaveBeenCalledWith(
expect.objectContaining({
name: 'Test Key',
password: expect.any(String),
expires_at: expect.any(String)
})
);

// Check that the key created modal is shown
const createdModal = document.getElementById('apikey-created-modal');
expect(createdModal).not.toBeNull();
expect(createdModal?.innerHTML).toContain('new-api-key-12345');
});

test('shows error on API failure', async () => {
const futureDate = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000);
(api.createApiKey as jest.Mock).mockRejectedValue(new Error('Create failed'));

(document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key';
(document.getElementById('apikey-password') as HTMLInputElement).value = 'my-password';
(document.getElementById('apikey-expires-at') as HTMLInputElement).value = futureDate.toISOString().split('T')[0] || "";

const event = { preventDefault: jest.fn() } as unknown as Event;
await handleCreateApiKey(event);
Expand Down Expand Up @@ -819,6 +835,7 @@ describe('API Keys Module', () => {
});

test('sets up form submission handler', async () => {
const futureDate = new Date(Date.now() + 30 * 24 * 60 * 60 * 1000);
(api.createApiKey as jest.Mock).mockResolvedValue({
api_key: 'test-key',
key_id: 'key-1',
Expand All @@ -829,6 +846,8 @@ describe('API Keys Module', () => {
initApiKeys();

(document.getElementById('apikey-name') as HTMLInputElement).value = 'Test Key';
(document.getElementById('apikey-password') as HTMLInputElement).value = 'my-password';
(document.getElementById('apikey-expires-at') as HTMLInputElement).value = futureDate.toISOString().split('T')[0] || "";

const form = document.getElementById('create-apikey-form');
form?.dispatchEvent(new Event('submit'));
Expand All @@ -838,38 +857,6 @@ describe('API Keys Module', () => {
expect(api.createApiKey).toHaveBeenCalled();
});

test('sets up expires checkbox toggle', () => {
initApiKeys();

const expiresCheckbox = document.getElementById('apikey-expires') as HTMLInputElement;
const expiresAtField = document.getElementById('apikey-expires-at-field');
const expiresAtInput = document.getElementById('apikey-expires-at') as HTMLInputElement;

expiresCheckbox.checked = true;
expiresCheckbox.dispatchEvent(new Event('change'));

expect(expiresAtField?.classList.contains('hidden')).toBe(false);
expect(expiresAtInput.required).toBe(true);
// Should set default date to 90 days from now
expect(expiresAtInput.value).not.toBe('');
});

test('expires checkbox toggle hides field when unchecked', () => {
initApiKeys();

const expiresCheckbox = document.getElementById('apikey-expires') as HTMLInputElement;
const expiresAtField = document.getElementById('apikey-expires-at-field');

// First check, then uncheck
expiresCheckbox.checked = true;
expiresCheckbox.dispatchEvent(new Event('change'));

expiresCheckbox.checked = false;
expiresCheckbox.dispatchEvent(new Event('change'));

expect(expiresAtField?.classList.contains('hidden')).toBe(true);
});

test('sets up modal backdrop click to close', () => {
initApiKeys();

Expand Down
14 changes: 8 additions & 6 deletions frontend/src/__tests__/html.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,15 +562,17 @@ describe('HTML Structure', () => {
expect(input?.hasAttribute('required')).toBe(true);
});

test('has expiration checkbox', () => {
const checkbox = document.getElementById('apikey-expires');
expect(checkbox).toBeTruthy();
test('has required password input (issue #102)', () => {
const input = document.getElementById('apikey-password') as HTMLInputElement | null;
expect(input).toBeTruthy();
expect(input?.type).toBe('password');
expect(input?.hasAttribute('required')).toBe(true);
});

test('has expiration date field (hidden by default)', () => {
const field = document.getElementById('apikey-expires-at-field');
test('has required expiration date field (issue #102)', () => {
const field = document.getElementById('apikey-expires-at') as HTMLInputElement | null;
expect(field).toBeTruthy();
expect(field?.classList.contains('hidden')).toBe(true);
expect(field?.hasAttribute('required')).toBe(true);
});
});

Expand Down
7 changes: 6 additions & 1 deletion frontend/src/api/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,8 +616,13 @@ export interface APIKeysUsageStats {

export interface CreateAPIKeyRequest {
name: string;
// Base64-encoded, same convention as login/change-password/MFA (issue
// #102: creation now re-verifies the caller's password).
password: string;
permissions?: Permission[];
expires_at?: string;
// Required and capped server-side (issue #102): a key can no longer be
// created with no expiration.
expires_at: string;
}

export interface CreateAPIKeyResponse {
Expand Down
Loading
Loading