fix(security): keep OAuth tokens out of renderer IPC and compare API keys in constant time
The cloud ORPC routes (addGoogleAccount, listCloudAccounts, refreshAccountQuota, syncLocalAccount) returned the full account including access and refresh tokens, but the renderer never reads them. Blank the two secrets in those responses; token metadata (expiry, project id) is kept so the schema holds. Also compare the presented gateway API key with timingSafeEqual.
This commit is contained in:
1 parent
291b8ac91f
commit
1ad15d6e3a
7 files changed
+117
-7
No files matched your search
@@ -28,6 +28,7 @@ import { CloudAccountRepo } from '../database/cloudHandler';
|
||||
import { CloudAccountSchema } from '../../types/cloudAccount';
|
||||
import { DeviceProfileSchema, DeviceProfilesSnapshotSchema } from '../../types/account';
|
||||
import { logger } from '../../utils/logger';
|
||||
import { redactCloudAccountTokens } from '../../utils/cloud-account-redaction';
|
||||
import { getSwitchMetricsSnapshot } from '../switchMetrics';
|
||||
import { getSwitchGuardSnapshot } from '../switchGuard';
|
||||
import { getDeviceHardeningSnapshot } from '../device/handler';
|
||||
@@ -107,11 +108,11 @@ export const cloudRouter = os.router({
|
||||
.input(z.object({ authCode: z.string(), oauthClientKey: z.string().optional() }))
|
||||
.output(CloudAccountSchema)
|
||||
.handler(async ({ input }) => {
|
||||
return addGoogleAccount(input.authCode, input.oauthClientKey);
|
||||
return redactCloudAccountTokens(await addGoogleAccount(input.authCode, input.oauthClientKey));
|
||||
}),
|
||||
|
||||
listCloudAccounts: os.output(z.array(CloudAccountSchema)).handler(async () => {
|
||||
return listCloudAccounts();
|
||||
return (await listCloudAccounts()).map(redactCloudAccountTokens);
|
||||
}),
|
||||
|
||||
deleteCloudAccount: os
|
||||
@@ -125,7 +126,7 @@ export const cloudRouter = os.router({
|
||||
.input(z.object({ accountId: z.string() }))
|
||||
.output(CloudAccountSchema)
|
||||
.handler(async ({ input }) => {
|
||||
return refreshAccountQuota(input.accountId);
|
||||
return redactCloudAccountTokens(await refreshAccountQuota(input.accountId));
|
||||
}),
|
||||
|
||||
switchCloudAccount: os
|
||||
@@ -206,7 +207,7 @@ export const cloudRouter = os.router({
|
||||
try {
|
||||
const result = await CloudAccountRepo.syncFromIDE();
|
||||
|
||||
return result;
|
||||
return result ? redactCloudAccountTokens(result) : result;
|
||||
} catch (error: unknown) {
|
||||
logger.error(
|
||||
'[ORPC] syncLocalAccount error:',
|
||||
|
||||
@@ -1,6 +1,11 @@
|
||||
import { CanActivate, ExecutionContext, Injectable, UnauthorizedException } from '@nestjs/common';
|
||||
import { getServerConfig } from '../server-config';
|
||||
import { extractApiKeyToken, hasConfiguredApiKey, RequestHeaders } from './api-key-auth.util';
|
||||
import {
|
||||
apiKeyMatches,
|
||||
extractApiKeyToken,
|
||||
hasConfiguredApiKey,
|
||||
RequestHeaders,
|
||||
} from './api-key-auth.util';
|
||||
|
||||
@Injectable()
|
||||
export class AdminGuard implements CanActivate {
|
||||
@@ -16,7 +21,7 @@ export class AdminGuard implements CanActivate {
|
||||
const headers = request.headers as RequestHeaders;
|
||||
const clientToken = extractApiKeyToken(headers);
|
||||
|
||||
if (clientToken && clientToken === apiKey) {
|
||||
if (apiKeyMatches(clientToken, apiKey)) {
|
||||
return true;
|
||||
}
|
||||
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { createHash, timingSafeEqual } from 'crypto';
|
||||
import { isEmpty, isString } from 'lodash-es';
|
||||
|
||||
export type RequestHeaderValue = string | string[] | undefined;
|
||||
@@ -8,6 +9,20 @@ export function hasConfiguredApiKey(apiKey: string | undefined): apiKey is strin
|
||||
return isString(apiKey) && !isEmpty(apiKey.trim());
|
||||
}
|
||||
|
||||
/**
|
||||
* Compares the presented key with the configured one without leaking, through response time,
|
||||
* how many leading characters matched. Both values are hashed so the buffers always have the
|
||||
* same length, which timingSafeEqual requires.
|
||||
*/
|
||||
export function apiKeyMatches(clientToken: string | null, apiKey: string): boolean {
|
||||
if (clientToken === null) {
|
||||
return false;
|
||||
}
|
||||
const presented = createHash('sha256').update(clientToken).digest();
|
||||
const expected = createHash('sha256').update(apiKey).digest();
|
||||
return timingSafeEqual(presented, expected);
|
||||
}
|
||||
|
||||
export function extractApiKeyToken(headers: RequestHeaders): string | null {
|
||||
const authorizationHeader = readHeaderValue(headers['authorization']);
|
||||
if (authorizationHeader) {
|
||||
|
||||
@@ -7,6 +7,7 @@ import {
|
||||
} from '@nestjs/common';
|
||||
import { getServerConfig } from '../../server-config';
|
||||
import {
|
||||
apiKeyMatches,
|
||||
extractApiKeyToken,
|
||||
hasConfiguredApiKey,
|
||||
RequestHeaders,
|
||||
@@ -32,7 +33,7 @@ export class ProxyGuard implements CanActivate {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (clientToken === apiKey) {
|
||||
if (apiKeyMatches(clientToken, apiKey)) {
|
||||
return true;
|
||||
}
|
||||
this.logger.warn(`Rejected unauthorized request from ${request.ip}`);
|
||||
|
||||
@@ -0,0 +1,52 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { redactCloudAccountTokens } from '../../utils/cloud-account-redaction';
|
||||
import type { CloudAccount } from '../../types/cloudAccount';
|
||||
|
||||
function makeAccount(): CloudAccount {
|
||||
return {
|
||||
id: 'acc-1',
|
||||
provider: 'google',
|
||||
email: '[email protected]',
|
||||
name: 'User',
|
||||
token: {
|
||||
access_token: 'ya29.secret-access',
|
||||
refresh_token: '1//secret-refresh',
|
||||
expires_in: 3600,
|
||||
expiry_timestamp: 1_900_000_000,
|
||||
token_type: 'Bearer',
|
||||
project_id: 'project-1',
|
||||
is_gcp_tos: true,
|
||||
},
|
||||
created_at: 1,
|
||||
last_used: 2,
|
||||
};
|
||||
}
|
||||
|
||||
describe('redactCloudAccountTokens', () => {
|
||||
it('blanks the OAuth credentials and keeps the non-secret token metadata', () => {
|
||||
const redacted = redactCloudAccountTokens(makeAccount());
|
||||
expect(redacted.token.access_token).toBe('');
|
||||
expect(redacted.token.refresh_token).toBe('');
|
||||
expect(redacted.token).toMatchObject({
|
||||
expires_in: 3600,
|
||||
expiry_timestamp: 1_900_000_000,
|
||||
token_type: 'Bearer',
|
||||
project_id: 'project-1',
|
||||
is_gcp_tos: true,
|
||||
});
|
||||
expect(redacted.email).toBe('[email protected]');
|
||||
});
|
||||
|
||||
it('does not mutate the original account', () => {
|
||||
const account = makeAccount();
|
||||
redactCloudAccountTokens(account);
|
||||
expect(account.token.refresh_token).toBe('1//secret-refresh');
|
||||
expect(account.token.access_token).toBe('ya29.secret-access');
|
||||
});
|
||||
|
||||
it('never leaves a credential in the serialized payload', () => {
|
||||
const payload = JSON.stringify(redactCloudAccountTokens(makeAccount()));
|
||||
expect(payload).not.toContain('secret-access');
|
||||
expect(payload).not.toContain('secret-refresh');
|
||||
});
|
||||
});
|
||||
@@ -1,5 +1,6 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import {
|
||||
apiKeyMatches,
|
||||
hasConfiguredApiKey,
|
||||
extractApiKeyToken,
|
||||
type RequestHeaders,
|
||||
@@ -81,4 +82,19 @@ describe('api-key-auth.util', () => {
|
||||
expect(extractApiKeyToken(headers)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('apiKeyMatches', () => {
|
||||
it('accepts only the exact configured key', () => {
|
||||
expect(apiKeyMatches('secret-key', 'secret-key')).toBe(true);
|
||||
expect(apiKeyMatches('secret-key ', 'secret-key')).toBe(false);
|
||||
expect(apiKeyMatches('Secret-Key', 'secret-key')).toBe(false);
|
||||
});
|
||||
|
||||
it('rejects missing, empty and different-length tokens without throwing', () => {
|
||||
expect(apiKeyMatches(null, 'secret-key')).toBe(false);
|
||||
expect(apiKeyMatches('', 'secret-key')).toBe(false);
|
||||
expect(apiKeyMatches('secret', 'secret-key')).toBe(false);
|
||||
expect(apiKeyMatches('secret-key-and-more', 'secret-key')).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,20 @@
|
||||
import type { CloudAccount } from '../types/cloudAccount';
|
||||
|
||||
/**
|
||||
* Returns a copy of the account that is safe to hand to the renderer.
|
||||
*
|
||||
* The renderer never needs the OAuth credentials: they are only used by the main process
|
||||
* (quota refresh, IDE switch, proxy). Keeping them out of IPC payloads means an XSS or a
|
||||
* compromised renderer cannot read refresh tokens. The non-secret token metadata
|
||||
* (expiry, project id, tier flags) is preserved so the output schema stays valid.
|
||||
*/
|
||||
export function redactCloudAccountTokens<T extends CloudAccount>(account: T): T {
|
||||
return {
|
||||
...account,
|
||||
token: {
|
||||
...account.token,
|
||||
access_token: '',
|
||||
refresh_token: '',
|
||||
},
|
||||
};
|
||||
}
|
||||
Reference in new issue
Block a user