Skip to content

Commit 805daab

Browse files
committed
Improve GitHub integration error handling and tests
Refactored GitHub OAuth and installation flow to provide more accurate error redirects and preserve existing taskManager config values. Enhanced timing-safe signature validation using crypto.timingSafeEqual. Updated GraphQL query in GitHubService to use variable for issue number. Expanded and improved test coverage for GitHub routes, including edge cases and config preservation. Refactored project resolver tests for better type safety and error handling.
1 parent 8c4c3c3 commit 805daab

4 files changed

Lines changed: 700 additions & 61 deletions

File tree

src/integrations/github/routes.ts

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import '../../typeDefs/expressContext';
22
import express from 'express';
33
import { v4 as uuid } from 'uuid';
44
import { ObjectId } from 'mongodb';
5-
import { createHmac } from 'crypto';
5+
import { createHmac, timingSafeEqual } from 'crypto';
66
import { GitHubService } from './service';
77
import { ProjectDBScheme } from '@hawk.so/types';
88
import { ContextFactories } from '../../types/graphql';
@@ -289,13 +289,13 @@ export function createGitHubRouter(factories: ContextFactories): express.Router
289289
* Validate required parameters
290290
*/
291291
if (!code || typeof code !== 'string') {
292-
return res.redirect(buildGarageRedirectUrl('/project/error/settings/task-manager', {
292+
return res.redirect(buildGarageRedirectUrl('/', {
293293
error: 'Missing or invalid OAuth code',
294294
}));
295295
}
296296

297297
if (!state || typeof state !== 'string') {
298-
return res.redirect(buildGarageRedirectUrl('/project/error/settings/task-manager', {
298+
return res.redirect(buildGarageRedirectUrl('/', {
299299
error: 'Missing or invalid state',
300300
}));
301301
}
@@ -309,7 +309,7 @@ export function createGitHubRouter(factories: ContextFactories): express.Router
309309
if (!stateData) {
310310
log('warn', `Invalid or expired state: ${sgr(state.slice(0, 8), Effect.ForegroundGray)}...`);
311311

312-
return res.redirect(buildGarageRedirectUrl('/project/error/settings/task-manager', {
312+
return res.redirect(buildGarageRedirectUrl('/', {
313313
error: 'Invalid or expired state. Please try connecting again.',
314314
}));
315315
}
@@ -326,7 +326,7 @@ export function createGitHubRouter(factories: ContextFactories): express.Router
326326
if (!project) {
327327
log('error', projectId, 'Project not found');
328328

329-
return res.redirect(buildGarageRedirectUrl('/project/error/settings/task-manager', {
329+
return res.redirect(buildGarageRedirectUrl(`/project/${projectId}/settings/task-manager`, {
330330
error: `Project not found: ${projectId}`,
331331
}));
332332
}
@@ -381,6 +381,9 @@ export function createGitHubRouter(factories: ContextFactories): express.Router
381381
...project.taskManager.config,
382382
// eslint-disable-next-line @typescript-eslint/camelcase, camelcase
383383
installationId: installation_id,
384+
// Preserve existing repoId and repoFullName if they exist, otherwise use defaults
385+
repoId: project.taskManager.config.repoId || taskManagerConfig.config.repoId,
386+
repoFullName: project.taskManager.config.repoFullName || taskManagerConfig.config.repoFullName,
384387
},
385388
} : taskManagerConfig,
386389
} as Partial<ProjectDBScheme>);
@@ -402,7 +405,7 @@ export function createGitHubRouter(factories: ContextFactories): express.Router
402405
if (!updatedProject) {
403406
log('error', projectId, 'Project not found after update');
404407

405-
return res.redirect(buildGarageRedirectUrl('/project/error/settings/task-manager', {
408+
return res.redirect(buildGarageRedirectUrl(`/project/${projectId}/settings/task-manager`, {
406409
error: `Project not found: ${projectId}`,
407410
}));
408411
}
@@ -543,19 +546,26 @@ export function createGitHubRouter(factories: ContextFactories): express.Router
543546

544547
/**
545548
* Use timing-safe comparison to prevent timing attacks
549+
* timingSafeEqual requires both arguments to be Buffer or Uint8Array of the same length
546550
*/
547551
let signatureValid = false;
548552

549553
if (signature.length === calculatedSignature.length) {
550-
let match = true;
554+
try {
555+
const signatureBuffer = Buffer.from(signature);
556+
const calculatedBuffer = Buffer.from(calculatedSignature);
551557

552-
for (let i = 0; i < signature.length; i++) {
553-
if (signature[i] !== calculatedSignature[i]) {
554-
match = false;
555-
}
558+
signatureValid = timingSafeEqual(
559+
signatureBuffer as Uint8Array,
560+
calculatedBuffer as Uint8Array
561+
);
562+
} catch (error) {
563+
/**
564+
* timingSafeEqual throws if buffers have different lengths
565+
* This shouldn't happen due to the length check above, but handle it gracefully
566+
*/
567+
signatureValid = false;
556568
}
557-
558-
signatureValid = match;
559569
}
560570

561571
if (!signatureValid) {

src/integrations/github/service.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -435,10 +435,10 @@ export class GitHubService {
435435
* Step 1: Get repository ID and find Copilot bot ID
436436
*/
437437
const repoInfoQuery = `
438-
query($owner: String!, $name: String!) {
438+
query($owner: String!, $name: String!, $issueNumber: Int!) {
439439
repository(owner: $owner, name: $name) {
440440
id
441-
issue(number: ${issueNumber}) {
441+
issue(number: $issueNumber) {
442442
id
443443
}
444444
suggestedActors(capabilities: [CAN_BE_ASSIGNED], first: 100) {
@@ -470,6 +470,7 @@ export class GitHubService {
470470
const repoInfo = await octokit.graphql<RepoInfoGraphQLResponse>(repoInfoQuery, {
471471
owner,
472472
name: repo,
473+
issueNumber,
473474
});
474475

475476
console.log('[GitHub API] Repository info query response:', JSON.stringify(repoInfo, null, 2));

0 commit comments

Comments
 (0)