Skip to content

Commit 0db704a

Browse files
zuharzzuharz
committed
fix(security): Remove inline token support from schema
- Remove string token support as it's considered a security footgun - V3 intentionally removed this feature from V2 - Tokens must now use secret or env references only Addresses: brendan-kellam's security concern
1 parent e7517a2 commit 0db704a

File tree

6 files changed

+257
-14
lines changed

6 files changed

+257
-14
lines changed

‎packages/crypto/src/tokenUtils.test.ts‎

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
import { describe, test, expect, vi, beforeEach } from 'vitest';
2+
import { PrismaClient } from '@sourcebot/db';
3+
import { getTokenFromConfig } from './tokenUtils';
4+
5+
// Mock the decrypt function
6+
vi.mock('./index.js', () => ({
7+
decrypt: vi.fn().mockReturnValue('decrypted-secret-value')
8+
}));
9+
10+
describe('tokenUtils', () => {
11+
let mockPrisma: any;
12+
const testOrgId = 1;
13+
14+
beforeEach(() => {
15+
mockPrisma = {
16+
secret: {
17+
findUnique: vi.fn(),
18+
},
19+
};
20+
21+
vi.clearAllMocks();
22+
delete process.env.TEST_TOKEN;
23+
delete process.env.EMPTY_TOKEN;
24+
});
25+
26+
describe('getTokenFromConfig', () => {
27+
test('handles secret-based tokens', async () => {
28+
const mockSecret = {
29+
iv: 'test-iv',
30+
encryptedValue: 'encrypted-value'
31+
};
32+
mockPrisma.secret.findUnique.mockResolvedValue(mockSecret);
33+
34+
const config = { secret: 'my-secret' };
35+
const result = await getTokenFromConfig(config, testOrgId, mockPrisma);
36+
37+
expect(result).toBe('decrypted-secret-value');
38+
expect(mockPrisma.secret.findUnique).toHaveBeenCalledWith({
39+
where: {
40+
orgId_key: {
41+
key: 'my-secret',
42+
orgId: testOrgId
43+
}
44+
}
45+
});
46+
});
47+
48+
test('handles environment variable tokens', async () => {
49+
process.env.TEST_TOKEN = 'env-token-value';
50+
51+
const config = { env: 'TEST_TOKEN' };
52+
const result = await getTokenFromConfig(config, testOrgId, mockPrisma);
53+
54+
expect(result).toBe('env-token-value');
55+
});
56+
57+
test('throws error for string tokens (security)', async () => {
58+
const config = 'direct-string-token';
59+
60+
await expect(getTokenFromConfig(config as any, testOrgId, mockPrisma))
61+
.rejects.toThrow('Invalid token configuration');
62+
});
63+
64+
test('throws error for malformed token objects', async () => {
65+
const config = { invalid: 'format' };
66+
67+
await expect(getTokenFromConfig(config as any, testOrgId, mockPrisma))
68+
.rejects.toThrow('Invalid token configuration');
69+
});
70+
71+
test('throws error for missing secret', async () => {
72+
mockPrisma.secret.findUnique.mockResolvedValue(null);
73+
74+
const config = { secret: 'non-existent-secret' };
75+
76+
await expect(getTokenFromConfig(config, testOrgId, mockPrisma))
77+
.rejects.toThrow('Secret with key non-existent-secret not found for org 1');
78+
});
79+
80+
test('throws error for missing environment variable', async () => {
81+
const config = { env: 'NON_EXISTENT_VAR' };
82+
83+
await expect(getTokenFromConfig(config, testOrgId, mockPrisma))
84+
.rejects.toThrow('Environment variable NON_EXISTENT_VAR not found.');
85+
});
86+
87+
test('handles empty environment variable', async () => {
88+
process.env.EMPTY_TOKEN = '';
89+
90+
const config = { env: 'EMPTY_TOKEN' };
91+
92+
await expect(getTokenFromConfig(config, testOrgId, mockPrisma))
93+
.rejects.toThrow('Environment variable EMPTY_TOKEN not found.');
94+
});
95+
});
96+
});

