fix external VCS deployment flow #10555
Conversation
📝 WalkthroughWalkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/controllers/api/vcs.php (2)
1621-1626: Repository scoping check removed — reintroduce project/installation guard.Fetching the repository by ID without verifying it belongs to the current project/installation (and using Authorization::skip) is an authorization footgun. A caller with a valid repo ID could affect another project’s repo.
Apply this diff to enforce scoping:
- $repository = Authorization::skip(fn () => $dbForPlatform->getDocument('repositories', $repositoryId)); - - if ($repository->isEmpty()) { - throw new Exception(Exception::REPOSITORY_NOT_FOUND); - } + $repository = Authorization::skip(fn () => $dbForPlatform->getDocument('repositories', $repositoryId)); + + if ( + $repository->isEmpty() + || $repository->getAttribute('projectInternalId') !== $project->getSequence() + || $repository->getAttribute('installationInternalId') !== $installation->getSequence() + ) { + throw new Exception(Exception::REPOSITORY_NOT_FOUND); + }
1396-1399: Gracefully handle commit fetch failures in PR webhook path.A missing commit (e.g., forked PR) will throw and fail the webhook. Degrade to empty details.
Apply this diff:
- $commitDetails = $github->getCommit($providerRepositoryOwner, $providerRepositoryName, $providerCommitHash); + try { + $commitDetails = $github->getCommit($providerRepositoryOwner, $providerRepositoryName, $providerCommitHash); + } catch (\Throwable $e) { + $commitDetails = []; + }
🧹 Nitpick comments (2)
app/controllers/api/vcs.php (2)
1659-1661: Avoid Undefined offset notices when parsing branch; use head.ref and URL-encode.Parsing label via explode risks notices; GitHub payload includes head.ref.
Apply this diff:
- $providerBranch = \explode(':', $pullRequestResponse['head']['label'])[1] ?? ''; - $providerBranchUrl = "$providerRepositoryUrl/tree/$providerBranch"; + $providerBranch = $pullRequestResponse['head']['ref'] ?? ''; + $providerBranchUrl = sprintf('%s/tree/%s', $providerRepositoryUrl, \rawurlencode($providerBranch));
160-171: Don’t mutate the lock key used for cleanup.After updateComment, $latestCommentId is reassigned; deleting the lock with a mutated value risks leaving stale locks. Keep a stable $lockId.
Apply this diff in both lock sections:
- if ($lockAcquired) { + if ($lockAcquired) { + $lockId = $latestCommentId; // Wrap in try/finally to ensure lock file gets deleted try { $comment = new Comment(); $comment->parseComment($github->getComment($owner, $repositoryName, $latestCommentId)); $comment->addBuild($project, $resource, $resourceType, $commentStatus, $deploymentId, $action, ''); $latestCommentId = \strval($github->updateComment($owner, $repositoryName, $latestCommentId, $comment->generateComment())); } finally { - $dbForPlatform->deleteDocument('vcsCommentLocks', $latestCommentId); + $dbForPlatform->deleteDocument('vcsCommentLocks', $lockId); } }And similarly for the branch comments block.
Also applies to: 231-242
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/vcs.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/vcs.php (2)
src/Appwrite/Extend/Exception.php (1)
Exception(7-447)app/realtime.php (1)
getProjectDB(76-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (3)
app/controllers/api/vcs.php (3)
119-119: Setting comment status to 'waiting' is aligned with PR goal.This keeps PR comments in a pending state until authorization completes. LGTM.
262-263: Change commit status to 'pending' for unauthorized externals — good fix.Switching from failure to pending prevents false negatives before authorization. LGTM.
1349-1349: All createGitDeployments calls updated to new signature Verified three call sites in app/controllers/api/vcs.php (lines 1349, 1405, 1669); no other references found.
✨ Benchmark results
⚡ Benchmark Comparison
|
Related task: https://linear.app/appwrite/issue/SER-355/fix-external-vcs-deployments