Skip to content

Use fresh GitHub token permissions for action installs#419

Merged
ralyodio merged 1 commit into
masterfrom
fix/use-fresh-github-token-permissions
May 23, 2026
Merged

Use fresh GitHub token permissions for action installs#419
ralyodio merged 1 commit into
masterfrom
fix/use-fresh-github-token-permissions

Conversation

@ralyodio
Copy link
Copy Markdown
Contributor

Summary

  • mint the GitHub installation token before enforcing workflow-write permission
  • use the token's fresh permissions instead of only the cached Supabase row
  • refresh stored installation permissions from the token response

Verification

  • pnpm --filter sh1pt-dot-com build

Why

After an org accepts the new Workflows permission, the existing cached github_installations.permissions row 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.

@github-actions
Copy link
Copy Markdown

vu1nz Security Review

1 finding(s) in PR #419

LOW: 1

Findings

Severity File Issue Suggestion
LOW route.ts SQL injection potential in database update Use parameterized queries and validate auth.installation.id is properly sanitized. Consider using a transaction and add error handling for the datab
Full AI Analysis

Looking 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 Analysis

After reviewing the code changes, I found one potential security issue:

Severity File Line Issue Suggestion
Low route.ts 96-100 SQL injection potential in database update Use parameterized queries and validate auth.installation.id is properly sanitized. Consider using a transaction and add error handling for the database update operation.

Detailed Analysis

The identified issue:

  • The code updates the database using auth.installation.id directly in the WHERE clause
  • While this appears to go through an ORM/query builder (.eq() method), it's important to ensure the auth.installation.id value is properly validated and sanitized

Other observations (not security issues):

  • The permission checking logic has been moved to occur after token minting, which is actually a good security practice as it uses fresh token permissions
  • The hasFreshWorkflowWrite function provides a fallback mechanism that maintains security
  • Input validation through normalizeInputs appears to be present
  • No hardcoded secrets, XSS, RCE, or other major vulnerabilities detected
  • The authentication flow appears to be properly implemented

Positive security aspects:

  • Permission checks are still enforced, just with fresher data
  • Database permissions are updated to maintain current state
  • Error handling maintains appropriate HTTP status codes
  • No sensitive data exposure in error messages

The overall security posture of this code appears reasonable, with only minor concerns around database operation safety.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 23, 2026

Greptile Summary

This 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 github_installations.permissions row. It also back-fills the DB row with the fresh permissions and adds a hasFreshWorkflowWrite helper that falls back to stored permissions only when the token carries no permissions data.

  • The workflow-write permission check is now correctly anchored to fresh token permissions, with the DB row updated as a side effect before the guard runs.
  • renderPack and mintInstallationToken are now called unconditionally before the permission guard, where previously a stale-cache denial would have exited before either ran.
  • The Supabase update() result is not inspected, so any DB update failure is silently ignored.

Confidence Score: 4/5

The 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

Filename Overview
sites/sh1pt.com/app/api/v1/github/installations/[id]/repos/[repoId]/actions/[actionId]/install/route.ts Moves workflow-write permission check to after token minting, adds DB cache refresh and hasFreshWorkflowWrite helper; Supabase update error is silently ignored, and renderPack/token mint now run unconditionally before the guard.

Sequence Diagram

sequenceDiagram
    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
Loading

Fix All in Codex Fix All in Claude Code

Reviews (1): Last reviewed commit: "Use fresh GitHub token permissions for a..." | Re-trigger Greptile

Comment on lines +96 to +101
if (token.data.permissions) {
await admin
.from('github_installations')
.update({ permissions: token.data.permissions, updated_at: new Date().toISOString() })
.eq('id', auth.installation.id);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Fix in Codex Fix in Claude Code

Comment on lines 78 to +113
@@ -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 },
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

Fix in Codex Fix in Claude Code

@ralyodio ralyodio merged commit 41a673c into master May 23, 2026
7 checks passed
@ralyodio ralyodio deleted the fix/use-fresh-github-token-permissions branch May 23, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant