Skip to content

Commit 9647a01

Browse files
author
jolestar
committed
fix(holonbot): address review feedback for token broker hardening
1 parent a827421 commit 9647a01

File tree

2 files changed

+246
-143
lines changed

2 files changed

+246
-143
lines changed

‎holonbot/api/exchange-token.js‎

Lines changed: 164 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { verifyOIDCToken, validateClaims } from '../lib/oidc.js';
33

44
const replayCache = new Map();
55
const rateLimitCache = new Map();
6+
let lastCleanupAtMs = 0;
67
const permissionRank = {
78
none: 0,
89
read: 1,
@@ -12,6 +13,15 @@ const permissionRank = {
1213
admin: 5,
1314
};
1415

16+
class HttpError extends Error {
17+
constructor(status, code, message) {
18+
super(message);
19+
this.name = 'HttpError';
20+
this.status = status;
21+
this.code = code;
22+
}
23+
}
24+
1525
function parseBool(value, defaultValue) {
1626
if (value === undefined || value === null || value === '') {
1727
return defaultValue;
@@ -40,7 +50,7 @@ function parseCSV(value) {
4050
function getRequiredAudiences(env = process.env) {
4151
const audiences = parseCSV(env.HOLON_OIDC_AUDIENCE);
4252
if (audiences.length === 0) {
43-
throw new Error('Missing HOLON_OIDC_AUDIENCE configuration');
53+
throw new HttpError(500, 'config.invalid', 'Missing HOLON_OIDC_AUDIENCE configuration');
4454
}
4555
return audiences;
4656
}
@@ -53,11 +63,28 @@ function getInstallationPermissions(env = process.env) {
5363
try {
5464
const parsed = JSON.parse(raw);
5565
if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) {
56-
throw new Error('not an object');
66+
throw new HttpError(500, 'config.invalid', 'HOLON_INSTALLATION_PERMISSIONS_JSON must be an object');
5767
}
5868
return parsed;
5969
} catch (error) {
60-
throw new Error(`Invalid HOLON_INSTALLATION_PERMISSIONS_JSON: ${error.message}`);
70+
if (error instanceof HttpError) {
71+
throw error;
72+
}
73+
throw new HttpError(500, 'config.invalid', `Invalid HOLON_INSTALLATION_PERMISSIONS_JSON: ${error.message}`);
74+
}
75+
}
76+
77+
function sanitizeForLog(value) {
78+
return String(value ?? '').replace(/[\r\n\t]/g, ' ').replace(/[^\x20-\x7E]/g, '?');
79+
}
80+
81+
function enforceCacheLimit(map, maxSize) {
82+
while (map.size > maxSize) {
83+
const firstKey = map.keys().next().value;
84+
if (firstKey === undefined) {
85+
break;
86+
}
87+
map.delete(firstKey);
6188
}
6289
}
6390

@@ -74,6 +101,15 @@ function cleanExpiredEntries(now) {
74101
}
75102
}
76103

104+
function maybeCleanupCaches(now, env = process.env) {
105+
const cleanupIntervalMs = parseIntEnv(env.HOLON_CACHE_CLEANUP_INTERVAL_MS, 5000);
106+
if ((now - lastCleanupAtMs) < cleanupIntervalMs) {
107+
return;
108+
}
109+
cleanExpiredEntries(now);
110+
lastCleanupAtMs = now;
111+
}
112+
77113
function applyReplayProtection(claims, repository, env = process.env) {
78114
const enabled = parseBool(env.HOLON_ENABLE_REPLAY_PROTECTION, true);
79115
if (!enabled) {
@@ -82,18 +118,21 @@ function applyReplayProtection(claims, repository, env = process.env) {
82118

83119
const replayId = claims.jti || claims.runId;
84120
if (!replayId) {
85-
throw new Error('Missing jti/run_id claim required for replay protection');
121+
throw new HttpError(403, 'policy.replay.invalid_claim', 'Missing jti/run_id claim required for replay protection');
86122
}
87123

88124
const windowSeconds = parseIntEnv(env.HOLON_REPLAY_WINDOW_SECONDS, 3600);
125+
const maxCacheSize = parseIntEnv(env.HOLON_REPLAY_CACHE_MAX_SIZE, 10000);
89126
const now = Date.now();
90-
cleanExpiredEntries(now);
127+
maybeCleanupCaches(now, env);
91128

92129
const key = `${repository}:${replayId}`;
93130
if (replayCache.has(key)) {
94-
throw new Error('Replay detected for jti/run_id');
131+
throw new HttpError(403, 'policy.replay.detected', 'Replay detected for jti/run_id');
95132
}
133+
replayCache.delete(key);
96134
replayCache.set(key, now + (windowSeconds * 1000));
135+
enforceCacheLimit(replayCache, maxCacheSize);
97136
}
98137

99138
function applyRateLimit(claims, repository, env = process.env) {
@@ -104,9 +143,10 @@ function applyRateLimit(claims, repository, env = process.env) {
104143

105144
const windowSeconds = parseIntEnv(env.HOLON_RATE_LIMIT_WINDOW_SECONDS, 60);
106145
const maxRequests = parseIntEnv(env.HOLON_RATE_LIMIT_MAX_REQUESTS, 10);
146+
const maxCacheSize = parseIntEnv(env.HOLON_RATE_LIMIT_CACHE_MAX_SIZE, 10000);
107147
const actor = claims.actor || 'unknown';
108148
const now = Date.now();
109-
cleanExpiredEntries(now);
149+
maybeCleanupCaches(now, env);
110150

111151
const key = `${repository}:${actor}`;
112152
const windowMs = windowSeconds * 1000;
@@ -117,49 +157,62 @@ function applyRateLimit(claims, repository, env = process.env) {
117157
}
118158

119159
if (state.count >= maxRequests) {
120-
throw new Error('Rate limit exceeded');
160+
throw new HttpError(403, 'policy.rate_limited', 'Rate limit exceeded');
121161
}
122162

123-
state.count += 1;
124-
rateLimitCache.set(key, state);
163+
const updated = { ...state, count: state.count + 1 };
164+
rateLimitCache.delete(key);
165+
rateLimitCache.set(key, updated);
166+
enforceCacheLimit(rateLimitCache, maxCacheSize);
125167
}
126168

127-
async function assertActorPermission(appOctokit, claims, env = process.env) {
169+
async function assertActorPermission(installationOctokit, claims, env = process.env) {
128170
const enabled = parseBool(env.HOLON_REQUIRE_ACTOR_COLLABORATOR, true);
129171
if (!enabled) {
130172
return;
131173
}
132174

133175
const minPermission = String(env.HOLON_MIN_ACTOR_PERMISSION || 'read').toLowerCase();
134176
if (!Object.prototype.hasOwnProperty.call(permissionRank, minPermission)) {
135-
throw new Error(`Invalid HOLON_MIN_ACTOR_PERMISSION: ${minPermission}`);
177+
throw new HttpError(500, 'config.invalid', `Invalid HOLON_MIN_ACTOR_PERMISSION: ${minPermission}`);
136178
}
137179

138-
let permission = 'none';
180+
let permission;
139181
try {
140-
const response = await appOctokit.rest.repos.getCollaboratorPermissionLevel({
182+
const response = await installationOctokit.rest.repos.getCollaboratorPermissionLevel({
141183
owner: claims.owner,
142184
repo: claims.repo,
143185
username: claims.actor,
144186
});
145187
permission = String(response.data.permission || 'none').toLowerCase();
146188
} catch (error) {
147189
if (error.status === 404) {
148-
throw new Error(`Actor ${claims.actor} is not a collaborator/member`);
190+
throw new HttpError(403, 'policy.actor_not_collaborator', `Actor ${claims.actor} is not a collaborator/member`);
191+
}
192+
if (error.status === 401 || error.status === 403) {
193+
throw new HttpError(500, 'github.auth_failed', `Failed to verify collaborator permission: ${error.message}`);
194+
}
195+
if (error.status === 429) {
196+
throw new HttpError(503, 'github.rate_limited', 'GitHub API rate limit while verifying collaborator permission');
149197
}
150198
throw error;
151199
}
152200

153201
const actualRank = permissionRank[permission] ?? permissionRank.none;
154202
const requiredRank = permissionRank[minPermission];
155203
if (actualRank < requiredRank) {
156-
throw new Error(`Insufficient actor permission: ${claims.actor} has ${permission}, requires ${minPermission}`);
204+
throw new HttpError(
205+
403,
206+
'policy.actor_permission_insufficient',
207+
`Insufficient actor permission: ${claims.actor} has ${permission}, requires ${minPermission}`
208+
);
157209
}
158210
}
159211

160212
export function resetSecurityCachesForTests() {
161213
replayCache.clear();
162214
rateLimitCache.clear();
215+
lastCleanupAtMs = 0;
163216
}
164217

165218
export default async function handler(req, res) {
@@ -180,45 +233,29 @@ export default async function handler(req, res) {
180233
const audiences = getRequiredAudiences(process.env);
181234

182235
// 2. Verify OIDC Token and strict claims policy
183-
const claimsPayload = await verifyOIDCToken(token, { audiences });
184-
const appOctokit = await probot.auth();
185-
const claims = validateClaims(claimsPayload, {
186-
requireWorkflowRef: parseBool(process.env.HOLON_REQUIRE_JOB_WORKFLOW_REF, true),
187-
allowedWorkflowRefs: parseCSV(process.env.HOLON_ALLOWED_WORKFLOW_REFS),
188-
});
189-
190-
// 3. Resolve repository and bind token issuance to the same repository_id
191-
const repoResponse = await appOctokit.rest.repos.get({
192-
owner: claims.owner,
193-
repo: claims.repo,
194-
});
195-
const repository = claims.repository;
196-
const repositoryId = String(repoResponse.data.id);
197-
if (claims.repositoryId !== repositoryId) {
198-
return res.status(403).json({
199-
error: 'OIDC repository_id does not match target repository',
200-
});
236+
let claimsPayload;
237+
try {
238+
claimsPayload = await verifyOIDCToken(token, { audiences });
239+
} catch (error) {
240+
if (String(error.message || '').startsWith('Invalid OIDC Token:')) {
241+
throw new HttpError(401, 'oidc.invalid_token', error.message);
242+
}
243+
throw error;
201244
}
202245

203-
const enforceDefaultRef = parseBool(process.env.HOLON_REQUIRE_DEFAULT_BRANCH_REF, true);
204-
if (enforceDefaultRef) {
205-
const validated = validateClaims(claimsPayload, {
246+
let claims;
247+
try {
248+
claims = validateClaims(claimsPayload, {
206249
requireWorkflowRef: parseBool(process.env.HOLON_REQUIRE_JOB_WORKFLOW_REF, true),
207250
allowedWorkflowRefs: parseCSV(process.env.HOLON_ALLOWED_WORKFLOW_REFS),
208-
requireDefaultBranchRef: true,
209-
defaultBranch: repoResponse.data.default_branch,
210251
});
211-
claims.ref = validated.ref;
252+
} catch (error) {
253+
throw new HttpError(403, 'oidc.invalid_claims', error.message);
212254
}
213255

214-
console.log(`Token request for repository: ${repository} by actor: ${claims.actor}`);
215-
216-
// 4. Abuse protections and actor permission gate
217-
applyReplayProtection(claims, repository, process.env);
218-
applyRateLimit(claims, repository, process.env);
219-
await assertActorPermission(appOctokit, claims, process.env);
256+
const appOctokit = await probot.auth();
220257

221-
// 5. Find the app installation for this repository
258+
// 3. Find the app installation for this repository using app-authenticated client.
222259
let installation;
223260
try {
224261
const response = await appOctokit.rest.apps.getRepoInstallation({
@@ -230,14 +267,74 @@ export default async function handler(req, res) {
230267
if (err.status === 404) {
231268
return res.status(404).json({ error: 'HolonBot is not installed on this repository' });
232269
}
270+
if (err.status === 401 || err.status === 403) {
271+
throw new HttpError(500, 'github.auth_failed', `GitHub app authentication failed: ${err.message}`);
272+
}
273+
if (err.status === 429) {
274+
throw new HttpError(503, 'github.rate_limited', 'GitHub API rate limit while resolving repository installation');
275+
}
233276
throw err;
234277
}
235278

236-
// 6. Generate least-privilege installation token scoped to this repository only
279+
const installationOctokit = await probot.auth(installation.id);
280+
281+
// 4. Resolve repository metadata and bind token issuance to the same repository_id.
282+
let repoResponse;
283+
try {
284+
repoResponse = await installationOctokit.rest.repos.get({
285+
owner: claims.owner,
286+
repo: claims.repo,
287+
});
288+
} catch (error) {
289+
if (error.status === 404) {
290+
throw new HttpError(403, 'oidc.invalid_claims', 'Repository in OIDC token does not exist or is not accessible');
291+
}
292+
if (error.status === 401 || error.status === 403) {
293+
throw new HttpError(500, 'github.auth_failed', `Failed to query repository metadata: ${error.message}`);
294+
}
295+
if (error.status === 429) {
296+
throw new HttpError(503, 'github.rate_limited', 'GitHub API rate limit while reading repository metadata');
297+
}
298+
throw error;
299+
}
300+
301+
const repository = claims.repository;
302+
const repositoryId = String(repoResponse.data.id);
303+
if (claims.repositoryId !== repositoryId) {
304+
return res.status(403).json({
305+
error: 'OIDC repository_id does not match target repository',
306+
});
307+
}
308+
309+
const enforceDefaultRef = parseBool(process.env.HOLON_REQUIRE_DEFAULT_BRANCH_REF, true);
310+
if (enforceDefaultRef) {
311+
let validated;
312+
try {
313+
validated = validateClaims(claimsPayload, {
314+
requireWorkflowRef: parseBool(process.env.HOLON_REQUIRE_JOB_WORKFLOW_REF, true),
315+
allowedWorkflowRefs: parseCSV(process.env.HOLON_ALLOWED_WORKFLOW_REFS),
316+
requireDefaultBranchRef: true,
317+
defaultBranch: repoResponse.data.default_branch,
318+
});
319+
} catch (error) {
320+
throw new HttpError(403, 'oidc.invalid_claims', error.message);
321+
}
322+
claims.ref = validated.ref;
323+
}
324+
325+
console.log(
326+
`Token request for repository: ${sanitizeForLog(repository)} by actor: ${sanitizeForLog(claims.actor)}`
327+
);
328+
329+
// 5. Abuse protections and actor permission gate
330+
applyReplayProtection(claims, repository, process.env);
331+
applyRateLimit(claims, repository, process.env);
332+
await assertActorPermission(installationOctokit, claims, process.env);
333+
334+
// 6. Generate least-privilege installation token scoped to this repository only.
237335
const installationPermissions = getInstallationPermissions(process.env);
238-
const installationId = installation.id;
239336
const installationTokenResponse = await appOctokit.rest.apps.createInstallationAccessToken({
240-
installation_id: installationId,
337+
installation_id: installation.id,
241338
repository_ids: [repoResponse.data.id],
242339
permissions: installationPermissions,
243340
});
@@ -251,23 +348,24 @@ export default async function handler(req, res) {
251348

252349
} catch (error) {
253350
console.error('Token Exchange Error:', error);
254-
if (/^Invalid OIDC Token:/.test(error.message)) {
255-
return res.status(401).json({
256-
error: 'Invalid OIDC token',
351+
if (error instanceof HttpError) {
352+
const response = {
353+
error: 'Token exchange failed',
354+
code: error.code,
257355
message: error.message,
258-
});
259-
}
260-
if (/Missing HOLON_OIDC_AUDIENCE|Missing .* claim|repository_id|job_workflow_ref|sub claim|default branch|repository_owner|repository format/.test(error.message)) {
261-
return res.status(403).json({
262-
error: 'OIDC claims validation failed',
263-
message: error.message,
264-
});
265-
}
266-
if (/Replay detected|Rate limit exceeded|collaborator\/member|Insufficient actor permission/.test(error.message)) {
267-
return res.status(403).json({
268-
error: 'Token request rejected by security policy',
269-
message: error.message,
270-
});
356+
};
357+
if (error.status === 401) {
358+
response.error = 'Invalid OIDC token';
359+
} else if (error.status === 403 && error.code.startsWith('oidc.')) {
360+
response.error = 'OIDC claims validation failed';
361+
} else if (error.status === 403 && error.code.startsWith('policy.')) {
362+
response.error = 'Token request rejected by security policy';
363+
} else if (error.status >= 500 && error.code.startsWith('github.')) {
364+
response.error = 'GitHub API error';
365+
} else if (error.status >= 500 && error.code.startsWith('config.')) {
366+
response.error = 'Token broker misconfiguration';
367+
}
368+
return res.status(error.status).json(response);
271369
}
272370
return res.status(500).json({
273371
error: 'Token exchange failed',

0 commit comments

Comments
 (0)