Require workflow permission for action installs#418
Conversation
vu1nz Security Review0 finding(s) in PR #418 No security issues found. Full AI AnalysisLooking at this pull request, I'll analyze each change for potential security vulnerabilities. Analysis SummaryThis PR adds GitHub Workflows write permissions to a GitHub App configuration and implements permission checking before installing actions that write to
Security Findings
Detailed AnalysisNo security vulnerabilities detected. Here's why each change is secure: ✅ Permission Model (Lines 14, 49)
✅ Authorization Checks (Lines 77-84, 118-130)
✅ Input Validation (Lines 140-144)
✅ Database Changes (Lines 19, 62)
✅ Error Handling (Lines 118-130)
Positive Security Aspects
The changes appear to be a security improvement, adding proper authorization checks for a sensitive operation (writing workflow files). |
Greptile SummaryThis PR gates workflow-file action installs behind a
Confidence Score: 4/5Safe to merge; the new permission gate is well-defended with both a pre-flight DB check and a GitHub API fallback, so no regression path is left unguarded. The install route now reads The install route and Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Route as install/route.ts
participant DB as Supabase (github_installations)
participant GH as GitHub API
Client->>Route: POST /install
Route->>DB: authorizeInstallation() SELECT permissions
DB-->>Route: InstallationRow (with permissions)
Route->>Route: requiresWorkflowWrite(manifest.files)?
alt needs workflows:write AND permissions missing
Route-->>Client: 403 - update App permissions and retry
else permission OK (or action has no workflow files)
Route->>GH: openPackPullRequest()
alt GitHub returns 403 Resource not accessible by integration
GH-->>Route: 403
Route-->>Client: 403 - update App permissions and retry
else success / other outcome
GH-->>Route: outcome
Route-->>Client: 200 / 409 / 5xx
end
end
Reviews (1): Last reviewed commit: "Require workflow permission for action i..." | Re-trigger Greptile |
| if (requiresWorkflowWrite(entry.manifest.files) && !hasWorkflowWrite(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.
Pre-flight check depends on DB permissions staying fresh
hasWorkflowWrite reads from the permissions column that was just added to the SELECT in authorizeInstallation. If the webhook handler that updates github_installations doesn't write the permissions JSON when an installation is updated (i.e., when a user accepts the new Workflows: write request in GitHub), a user who has already accepted the permission will still receive a 403 here until the row is resynced. The fallback handler on line 118 catches the inverse case (DB says ✓ but GitHub says ✗), but there's no matching recovery path for the false-negative. Is the permissions column reliably refreshed by the installation webhook? Is the permissions column in github_installations updated by the installation webhook event when a user accepts new GitHub App permissions?
Summary
Workflows: writepermission in the app manifestVerification
Operational note
Existing GitHub App installations must accept the updated
Workflows: writepermission in GitHub before sh1pt can open PRs that add or update.github/workflows/*.yml.