Added verify user action to preview #87
Conversation
…portant actions at the moment
Reviewer's Guide by SourceryThis pull request modifies the deploy-preview workflow to include a user verification step and simplifies the workflow by removing security checks and cleanup steps. Sequence diagram for the simplified preview deployment workflowsequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant Vercel
PR->>GH: PR opened/synchronized
GH->>GH: Verify user in organization
GH->>GH: Checkout repository
GH->>GH: Setup environment
GH->>Vercel: Deploy preview
Vercel-->>GH: Return deployment URL
GH->>GH: Generate App ID
GH->>PR: Post preview URL comment
GH->>GH: Update deployment status
Flow diagram showing the new preview deployment processflowchart TD
A[PR Event] --> B{Verify User}
B -->|User Verified| C[Setup Environment]
C --> D[Deploy to Vercel]
D --> E[Generate App ID]
E --> F[Post Preview URL]
F --> G[Update Status]
B -->|User Not Verified| H[Deployment Failed]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
aliakbar-deriv
had a problem deploying
to
preview
GitHub Actions
Failure
There was a problem hiding this comment.
AI Code Review
🔴 BUGS & LOGICAL ISSUES:
-
Removal of “security-check” job leaves fork PRs unverified.
• Issue Description: Previously, the workflow guarded against malicious changes in forked PRs by checking author permissions, scanning the diff for suspicious patterns, and halting the deployment if the contributor was not authorized. This entire job has been removed, so there is now no gate that prevents untrusted fork code from running privileged steps with write permissions and repository secrets.
• Potential Impacts: Malicious actors could embed dangerous code in a forked PR, gain access to deployment tokens, or alter the workflow environment. This is a serious security hole for public repositories.
• Reproduction Scenarios:
– Open a PR from a fork that modifies scripts or calls dangerous APIs; the workflow would now run without the prior security checks or maintainers’ approval gate.
• Fix Implementation with Code Example (reintroduce or replace security checks):security-check:
name: Security Check (Restored)
runs-on: ubuntu-latest
steps:
- name: Inspect PR source
id: check
uses: actions/github-script@v7
with:
script: |
const pr = context.payload.pull_request;
const isFork = pr.head.repo.full_name !== pr.base.repo.full_name;
// ...existing collaborator checks...
core.setOutput('is-fork', isFork.toString());
// Additional checks# Optionally re-add pattern scanning if needed - name: Scan for suspicious patterns if: steps.check.outputs.is-fork == 'true' run: | # your scanning logic here... echo "Scanning forked PR for malicious patterns..."Then have your main job depend on:
needs: security-check
if: needs.security-check.result == 'success'etc.
-
“job.status” usage lumps canceled and other states as “failure.”
• Issue Description: The final step sets the deployment status description to “❌ Preview deployment failed” for any status other than “success.” That includes canceled, skipped, or neutral runs.
• Potential Impacts: Contributors may be misled into thinking their PR explicitly failed, when it may have been canceled or skipped for unrelated reasons.
• Reproduction Scenarios:
– A PR is merged or closed, causing a cancellation of the job. The status comment incorrectly says “Preview deployment failed.”
• Fix Implementation with Code Example:- name: Update deployment status
if: always()
uses: ./.github/actions/deployment-status
with:
environment: 'preview'
deployment-url: ${{ steps.preview-url.outputs.url }}
sha: ${{ github.event.pull_request.head.sha }}
status: ${{ job.status }}
description: ${{
job.status == 'success' && '✨ Preview deployment completed' ||
job.status == 'cancelled' && '🚫 Preview deployment canceled' ||
'❌ Preview deployment failed'
}}
- name: Update deployment status
🟡 RELIABILITY CONCERNS:
-
Old preview deployments are no longer cleaned up.
• Edge Cases Identified: Over time, stale previews may accumulate in Vercel, leading to wasted resources or confusion.
• Potential Failure Scenarios: If limits/quota on the Vercel account are reached, new previews might fail.
• Mitigation Steps with Code Example:-
Reintroduce the cleanup step or incorporate it in the deploy action:
- name: Cleanup old previews
if: always()
run: |Example, using Vercel’s API to delete old previews
or optionally use a separate action that enumerates existing previews
echo "Cleaning up old preview deployments..."
curl -X DELETE
-H "Authorization: Bearer ${{ secrets.VERCEL_TOKEN }}"
"https://api.vercel.com/v13/deployments/${{ steps.deploy.outputs.deployment-id }}"
- name: Cleanup old previews
-
-
Removal of “needs” chain can cause unguarded parallel execution.
• Potential Failure Scenarios: Without “needs: security-check,” if other steps rely on security gating, they might run prematurely.
• Mitigation Steps: Re-establish dependencies between jobs to ensure security checks (or new “verify_user_in_organization”) complete before deployment.
💡 ROBUSTNESS IMPROVEMENTS:
-
Harden “verify_user_in_organization” usage.
• Error Handling Enhancements: If the user is not in your organization, consider blocking the job or limiting permissions.
• Code Example:-
name: Verify user
id: verify_user
uses: 'deriv-com/shared-actions/.github/actions/verify_user_in_organization@v1'
with:
username: ${{ github.event.pull_request.user.login }}
token: ${{ secrets.PERSONAL_ACCESS_TOKEN }} -
name: Stop job if user is not verified
if: steps.verify_user.outputs.isVerified != 'true'
run: |
echo "User not verified. Stopping the workflow."
exit 1
-
-
Restore scan/policy checks in a separate job for minimal overhead.
• Input Validation Additions: If the intent is to trust organization members but still scan new code, keep a lightweight diff-based check for suspicious changes.
• Code Example:- name: Minimal diff-based security scan
run: |
echo "Scanning patch for suspicious changes..."Simple grep-based checks or a known security scanning tool
- name: Minimal diff-based security scan
-
Explicit Timeout Handling for Deployment Step.
• Current deployment steps have a 10-minute timeout-minutes. Consider fallback logic or re-deployment if the environment is slow.
By reintroducing appropriate checks, refining the final status messaging, and maintaining the cleanup job, you ensure both security and clarity for contributors’ PRs.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @aliakbar-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- While the simplification is appreciated, consider retaining the security checks for fork PRs, especially the pattern scanning for suspicious code. This is important for maintaining repository security.
- The removal of the cleanup step for old preview deployments could lead to resource waste. Consider re-implementing this functionality.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
🔗 Linked Issue
Added verify user action and simplified the workflow by removing unimportant actions at the moment.
❓ Type of Change
📝 Description
🧪 How Has This Been Tested?
📷 Screenshots (if appropriate)
Summary by Sourcery
CI: