From c8d62959bfdc106c6c6438ed212bf6dd3e68dfa1 Mon Sep 17 00:00:00 2001 From: Satish Jhanwer Date: Tue, 4 Aug 2026 09:54:16 +0530 Subject: [PATCH 1/4] fix: send GraphQL query/variables as POST body soxa.post(url, data, config) uses the explicit data argument over config.data when merging, so passing {} as data and {query, variables} inside config.data silently sent an empty body to the GitHub GraphQL API on every request. --- src/Services/request.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Services/request.ts b/src/Services/request.ts index ccc1a9f1..0a1fa345 100644 --- a/src/Services/request.ts +++ b/src/Services/request.ts @@ -11,8 +11,7 @@ export async function requestGithubData( variables: { [key: string]: string }, token = "", ) { - const response = await soxa.post("", {}, { - data: { query, variables }, + const response = await soxa.post("", { query, variables }, { headers: { Authorization: `bearer ${token}`, }, From 3cd1959c55559d0a6762d72e1156769b5dbdfcc4 Mon Sep 17 00:00:00 2001 From: Satish Jhanwer Date: Tue, 4 Aug 2026 09:55:13 +0530 Subject: [PATCH 2/4] fix: guard UserInfo against null/missing GraphQL sub-fields GitHub's GraphQL API can return null for fields like organizations when the token lacks the relevant scope, and null entries inside repositories.nodes for repos it can't fully resolve for the given token. UserInfo's constructor did unguarded property access on these, throwing TypeErrors that were silently swallowed by an empty catch block elsewhere. Default missing sub-fields to 0/[] and filter null nodes out of repositories.nodes before use. --- src/user_info.ts | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/user_info.ts b/src/user_info.ts index 1f3426db..b6245c60 100644 --- a/src/user_info.ts +++ b/src/user_info.ts @@ -72,19 +72,23 @@ export class UserInfo { userPullRequest: GitHubUserPullRequest, userRepository: GitHubUserRepository, ) { + const repoNodes = (userRepository.repositories?.nodes ?? []).filter( + (node): node is Repository => node != undefined, + ); + const totalCommits = - userActivity.contributionsCollection.restrictedContributionsCount + - userActivity.contributionsCollection.totalCommitContributions; - const totalStargazers = userRepository.repositories.nodes.reduce( + (userActivity.contributionsCollection?.restrictedContributionsCount ?? 0) + + (userActivity.contributionsCollection?.totalCommitContributions ?? 0); + const totalStargazers = repoNodes.reduce( (prev: number, node: Repository) => { - return prev + node.stargazerCount; + return prev + (node.stargazerCount ?? 0); }, 0, ); const languages = new Set(); - userRepository.repositories.nodes.forEach((node: Repository) => { - if (node.languages.nodes != undefined) { + repoNodes.forEach((node: Repository) => { + if (node.languages?.nodes != undefined) { node.languages.nodes.forEach((node: Language) => { if (node != undefined) { languages.add(node.name); @@ -96,7 +100,7 @@ export class UserInfo { // Find the earliest repository creation date let earliestRepoDate = userActivity.createdAt; // start with the oldest possible - earliestRepoDate = userRepository.repositories.nodes.reduce( + earliestRepoDate = repoNodes.reduce( (earliest, node) => { return new Date(node.createdAt).getTime() < new Date(earliest).getTime() ? node.createdAt @@ -118,15 +122,15 @@ export class UserInfo { const ogAccount = new Date(earliestRepoDate).getFullYear() <= 2008 ? 1 : 0; this.totalCommits = totalCommits; - this.totalFollowers = userActivity.followers.totalCount; - this.totalIssues = userIssue.openIssues.totalCount + - userIssue.closedIssues.totalCount; - this.totalOrganizations = userActivity.organizations.totalCount; - this.totalPullRequests = userPullRequest.pullRequests.totalCount; + this.totalFollowers = userActivity.followers?.totalCount ?? 0; + this.totalIssues = (userIssue.openIssues?.totalCount ?? 0) + + (userIssue.closedIssues?.totalCount ?? 0); + this.totalOrganizations = userActivity.organizations?.totalCount ?? 0; + this.totalPullRequests = userPullRequest.pullRequests?.totalCount ?? 0; this.totalReviews = - userActivity.contributionsCollection.totalPullRequestReviewContributions; + userActivity.contributionsCollection?.totalPullRequestReviewContributions ?? 0; this.totalStargazers = totalStargazers; - this.totalRepositories = userRepository.repositories.totalCount; + this.totalRepositories = userRepository.repositories?.totalCount ?? 0; this.languageCount = languages.size; this.durationYear = durationYear; this.durationDays = durationDays; From 0b705ce0a70697d2878ceef5d6cea4a4d5515bd2 Mon Sep 17 00:00:00 2001 From: Satish Jhanwer Date: Tue, 4 Aug 2026 09:55:20 +0530 Subject: [PATCH 3/4] fix: surface real errors without logging sensitive request data requestUserInfo had a bare catch {} that discarded the actual thrown error, making failures impossible to diagnose from CI logs. Fixing that surfaced a second issue: soxa attaches the full request config (including the Authorization bearer token header) onto thrown errors and defines toJSON() to re-serialize it, and executeQuery/Retry logged raw error objects or JSON.stringify(error.cause), which would print that header into the Actions log. Added safeErrorMessage(), which only ever extracts .message from a thrown value, and routed all error logging in these files through it. --- src/Helpers/Retry.ts | 3 ++- src/Helpers/safeErrorMessage.ts | 9 +++++++++ src/Services/GithubApiService.ts | 13 +++++++------ 3 files changed, 18 insertions(+), 7 deletions(-) create mode 100644 src/Helpers/safeErrorMessage.ts diff --git a/src/Helpers/Retry.ts b/src/Helpers/Retry.ts index 7718f78a..53143ae1 100644 --- a/src/Helpers/Retry.ts +++ b/src/Helpers/Retry.ts @@ -1,5 +1,6 @@ import { ServiceError } from "../Types/index.ts"; import { Logger } from "./Logger.ts"; +import { safeErrorMessage } from "./safeErrorMessage.ts"; export type RetryCallbackProps = { attempt: number; @@ -25,7 +26,7 @@ async function* createAsyncIterable( } yield null; - Logger.error(e); + Logger.error(safeErrorMessage(e)); await new Promise((resolve) => setTimeout(resolve, delay)); } } diff --git a/src/Helpers/safeErrorMessage.ts b/src/Helpers/safeErrorMessage.ts new file mode 100644 index 00000000..086b957f --- /dev/null +++ b/src/Helpers/safeErrorMessage.ts @@ -0,0 +1,9 @@ +// Errors from soxa carry a .config with the Authorization header attached +// (and a .toJSON() that serializes it back out), so only ever log the +// message - never the raw error/cause/config, which could leak the token. +export function safeErrorMessage(error: unknown): string { + if (error instanceof Error) { + return error.message; + } + return String(error); +} diff --git a/src/Services/GithubApiService.ts b/src/Services/GithubApiService.ts index fbcc8f0e..6b73f7eb 100644 --- a/src/Services/GithubApiService.ts +++ b/src/Services/GithubApiService.ts @@ -19,6 +19,7 @@ import { CONSTANTS } from "../utils.ts"; import { EServiceKindError, ServiceError } from "../Types/index.ts"; import { Logger } from "../Helpers/Logger.ts"; import { requestGithubData } from "./request.ts"; +import { safeErrorMessage } from "../Helpers/safeErrorMessage.ts"; // Need to be here - Exporting from another file makes array of null export const TOKENS = [ @@ -71,8 +72,9 @@ export class GithubApiService extends GithubRepository { return result; } return UserInfo.fromCombined(result); - } catch { + } catch (error) { Logger.error(`Error fetching user info for username: ${username}`); + Logger.error(safeErrorMessage(error)); return new ServiceError("Not found", EServiceKindError.NOT_FOUND); } } @@ -98,11 +100,10 @@ export class GithubApiService extends GithubRepository { Logger.error(error.cause.message); return error.cause; } - if (error instanceof Error && error.cause) { - Logger.error(JSON.stringify(error.cause, null, 2)); - } else { - Logger.error(error); - } + const cause = error instanceof Error && error.cause + ? error.cause + : error; + Logger.error(safeErrorMessage(cause)); return new ServiceError("not found", EServiceKindError.NOT_FOUND); } } From 9f30704563b515641a23ee3c90b32f1626368b41 Mon Sep 17 00:00:00 2001 From: Satish Jhanwer Date: Tue, 4 Aug 2026 10:35:59 +0530 Subject: [PATCH 4/4] test: add regression tests for the three bugs fixed - request.ts: assert the POST body actually sent to soxa.post is {query, variables}, not {} - fails on the pre-fix code with the exact empty-body bug. - user_info.ts: assert UserInfo defaults null/missing GraphQL sub-fields instead of throwing - fails on the pre-fix code with the exact 'Cannot read properties of null' crash seen in production. - safeErrorMessage.ts: unit tests for the new helper, including asserting it never surfaces extra properties (e.g. a token in error.config) attached to an Error. Verified both regression tests fail against the original buggy src/Services/request.ts and src/user_info.ts before being fixed, and that the full suite (15 tests) passes with the fixes applied. --- .../__tests__/safeErrorMessage.test.ts | 27 ++++++++++++ src/Services/__tests__/request.test.ts | 29 +++++++++++++ src/__tests__/user_info.test.ts | 43 +++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 src/Helpers/__tests__/safeErrorMessage.test.ts diff --git a/src/Helpers/__tests__/safeErrorMessage.test.ts b/src/Helpers/__tests__/safeErrorMessage.test.ts new file mode 100644 index 00000000..83d65272 --- /dev/null +++ b/src/Helpers/__tests__/safeErrorMessage.test.ts @@ -0,0 +1,27 @@ +import { assertEquals } from "../../../deps.ts"; +import { safeErrorMessage } from "../safeErrorMessage.ts"; + +Deno.test("safeErrorMessage extracts only the message from an Error", () => { + const error = new Error("boom"); + assertEquals(safeErrorMessage(error), "boom"); +}); + +Deno.test("safeErrorMessage never surfaces extra properties attached to an Error", () => { + // soxa attaches the full request config (including the Authorization + // header) onto thrown errors. safeErrorMessage must only ever return + // .message, never those extra properties. + const error = new Error("request failed") as Error & { config: unknown }; + error.config = { + headers: { Authorization: "bearer super-secret-token" }, + }; + + const message = safeErrorMessage(error); + + assertEquals(message, "request failed"); + assertEquals(message.includes("super-secret-token"), false); +}); + +Deno.test("safeErrorMessage stringifies non-Error values", () => { + assertEquals(safeErrorMessage("plain string"), "plain string"); + assertEquals(safeErrorMessage(null), "null"); +}); diff --git a/src/Services/__tests__/request.test.ts b/src/Services/__tests__/request.test.ts index 1a1c5f79..dae15c16 100644 --- a/src/Services/__tests__/request.test.ts +++ b/src/Services/__tests__/request.test.ts @@ -2,6 +2,35 @@ import { assertEquals, assertRejects, soxa, stub } from "../../../deps.ts"; import { EServiceKindError, ServiceError } from "../../Types/index.ts"; import { requestGithubData } from "../request.ts"; +Deno.test("requestGithubData sends query and variables as the POST body", async () => { + const post = stub(soxa, "post", () => { + return Promise.resolve({ + data: { data: { user: { login: "test" } } }, + }); + }); + + try { + await requestGithubData( + "query { viewer { login } }", + { username: "test" }, + "tok", + ); + + assertEquals(post.calls.length, 1); + const [, data] = post.calls[0].args; + // Regression check: soxa.post(url, data, config) uses the explicit + // `data` argument over `config.data` when merging, so the actual + // request body must be passed as the 2nd argument, not nested + // inside the 3rd (config) argument. + assertEquals(data, { + query: "query { viewer { login } }", + variables: { username: "test" }, + }); + } finally { + post.restore(); + } +}); + Deno.test("requestGithubData rejects partial GraphQL responses", async () => { const post = stub(soxa, "post", () => { return Promise.resolve({ diff --git a/src/__tests__/user_info.test.ts b/src/__tests__/user_info.test.ts index ae32d706..9ea70ff4 100644 --- a/src/__tests__/user_info.test.ts +++ b/src/__tests__/user_info.test.ts @@ -39,3 +39,46 @@ Deno.test("UserInfo calculates total stargazers", () => { assertEquals(userInfo.totalStargazers, 8); }); + +Deno.test("UserInfo defaults null/missing GraphQL sub-fields instead of throwing", () => { + // GitHub's GraphQL API can return null for fields like `organizations` + // when the token lacks the relevant scope, and null entries inside + // `repositories.nodes` for repos it can't fully resolve for the given + // token. UserInfo must degrade gracefully instead of throwing. + const userInfo = new UserInfo( + { + createdAt: "2025-01-01T00:00:00Z", + contributionsCollection: { + restrictedContributionsCount: 0, + totalCommitContributions: 0, + totalPullRequestReviewContributions: 0, + }, + organizations: null as unknown as { totalCount: number }, + followers: null as unknown as { totalCount: number }, + }, + { + openIssues: { totalCount: 0 }, + closedIssues: { totalCount: 0 }, + }, + { pullRequests: { totalCount: 0 } }, + { + repositories: { + totalCount: 1, + // deno-lint-ignore no-explicit-any + nodes: [ + null as any, + { + languages: { nodes: [] }, + stargazerCount: 5, + createdAt: "2025-01-02T00:00:00Z", + }, + ], + }, + }, + ); + + assertEquals(userInfo.totalOrganizations, 0); + assertEquals(userInfo.totalFollowers, 0); + assertEquals(userInfo.totalStargazers, 5); + assertEquals(userInfo.totalRepositories, 1); +});