Fix Danger.js Security Issues (#4462)

2026-04-24 16:29:45 +00:00 · 2025-07-16 15:15:05 -04:00
parent 1200c07fcd
commit 927773f553
6 changed files with 311 additions and 898 deletions
--- a/.github/workflows/danger.yaml
+++ b/.github/workflows/danger.yaml
@@ -1,27 +0,0 @@
-name: Danger PR Review
-on:
-  pull_request:
-    types: [opened, edited, synchronize]
-
-jobs:
-  danger:
-    runs-on: ubuntu-latest
-    steps:
-      - name: Checkout code
-        uses: actions/checkout@v4
-        with:
-          fetch-depth: 0
-
-      - name: Setup Node.js
-        uses: actions/setup-node@v4
-        with:
-          node-version: 'lts/*'
-
-      - name: Install dependencies
-        run: npm ci
-
-      - name: Run Danger
-        run: npx danger ci
-        env:
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-          DANGER_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
--- a/.github/workflows/pr-checks.yml
+++ b/.github/workflows/pr-checks.yml
@@ -0,0 +1,154 @@
+name: PR Checks
+on:
+  pull_request:
+    types: [opened, edited, synchronize, reopened]
+
+permissions:
+  contents: read
+  pull-requests: read
+
+jobs:
+  analyze:
+    runs-on: ubuntu-latest
+    outputs:
+      should_run: ${{ steps.check-changes.outputs.should_run }}
+      has_browser_tests: ${{ steps.check-coverage.outputs.has_browser_tests }}
+      has_screen_recording: ${{ steps.check-recording.outputs.has_recording }}
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+
+      - name: Ensure base branch is available
+        run: |
+          # Fetch the specific base commit to ensure it's available for git diff
+          git fetch origin ${{ github.event.pull_request.base.sha }}
+
+      - name: Check if significant changes exist
+        id: check-changes
+        run: |
+          # Get list of changed files
+          CHANGED_FILES=$(git diff --name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }})
+          
+          # Filter for src/ files
+          SRC_FILES=$(echo "$CHANGED_FILES" | grep '^src/' || true)
+          
+          if [ -z "$SRC_FILES" ]; then
+            echo "No src/ files changed"
+            echo "should_run=false" >> "$GITHUB_OUTPUT"
+            exit 0
+          fi
+          
+          # Count lines changed in src files
+          TOTAL_LINES=0
+          for file in $SRC_FILES; do
+            if [ -f "$file" ]; then
+              # Count added lines (non-empty)
+              ADDED=$(git diff ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} -- "$file" | grep '^+' | grep -v '^+++' | grep -v '^+$' | wc -l)
+              # Count removed lines (non-empty)
+              REMOVED=$(git diff ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} -- "$file" | grep '^-' | grep -v '^---' | grep -v '^-$' | wc -l)
+              TOTAL_LINES=$((TOTAL_LINES + ADDED + REMOVED))
+            fi
+          done
+          
+          echo "Total lines changed in src/: $TOTAL_LINES"
+          
+          if [ $TOTAL_LINES -gt 3 ]; then
+            echo "should_run=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "should_run=false" >> "$GITHUB_OUTPUT"
+          fi
+
+      - name: Check browser test coverage
+        id: check-coverage
+        if: steps.check-changes.outputs.should_run == 'true'
+        run: |
+          # Check if browser tests were updated
+          BROWSER_TEST_CHANGES=$(git diff --name-only ${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }} | grep '^browser_tests/.*\.ts$' || true)
+          
+          if [ -n "$BROWSER_TEST_CHANGES" ]; then
+            echo "has_browser_tests=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "has_browser_tests=false" >> "$GITHUB_OUTPUT"
+          fi
+
+      - name: Check for screen recording
+        id: check-recording
+        if: steps.check-changes.outputs.should_run == 'true'
+        run: |
+          # Check PR body for screen recording
+          PR_BODY="${{ github.event.pull_request.body }}"
+          
+          # Check for GitHub user attachments or YouTube links
+          if echo "$PR_BODY" | grep -qiE 'github\.com/user-attachments/assets/[a-f0-9-]+|youtube\.com/watch|youtu\.be/'; then
+            echo "has_recording=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "has_recording=false" >> "$GITHUB_OUTPUT"
+          fi
+
+      - name: Final check and create results
+        id: final-check
+        if: always()
+        run: |
+          # Initialize results
+          WARNINGS_JSON=""
+          
+          # Only run checks if should_run is true
+          if [ "${{ steps.check-changes.outputs.should_run }}" == "true" ]; then
+            # Check browser test coverage
+            if [ "${{ steps.check-coverage.outputs.has_browser_tests }}" != "true" ]; then
+              if [ -n "$WARNINGS_JSON" ]; then
+                WARNINGS_JSON="${WARNINGS_JSON},"
+              fi
+              WARNINGS_JSON="${WARNINGS_JSON}{\"message\":\"⚠️ **Warning: E2E Test Coverage Missing**\\n\\nIf this PR modifies behavior that can be covered by browser-based E2E tests, those tests are required. PRs lacking applicable test coverage may not be reviewed until added. Please add or update browser tests to ensure code quality and prevent regressions.\"}"
+            fi
+            
+            # Check screen recording
+            if [ "${{ steps.check-recording.outputs.has_recording }}" != "true" ]; then
+              if [ -n "$WARNINGS_JSON" ]; then
+                WARNINGS_JSON="${WARNINGS_JSON},"
+              fi
+              WARNINGS_JSON="${WARNINGS_JSON}{\"message\":\"⚠️ **Warning: Visual Documentation Missing**\\n\\nIf this PR changes user-facing behavior, visual proof (screen recording or screenshot) is required. PRs without applicable visual documentation may not be reviewed until provided.\\nYou can add it by:\\n\\n- GitHub: Drag & drop media directly into the PR description\\n\\n- YouTube: Include a link to a short demo\"}"
+            fi
+          fi
+          
+          # Create results JSON
+          if [ -n "$WARNINGS_JSON" ]; then
+            # Create JSON with warnings
+            cat > pr-check-results.json << EOF
+          {
+            "fails": [],
+            "warnings": [$WARNINGS_JSON],
+            "messages": [],
+            "markdowns": []
+          }
+          EOF
+            echo "failed=false" >> "$GITHUB_OUTPUT"
+          else
+            # Create JSON with success
+            cat > pr-check-results.json << 'EOF'
+          {
+            "fails": [],
+            "warnings": [],
+            "messages": [],
+            "markdowns": []
+          }
+          EOF
+            echo "failed=false" >> "$GITHUB_OUTPUT"
+          fi
+          
+          # Write PR metadata
+          echo "${{ github.event.pull_request.number }}" > pr-number.txt
+          echo "${{ github.event.pull_request.head.sha }}" > pr-sha.txt
+
+      - name: Upload results
+        uses: actions/upload-artifact@v4
+        if: always()
+        with:
+          name: pr-check-results-${{ github.run_id }}
+          path: |
+            pr-check-results.json
+            pr-number.txt
+            pr-sha.txt
+          retention-days: 1
--- a/.github/workflows/pr-comment.yml
+++ b/.github/workflows/pr-comment.yml
@@ -0,0 +1,149 @@
+name: PR Comment
+on:
+  workflow_run:
+    workflows: ["PR Checks"]
+    types: [completed]
+
+permissions:
+  pull-requests: write
+  issues: write
+  statuses: write
+
+jobs:
+  comment:
+    if: github.event.workflow_run.event == 'pull_request'
+    runs-on: ubuntu-latest
+    steps:
+      - name: Download artifacts
+        uses: actions/download-artifact@v4
+        with:
+          name: pr-check-results-${{ github.event.workflow_run.id }}
+          path: /tmp/pr-artifacts
+          github-token: ${{ secrets.GITHUB_TOKEN }}
+          run-id: ${{ github.event.workflow_run.id }}
+          
+      - name: Post results
+        uses: actions/github-script@v7
+        with:
+          script: |
+            const fs = require('fs');
+            const path = require('path');
+            
+            // Helper function to safely read files
+            function safeReadFile(filePath) {
+              try {
+                if (!fs.existsSync(filePath)) return null;
+                return fs.readFileSync(filePath, 'utf8').trim();
+              } catch (e) {
+                console.error(`Error reading ${filePath}:`, e);
+                return null;
+              }
+            }
+            
+            // Read artifact files
+            const artifactDir = '/tmp/pr-artifacts';
+            const prNumber = safeReadFile(path.join(artifactDir, 'pr-number.txt'));
+            const prSha = safeReadFile(path.join(artifactDir, 'pr-sha.txt'));
+            const resultsJson = safeReadFile(path.join(artifactDir, 'pr-check-results.json'));
+            
+            // Validate PR number
+            if (!prNumber || isNaN(parseInt(prNumber))) {
+              throw new Error('Invalid or missing PR number');
+            }
+            
+            // Parse and validate results
+            let results;
+            try {
+              results = JSON.parse(resultsJson || '{}');
+            } catch (e) {
+              console.error('Failed to parse check results:', e);
+              
+              // Post error comment
+              await github.rest.issues.createComment({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                issue_number: parseInt(prNumber),
+                body: `⚠️ PR checks failed to complete properly. Error parsing results: ${e.message}`
+              });
+              return;
+            }
+            
+            // Format check messages
+            const messages = [];
+            
+            if (results.fails && results.fails.length > 0) {
+              messages.push('### ❌ Failures\n' + results.fails.map(f => f.message).join('\n\n'));
+            }
+            
+            if (results.warnings && results.warnings.length > 0) {
+              messages.push('### ⚠️ Warnings\n' + results.warnings.map(w => w.message).join('\n\n'));
+            }
+            
+            if (results.messages && results.messages.length > 0) {
+              messages.push('### 💬 Messages\n' + results.messages.map(m => m.message).join('\n\n'));
+            }
+            
+            if (results.markdowns && results.markdowns.length > 0) {
+              messages.push(...results.markdowns.map(m => m.message));
+            }
+            
+            // Find existing bot comment
+            const comments = await github.rest.issues.listComments({
+              owner: context.repo.owner,
+              repo: context.repo.repo,
+              issue_number: parseInt(prNumber)
+            });
+            
+            const botComment = comments.data.find(comment => 
+              comment.user.type === 'Bot' && 
+              comment.body.includes('<!-- pr-checks-comment -->')
+            );
+            
+            // Post comment if there are any messages
+            if (messages.length > 0) {
+              const body = messages.join('\n\n');
+              const commentBody = `<!-- pr-checks-comment -->\n${body}`;
+              
+              if (botComment) {
+                await github.rest.issues.updateComment({
+                  owner: context.repo.owner,
+                  repo: context.repo.repo,
+                  comment_id: botComment.id,
+                  body: commentBody
+                });
+              } else {
+                await github.rest.issues.createComment({
+                  owner: context.repo.owner,
+                  repo: context.repo.repo,
+                  issue_number: parseInt(prNumber),
+                  body: commentBody
+                });
+              }
+            } else {
+              // No messages - delete existing comment if present
+              if (botComment) {
+                await github.rest.issues.deleteComment({
+                  owner: context.repo.owner,
+                  repo: context.repo.repo,
+                  comment_id: botComment.id
+                });
+              }
+            }
+            
+            // Set commit status based on failures
+            if (prSha) {
+              const hasFailures = results.fails && results.fails.length > 0;
+              const hasWarnings = results.warnings && results.warnings.length > 0;
+              await github.rest.repos.createCommitStatus({
+                owner: context.repo.owner,
+                repo: context.repo.repo,
+                sha: prSha,
+                state: hasFailures ? 'failure' : 'success',
+                context: 'pr-checks',
+                description: hasFailures 
+                  ? `${results.fails.length} check(s) failed` 
+                  : hasWarnings 
+                    ? `${results.warnings.length} warning(s)` 
+                    : 'All checks passed'
+              });
+            }
--- a/dangerfile.ts
+++ b/dangerfile.ts
@@ -1,72 +0,0 @@
-import { danger, fail } from 'danger'
-
-// Check if we should run the checks
-const shouldRunChecks = async () => {
-  const allChangedFiles = [
-    ...danger.git.modified_files,
-    ...danger.git.created_files
-  ]
-  const srcChanges = allChangedFiles.filter((file) => file.startsWith('src/'))
-
-  if (srcChanges.length === 0) {
-    return false
-  }
-
-  // Check total lines changed in src files
-  let totalLinesChanged = 0
-  for (const file of srcChanges) {
-    const diff = await danger.git.diffForFile(file)
-    if (diff) {
-      // Count only lines with actual content (non-empty after trimming whitespace)
-      // This excludes empty lines and lines containing only spaces/tabs
-      const additions =
-        diff.added?.split('\n').filter((line) => line.trim()).length || 0
-      const deletions =
-        diff.removed?.split('\n').filter((line) => line.trim()).length || 0
-      totalLinesChanged += additions + deletions
-    }
-  }
-
-  return totalLinesChanged > 3
-}
-
-// Check if browser tests were updated
-const checkBrowserTestCoverage = () => {
-  const allChangedFiles = [
-    ...danger.git.modified_files,
-    ...danger.git.created_files
-  ]
-  const hasBrowserTestChanges = allChangedFiles.some(
-    (file) => file.startsWith('browser_tests/') && file.endsWith('.ts')
-  )
-
-  if (!hasBrowserTestChanges) {
-    fail(`🧪 **E2E Test Coverage Missing**
-
-All changes should be covered under E2E testing. Please add or update browser tests.`)
-  }
-}
-
-// Check for screen recording in PR description
-const checkScreenRecording = () => {
-  const description = danger.github.pr.body || ''
-  const hasRecording =
-    /github\.com\/user-attachments\/assets\/[a-f0-9-]+/i.test(description) ||
-    /youtube\.com\/watch|youtu\.be\//i.test(description)
-
-  if (!hasRecording) {
-    fail(`📹 **Visual Documentation Missing**
-
-Please add a screen recording or screenshot:
- GitHub: Drag & drop media to PR description  
- YouTube: Add YouTube link`)
-  }
-}
-
-// Run the checks only if conditions are met
-shouldRunChecks().then((shouldRun) => {
-  if (shouldRun) {
-    checkBrowserTestCoverage()
-    checkScreenRecording()
-  }
-})
--- a/package-lock.json
+++ b/package-lock.json
--- a/package.json
+++ b/package.json
@@ -45,7 +45,6 @@
    "@vue/test-utils": "^2.4.6",
    "autoprefixer": "^10.4.19",
    "chalk": "^5.3.0",
-    "danger": "^13.0.4",
    "eslint": "^9.12.0",
    "eslint-config-prettier": "^10.1.2",
    "eslint-plugin-prettier": "^5.2.6",