Use fresh GitHub token permissions for action installs#419
Conversation
vu1nz Security Review1 finding(s) in PR #419 LOW: 1 Findings
Full AI AnalysisLooking at this diff, I can see changes to a GitHub App installation route that handles permission checking and database updates. Let me analyze each change for security issues: Security AnalysisAfter reviewing the code changes, I found one potential security issue:
Detailed AnalysisThe identified issue:
Other observations (not security issues):
Positive security aspects:
The overall security posture of this code appears reasonable, with only minor concerns around database operation safety. |
Greptile SummaryThis PR fixes stale-cache blocking of action installs by minting the GitHub installation token first and using the fresh permissions returned in the token response, rather than relying solely on the cached
Confidence Score: 4/5The core logic correctly solves the stale-cache problem and the fallback in hasFreshWorkflowWrite is sound; two non-blocking quality issues remain. The fresh-token approach works correctly for the happy path and the denied path. The Supabase update silently drops errors, so the refresh stored permissions goal can fail without any observability. The permission guard also now sits after renderPack and a GitHub API call, adding latency on every denied retry for workflow-write actions. sites/sh1pt.com/app/api/v1/github/installations/[id]/repos/[repoId]/actions/[actionId]/install/route.ts — specifically the unguarded Supabase update and the ordering of renderPack relative to the permission check. Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Route as install/route.ts
participant GitHub as GitHub API
participant DB as Supabase DB
Client->>Route: POST /install
Route->>DB: authorizeInstallation (loads cached permissions)
DB-->>Route: auth.installation (with stored permissions)
Route->>Route: loadBuiltinPacks / compatibility check
Route->>Route: renderPack (now before permission check)
Route->>GitHub: mintInstallationToken
GitHub-->>Route: token + fresh permissions
alt token.data.permissions present
Route->>DB: UPDATE github_installations (refresh permissions cache)
DB-->>Route: (result ignored)
end
Route->>Route: hasFreshWorkflowWrite(tokenPermissions, storedPermissions)
alt requires workflow:write AND not granted
Route-->>Client: 403 Forbidden
else permission OK
Route->>GitHub: openPackPullRequest
GitHub-->>Route: outcome
Route-->>Client: 200 / 409 / 500
end
Reviews (1): Last reviewed commit: "Use fresh GitHub token permissions for a..." | Re-trigger Greptile |
| if (token.data.permissions) { | ||
| await admin | ||
| .from('github_installations') | ||
| .update({ permissions: token.data.permissions, updated_at: new Date().toISOString() }) | ||
| .eq('id', auth.installation.id); | ||
| } |
There was a problem hiding this comment.
Supabase update error silently swallowed
The update() call's result is awaited but never destructured for { error }. If the row update fails (network hiccup, RLS policy, wrong primary key type), the failure is invisible — no log, no side-effect on the current request. The PR explicitly lists "refresh stored installation permissions from the token response" as a goal, so a silent failure here defeats that goal without any signal for debugging.
| @@ -102,6 +93,24 @@ export async function POST( | |||
| if (!token.ok || !token.data) { | |||
| return NextResponse.json({ error: token.error ?? 'Could not mint installation token' }, { status: token.status || 500 }); | |||
| } | |||
| if (token.data.permissions) { | |||
| await admin | |||
| .from('github_installations') | |||
| .update({ permissions: token.data.permissions, updated_at: new Date().toISOString() }) | |||
| .eq('id', auth.installation.id); | |||
| } | |||
| if ( | |||
| requiresWorkflowWrite(entry.manifest.files) && | |||
| !hasFreshWorkflowWrite(token.data.permissions, auth.installation.permissions) | |||
| ) { | |||
| return NextResponse.json( | |||
| { | |||
| error: | |||
| 'GitHub App needs Workflows: write permission to install actions into .github/workflows. Update the sh1pt GitHub App permissions, accept the installation update in GitHub, then retry.', | |||
| }, | |||
| { status: 403 }, | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
renderPack and token minting now precede the permission guard
In the previous code, a missing workflow:write permission produced an early 403 before renderPack or mintInstallationToken were called. Now both are invoked unconditionally, so a denied request burns a GitHub API token-mint call and runs the pack renderer for nothing. For actions that write to .github/workflows/ and are frequently attempted before the org accepts the new permission, this adds avoidable latency and GitHub API quota consumption on each retry.
Summary
Verification
Why
After an org accepts the new Workflows permission, the existing cached
github_installations.permissionsrow can still say the app lacks workflow write access. The newly minted installation token reflects GitHub's current permission grant, so this prevents stale cache from blocking installs.