‎packages/crypto/src/tokenUtils.ts‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,8 @@ import { Token } from "@sourcebot/schemas/v3/shared.type";
33
import { decrypt } from "./index.js";
44

55
export const getTokenFromConfig = async (token: Token, orgId: number, db: PrismaClient) => {
6-
// Handle direct string tokens
7-
if (typeof token === 'string') {
8-
return token;
6+
if (typeof token !== 'object' || token === null) {
7+
throw new Error('Invalid token configuration');
98
}
109

1110
if ('secret' in token) {

‎packages/schemas/src/v3/shared.schema.test.ts‎

Lines changed: 155 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,155 @@
1+
import { describe, test, expect, beforeEach } from 'vitest';
2+
import Ajv from 'ajv';
3+
import { sharedSchema } from './shared.schema';
4+
5+
describe('shared schema validation', () => {
6+
let ajv: Ajv;
7+
8+
beforeEach(() => {
9+
ajv = new Ajv({ strict: false });
10+
});
11+
12+
describe('Token validation', () => {
13+
test('accepts valid secret token format', () => {
14+
const tokenSchema = sharedSchema.definitions!.Token;
15+
const validate = ajv.compile(tokenSchema);
16+
17+
const validToken = { secret: 'my-secret-name' };
18+
const isValid = validate(validToken);
19+
20+
expect(isValid).toBe(true);
21+
expect(validate.errors).toBeNull();
22+
});
23+
24+
test('accepts valid environment variable token format', () => {
25+
const tokenSchema = sharedSchema.definitions!.Token;
26+
const validate = ajv.compile(tokenSchema);
27+
28+
const validToken = { env: 'MY_TOKEN_VAR' };
29+
const isValid = validate(validToken);
30+
31+
expect(isValid).toBe(true);
32+
expect(validate.errors).toBeNull();
33+
});
34+
35+
test('rejects string tokens (security measure)', () => {
36+
const tokenSchema = sharedSchema.definitions!.Token;
37+
const validate = ajv.compile(tokenSchema);
38+
39+
const stringToken = 'direct-string-token';
40+
const isValid = validate(stringToken);
41+
42+
expect(isValid).toBe(false);
43+
expect(validate.errors).toBeTruthy();
44+
expect(validate.errors![0].message).toContain('must be object');
45+
});
46+
47+
test('rejects empty string tokens', () => {
48+
const tokenSchema = sharedSchema.definitions!.Token;
49+
const validate = ajv.compile(tokenSchema);
50+
51+
const emptyStringToken = '';
52+
const isValid = validate(emptyStringToken);
53+
54+
expect(isValid).toBe(false);
55+
expect(validate.errors).toBeTruthy();
56+
});
57+
58+
test('rejects malformed token objects', () => {
59+
const tokenSchema = sharedSchema.definitions!.Token;
60+
const validate = ajv.compile(tokenSchema);
61+
62+
const malformedToken = { invalid: 'format' };
63+
const isValid = validate(malformedToken);
64+
65+
expect(isValid).toBe(false);
66+
expect(validate.errors).toBeTruthy();
67+
});
68+
69+
test('rejects token objects with both secret and env', () => {
70+
const tokenSchema = sharedSchema.definitions!.Token;
71+
const validate = ajv.compile(tokenSchema);
72+
73+
const invalidToken = { secret: 'my-secret', env: 'MY_VAR' };
74+
const isValid = validate(invalidToken);
75+
76+
expect(isValid).toBe(false);
77+
expect(validate.errors).toBeTruthy();
78+
});
79+
80+
test('rejects empty secret name (security measure)', () => {
81+
const tokenSchema = sharedSchema.definitions!.Token;
82+
const validate = ajv.compile(tokenSchema);
83+
84+
const tokenWithEmptySecret = { secret: '' };
85+
const isValid = validate(tokenWithEmptySecret);
86+
87+
expect(isValid).toBe(false);
88+
expect(validate.errors).toBeTruthy();
89+
});
90+
91+
test('rejects empty environment variable name (security measure)', () => {
92+
const tokenSchema = sharedSchema.definitions!.Token;
93+
const validate = ajv.compile(tokenSchema);
94+
95+
const tokenWithEmptyEnv = { env: '' };
96+
const isValid = validate(tokenWithEmptyEnv);
97+
98+
expect(isValid).toBe(false);
99+
expect(validate.errors).toBeTruthy();
100+
});
101+
102+
test('rejects token objects with additional properties', () => {
103+
const tokenSchema = sharedSchema.definitions!.Token;
104+
const validate = ajv.compile(tokenSchema);
105+
106+
const invalidToken = { secret: 'my-secret', extra: 'property' };
107+
const isValid = validate(invalidToken);
108+
109+
expect(isValid).toBe(false);
110+
expect(validate.errors).toBeTruthy();
111+
});
112+
});
113+
114+
describe('GitRevisions validation', () => {
115+
test('accepts valid GitRevisions object', () => {
116+
const revisionsSchema = sharedSchema.definitions!.GitRevisions;
117+
const validate = ajv.compile(revisionsSchema);
118+
119+
const validRevisions = {
120+
branches: ['main', 'develop'],
121+
tags: ['v1.0.0', 'latest']
122+
};
123+
const isValid = validate(validRevisions);
124+
125+
expect(isValid).toBe(true);
126+
expect(validate.errors).toBeNull();
127+
});
128+
129+
test('accepts empty GitRevisions object', () => {
130+
const revisionsSchema = sharedSchema.definitions!.GitRevisions;
131+
const validate = ajv.compile(revisionsSchema);
132+
133+
const emptyRevisions = {};
134+
const isValid = validate(emptyRevisions);
135+
136+
expect(isValid).toBe(true);
137+
expect(validate.errors).toBeNull();
138+
});
139+
140+
test('rejects GitRevisions with additional properties', () => {
141+
const revisionsSchema = sharedSchema.definitions!.GitRevisions;
142+
const validate = ajv.compile(revisionsSchema);
143+
144+
const invalidRevisions = {
145+
branches: ['main'],
146+
tags: ['v1.0.0'],
147+
invalid: 'property'
148+
};
149+
const isValid = validate(invalidRevisions);
150+
151+
expect(isValid).toBe(false);
152+
expect(validate.errors).toBeTruthy();
153+
});
154+
});
155+
});

‎packages/schemas/src/v3/shared.schema.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,12 @@ const schema = {
55
"definitions": {
66
"Token": {
77
"anyOf": [
8-
{
9-
"type": "string",
10-
"description": "Direct token value (SECURITY RISK: not recommended for production - use secrets or environment variables instead)",
11-
"minLength": 1
12-
},
138
{
149
"type": "object",
1510
"properties": {
1611
"secret": {
1712
"type": "string",
13+
"minLength": 1,
1814
"description": "The name of the secret that contains the token."
1915
}
2016
},
@@ -28,6 +24,7 @@ const schema = {
2824
"properties": {
2925
"env": {
3026
"type": "string",
27+
"minLength": 1,
3128
"description": "The name of the environment variable that contains the token. Only supported in declarative connection configs."
3229
}
3330
},

‎packages/schemas/src/v3/shared.type.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
* via the `definition` "Token".
66
*/
77
export type Token =
8-
| string
98
| {
109
/**
1110
* The name of the secret that contains the token.

‎schemas/v3/shared.json‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,12 @@
44
"definitions": {
55
"Token": {
66
"anyOf": [
7-
{
8-
"type": "string",
9-
"description": "Direct token value (SECURITY RISK: not recommended for production - use secrets or environment variables instead)",
10-
"minLength": 1
11-
},
127
{
138
"type": "object",
149
"properties": {
1510
"secret": {
1611
"type": "string",
12+
"minLength": 1,
1713
"description": "The name of the secret that contains the token."
1814
}
1915
},
@@ -27,6 +23,7 @@
2723
"properties": {
2824
"env": {
2925
"type": "string",
26+
"minLength": 1,
3027
"description": "The name of the environment variable that contains the token. Only supported in declarative connection configs."
3128
}
3229
},

0 commit comments

Comments
 (0)