prove github creds via graphql (faster, but token always required) - #4
prove github creds via graphql (faster, but token always required)#4Chmarusso wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis pull request refactors API request handling to support both GraphQL and REST endpoints. A new utility module ( Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
app/api/prove/route.ts (2)
3-4: AlignmaxDurationwith comment or platform limitThe comment says “up to 90 seconds” but
maxDurationis set to160. This mismatch is confusing and could suggest an unsupported runtime config. Consider either updating the comment to match the chosen value or loweringmaxDurationif 90s is the actual platform limit.
13-31: Guard GraphQL path with clearer validation (optional)The GraphQL branch currently triggers whenever
body.queryis truthy, and silently allows missinggithubToken(GitHub will then 401). If this endpoint is used beyond your own UI, you may want to explicitly validate thatbody.queryis a non‑empty string and that a token is present when taking the GraphQL path, returning a 400 for bad input instead of delegating all failures to GitHub.lib/graphql-transformer.ts (1)
196-221: HardenextractRepoInfoagainst non‑stringrequestBody.query
requestBody.query.match(...)andrequestBody.query.substring(...)assumequeryis a string. If an upstream change ever sends a non‑string (e.g., object or null), this will throw before reaching the outer catch. A small type guard would make this more robust.- // If no variables, try to extract from GraphQL query string (search query format) - if ((!repoOwner || !repoName) && requestBody.query) { + // If no variables, try to extract from GraphQL query string (search query format) + if ((!repoOwner || !repoName) && typeof requestBody.query === 'string' && requestBody.query) { // Parse search query like: repo:owner/name is:pr is:merged author:username // Match pattern: repo:owner/name (handles cases where name might have special chars) const searchQueryMatch = requestBody.query.match(/repo:([^\/\s"']+)\/([^\s"']+)/); if (searchQueryMatch) { repoOwner = searchQueryMatch[1]; // Remove any trailing characters that might be part of the query (like quotes or spaces) repoName = searchQueryMatch[2].replace(/["']/g, '').trim(); console.log(`Extracted repo info from search query: ${repoOwner}/${repoName}`); } else { - console.log('Could not extract repo from search query:', requestBody.query.substring(0, 100)); + console.log('Could not extract repo from search query:', requestBody.query.substring(0, 100)); } }app/page.tsx (3)
12-18: Type improvements look good; consider tighteningtypeThe stricter typing for
presentationandresult.data.contributionDatais a nice step away fromany. As a further (optional) improvement, you could narrowtypefromstringto a union like'prove' | 'verify'to get exhaustiveness checking where you branch onresult.type.
50-65: Prefer GraphQL variables for the search query instead of string interpolation
searchQueryis interpolated directly into the GraphQL string (query: "${searchQuery}"). While owner/repo/username are constrained on GitHub, building queries this way is brittle (quoting/escaping issues) and harder to evolve. Consider switching to a parameterized query with a$searchQueryvariable and sending it via thevariablesfield (which/api/provealready supports):const graphqlQuery = ` query MergedPrCount($searchQuery: String!) { search( query: $searchQuery type: ISSUE first: 1 ) { issueCount } } `; // and in the fetch body: body: JSON.stringify({ query: graphqlQuery, variables: { searchQuery }, githubToken: githubToken.trim(), });This avoids manual quoting and keeps the query template cleaner.
125-173: GraphQL response parsing duplicates server transformer logicThe parsing here (checking
data.search.issueCount,repository.pullRequests.nodes,totalCount, and array length) closely mirrorstransformGraphQLPRResponseinlib/graphql-transformer.ts. To reduce drift and bugs if GitHub’s shapes change, you might consider reusing that transformer on the client as well (it already accepts an object, not only strings) and mapping itscontributionsfield into yourcontributionData.total.import { transformGraphQLPRResponse } from '@/lib/graphql-transformer'; // ... if (data.response && data.response.body) { try { const contributor = transformGraphQLPRResponse( JSON.parse(data.response.body), username.trim() ); if (!contributor || contributor.contributions === 0) { setError(`No merged PRs found for @${username.trim()} in repository`); return; } contributionData = { username: contributor.username, total: contributor.contributions, avatar: contributor.avatar, }; } catch { setError('Failed to parse contribution data'); return; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/api/prove/route.ts(2 hunks)app/api/upload-proof/route.ts(3 hunks)app/api/verify-all/route.ts(2 hunks)app/page.tsx(9 hunks)lib/graphql-transformer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
app/api/upload-proof/route.ts (1)
lib/graphql-transformer.ts (2)
extractRepoInfo(192-255)transformGraphQLPRResponse(44-187)
app/api/verify-all/route.ts (1)
lib/graphql-transformer.ts (2)
extractRepoInfo(192-255)transformGraphQLPRResponse(44-187)
app/api/prove/route.ts (1)
next.config.ts (1)
headers(6-18)
🔇 Additional comments (4)
app/api/upload-proof/route.ts (1)
3-4: Good consolidation of parsing via shared transformersUsing
extractRepoInfo(verifyData)andtransformGraphQLPRResponse(verifyData.response.body, username.trim())removes duplicated parsing logic and centralizes handling of GraphQL vs legacy REST/commits formats. This should make supporting new response shapes and pagination much easier going forward.Also applies to: 72-95
lib/graphql-transformer.ts (1)
65-185: Transformer logic looks solid across GraphQL and legacy shapesThe control flow in
transformGraphQLPRResponsecovers all expected formats (search.issueCount, PR totalCount, PR nodes, contributors array, commits array) and always falls back tonullon empty or error cases, which matches how the callers treat “no contributions found”. This is a good centralization of the parsing logic.app/api/verify-all/route.ts (1)
81-84: Nice reuse of shared transformers to simplify aggregationUsing
extractRepoInfoandtransformGraphQLPRResponsehere makes the “verify all” flow much simpler and keeps contributor shaping consistent with/api/upload-proof. TherepoUrlderivation from{owner, name}is also a good touch for downstream display.Also applies to: 87-105
app/page.tsx (1)
36-45: Client-side validation and button disabling align well with new GraphQL flowRequiring both username and GitHub token before calling
handleProve, and wiring that into the button’sdisabledstate, nicely prevents invalid GraphQL prove requests from ever hitting the backend and surfaces clear error messages to the user.Also applies to: 310-316
| // Construct headers for GraphQL request | ||
| const headers = [ | ||
| "User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/141.0.0.0 Safari/537.36", | ||
| "Content-Type: application/json", | ||
| "Accept: application/vnd.github+json" | ||
| ] | ||
| }; | ||
| ]; | ||
|
|
||
| if (body.githubToken) { | ||
| headers.push(`Authorization: Bearer ${body.githubToken}`); | ||
| } | ||
|
|
||
| requestBody = { | ||
| url: graphqlUrl, | ||
| method: 'POST', | ||
| headers: headers, | ||
| body: graphqlBody | ||
| }; |
There was a problem hiding this comment.
Do not log GitHub PAT in requestBody
requestBody.headers includes Authorization: Bearer ${body.githubToken}, and console.log('Request body:', JSON.stringify(requestBody, null, 2)); will emit that PAT into server logs. This is high‑risk credential leakage; please avoid logging the full request body or explicitly strip/redact the Authorization header before logging.
- console.log('Request body:', JSON.stringify(requestBody, null, 2));
+ // Avoid logging full requestBody because it may contain sensitive headers (e.g., GitHub PATs).
+ // If needed for debugging, log only non-sensitive metadata:
+ console.debug('Request body summary:', {
+ url: requestBody.url,
+ method: requestBody.method,
+ headerCount: Array.isArray(requestBody.headers) ? requestBody.headers.length : undefined,
+ });Also applies to: 61-62
🤖 Prompt for AI Agents
In app/api/prove/route.ts around lines 21 to 37 (and also where requestBody is
logged around lines 61-62), the requestBody is built with an Authorization
header containing the GitHub PAT which must not be emitted to logs; update
logging to never print the full requestBody or to redact sensitive headers by
removing or replacing the Authorization header value before any
JSON.stringify/console.log (e.g., clone the requestBody for logging and set
cloned.headers to include a redacted Authorization or filter it out entirely),
and ensure any other logs that could include body.githubToken are removed or
redacted.
| const verifyData = await verifyResponse.json(); | ||
|
|
||
| console.log('Verify data:', verifyData); | ||
|
|
There was a problem hiding this comment.
Avoid logging full verification payloads (GitHub PAT leakage risk)
console.log('Verify data:', verifyData); combined with the fact that verifyData carries the original request (including headers) means GitHub personal access tokens sent via /api/prove will end up in server logs. That’s a serious secret‑exposure risk; please remove or strictly sanitize these logs (e.g., drop headers, especially Authorization) before logging.
- console.log('Verify data:', verifyData);
+ // Avoid logging full verifyData because it may contain sensitive headers (e.g., GitHub PATs).
+ // console.debug('Verify summary:', {
+ // hasRequest: !!verifyData.request,
+ // hasResponse: !!verifyData.response,
+ // });Also applies to: 79-83
🤖 Prompt for AI Agents
In app/api/upload-proof/route.ts around lines 58-61 (and also remove or fix
similar logging at 79-83), the code logs the full verifyData object which
contains request headers and can leak GitHub PATs; remove the full console.log
or replace it with a sanitized log that omits headers (especially Authorization)
and any sensitive fields, e.g. log only safe metadata like response status or
specific non‑sensitive properties; ensure any future logging explicitly strips
or redacts headers.Authorization before output.
| console.log('Verify response structure:', JSON.stringify(verifyData, null, 2)); | ||
|
|
||
| // Extract repository info from the verified response | ||
| let repoOwner: string | null = null; | ||
| let repoName: string | null = null; | ||
| try { | ||
| // Try multiple locations where the URL might be | ||
| let urlToMatch = null; | ||
|
|
||
| // Check different possible locations | ||
| if (verifyData?.response?.url) { | ||
| urlToMatch = verifyData.response.url; | ||
| } else if (verifyData?.request?.url) { | ||
| urlToMatch = verifyData.request.url; | ||
| } else if (verifyData?.response?.request) { | ||
| urlToMatch = verifyData.response.request; | ||
| } | ||
|
|
||
| console.log('URL to match:', urlToMatch); | ||
|
|
||
| if (urlToMatch) { | ||
| const urlMatch = urlToMatch.match(/\/repos\/([^\/]+)\/([^\/]+)\/contributors/); | ||
| if (urlMatch) { | ||
| repoOwner = urlMatch[1]; | ||
| repoName = urlMatch[2]; | ||
| console.log(`Extracted repo: ${repoOwner}/${repoName}`); | ||
| } | ||
| } | ||
| } catch (e) { | ||
| console.log('Could not extract repo info from response:', e); | ||
| } | ||
| const { owner: repoOwner, name: repoName } = extractRepoInfo(verifyData); | ||
| console.log(`Extracted repo: ${repoOwner}/${repoName}`); | ||
|
|
There was a problem hiding this comment.
Sanitize or remove logs that include full verification payloads
Between console.log('Verify response structure:', JSON.stringify(verifyData, null, 2)); and logging of responseBody, you’re likely emitting the original request (including GitHub Authorization headers from the webproof) into server logs. That PAT is sensitive and should not be logged. Recommend either removing these logs or logging only high‑level metadata (e.g., filenames, repo owner/name, counts) without headers.
- console.log('Verify response structure:', JSON.stringify(verifyData, null, 2));
+ // Avoid logging full verifyData; it may contain sensitive headers (e.g., GitHub PATs).
+ console.debug('Verify response summary:', {
+ file,
+ hasRequest: !!verifyData.request,
+ hasResponse: !!verifyData.response,
+ });
@@
- const responseBody = verifyData.response.body;
- const contributorData = transformGraphQLPRResponse(responseBody, targetUsername || undefined);
+ const responseBody = verifyData.response.body;
+ const contributorData = transformGraphQLPRResponse(responseBody, targetUsername || undefined);Also applies to: 87-91
🤖 Prompt for AI Agents
In app/api/verify-all/route.ts around lines 79-84 (and similarly 87-91), remove
or replace the console.log that prints the full verifyData/responseBody JSON
(which may contain sensitive GitHub Authorization headers/PATs); instead log
only high-level, non-sensitive metadata such as repo owner/name, filenames or
counts, and strip any headers or auth fields before logging. Ensure any helper
that extracts repo info runs before logging and only sanitized fields are
emitted to logs.
1 participant
fixes issue with repos that have a lot contributors (pagination)