feat: enhance docs-analysis action security and error handling

- Implement strict whitelist input validation for branch references - Add path traversal detection to sanitize_path function - Replace eval with direct command execution in git_with_retry - Add error tracing with line numbers for better debugging - Add performance monitoring and metrics generation - Update README with security enhancements 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
coder · EdwardAngert · Apr 7, 2025 · Apr 7, 2025 · Apr 7, 2025 · Apr 7, 2025
commit ea543146a971bec3dbcdee24c77cbfa763973ba5
diff --git a/.github/actions/docs-analysis/README.md b/.github/actions/docs-analysis/README.md
@@ -9,6 +9,7 @@ A composite GitHub Action to analyze documentation changes in pull requests and
 - Tracks image modifications with detailed reporting
 - Analyzes document structure (headings, titles)
 - Identifies the most significantly changed files
+- Integrates with other doc workflows (weekly checks, PR previews)
 - Provides standardized outputs that can be used by any workflow
 
 ## Usage
@@ -28,6 +29,28 @@ It only runs on PRs that modify files in the docs directory or markdown files el
     base-ref: 'main'
 ```
 
+### Integration with tj-actions/changed-files (Recommended)
+
+For optimal performance and reliability, we recommend using with `tj-actions/changed-files`:
+
+```yaml
+- uses: tj-actions/changed-files@v45
+  id: changed-files
+  with:
+    files: |
+      docs/**
+      **.md
+    separator: ","
+
+- name: Analyze Documentation Changes
+  id: docs-analysis
+  uses: ./.github/actions/docs-analysis
+  with:
+    docs-path: 'docs/'
+    changed-files: ${{ steps.changed-files.outputs.all_changed_files }}
+    files-pattern: 'docs/**|**.md'
+```
+
 ### Complete Example with Conditionals
 
 ```yaml
@@ -40,13 +63,20 @@ jobs:
         with:
           fetch-depth: 0
 
+      - uses: tj-actions/changed-files@v45
+        id: changed-files
+        with:
+          files: |
+            docs/**
+            **.md
+          separator: ","
+
       - name: Analyze Documentation Changes
         uses: ./.github/actions/docs-analysis
         id: docs-analysis
         with:
           docs-path: 'docs/'
-          pr-ref: ${{ github.event.pull_request.head.ref }}
-          base-ref: 'main'
+          changed-files: ${{ steps.changed-files.outputs.all_changed_files }}
           significant-words-threshold: '100'
           skip-if-no-docs: 'true'
           debug-mode: 'false'
@@ -71,15 +101,18 @@ jobs:
 | Name | Description | Required | Default |
 |------|-------------|----------|---------|
 | `docs-path` | Path to the documentation directory | No | `docs/` |
+| `files-pattern` | Glob pattern(s) for documentation files (use vertical bar \| to separate multiple patterns) | No | `**.md\|docs/**` |
+| `changed-files` | Comma-separated list of changed files (from tj-actions/changed-files) | No | `` |
 | `pr-ref` | PR reference to analyze | No | `github.event.pull_request.head.ref` |
 | `base-ref` | Base reference to compare against | No | `main` |
-| `files-changed` | Comma-separated list of files changed (alternative to git diff) | No | `` |
+| `files-changed` | Comma-separated list of files changed (legacy input, use `changed-files` instead) | No | `` |
 | `max-scan-files` | Maximum number of files to scan | No | `100` |
 | `max-files-to-analyze` | Maximum files to analyze in detail (for performance) | No | `20` |
 | `throttle-large-repos` | Enable throttling for large repositories | No | `true` |
 | `significant-words-threshold` | Threshold for significant text changes | No | `100` |
 | `skip-if-no-docs` | Whether to skip if no docs files are changed | No | `true` |
 | `debug-mode` | Enable verbose debugging output | No | `false` |
+| `use-changed-files-action` | Whether to use tj-actions/changed-files instead of git commands | No | `false` |
 
 ## Outputs
 
@@ -107,12 +140,14 @@ jobs:
 
 ## Security Features
 
-- Input validation to prevent command injection
-- Path sanitization for safer file operations
-- Git command retry logic for improved reliability
+- Stronger input validation with whitelist approach for branch references
+- Enhanced path sanitization with traversal detection
+- Secure command execution (no eval) in git retry operations
+- Error tracing with line numbers for better debugging
 - Cross-platform compatibility with fallbacks
 - Repository size detection with adaptive throttling
 - Python integration for safer JSON handling (with bash fallbacks)
+- Performance monitoring with execution metrics
 
 ## Performance Optimization
 
@@ -155,4 +190,63 @@ jobs:
   with:
     docs-path: 'docs/'
     debug-mode: 'true'
-```
+```
+
+## Unified Documentation Workflows
+
+This action is designed to work seamlessly with Coder's other documentation-related workflows:
+
+### How to Use with docs-ci.yaml
+
+The `docs-ci.yaml` workflow uses this action to analyze documentation changes for linting and formatting:
+
+```yaml
+# From .github/workflows/docs-ci.yaml
+- uses: tj-actions/changed-files@v45
+  id: changed-files
+  with:
+    files: |
+      docs/**
+      **.md
+    separator: ","
+
+- name: Analyze documentation changes
+  id: docs-analysis
+  uses: ./.github/actions/docs-analysis
+  with:
+    docs-path: "docs/"
+    changed-files: ${{ steps.changed-files.outputs.all_changed_files }}
+    files-pattern: "docs/**|**.md"
+```
+
+### How to Use with docs-preview-link.yml
+
+This action can be used in the `docs-preview-link.yml` workflow to analyze documentation changes for preview generation:
+
+```yaml
+# Example integration with docs-preview-link.yml
+- name: Analyze documentation changes
+  id: docs-analysis
+  uses: ./.github/actions/docs-analysis
+  with:
+    docs-path: "docs/"
+    pr-ref: ${{ steps.pr_info.outputs.branch_name }}
+    base-ref: 'main'
+```
+
+### How to Use with weekly-docs.yaml
+
+This action can be used to enhance the weekly documentation checks:
+
+```yaml
+# Example integration with weekly-docs.yaml
+- name: Analyze documentation structure
+  id: docs-analysis
+  uses: ./.github/actions/docs-analysis
+  with:
+    docs-path: "docs/"
+    files-pattern: "docs/**"
+    max-scan-files: "500"  # Higher limit for full repo scan
+```
+
+By using this shared action across all documentation workflows, you ensure consistent analysis, metrics, and reporting for all documentation-related tasks.
diff --git a/.github/actions/docs-analysis/action.yml b/.github/actions/docs-analysis/action.yml
@@ -146,14 +146,14 @@ runs:
           echo "::warning::Documentation path '${{ inputs.docs-path }}' does not exist - some functions may not work correctly"
         fi
 
-        # Validate branch references for command injection prevention
-        if [[ "${{ inputs.pr-ref }}" =~ [;&|$"'`] ]]; then
-          echo "::error::Invalid characters in pr-ref"
+        # Validate branch references with strict whitelist approach for better security
+        if [[ ! "${{ inputs.pr-ref }}" =~ ^[a-zA-Z0-9_\-\.\/]+$ ]]; then
+          echo "::error::Invalid characters in pr-ref - only alphanumeric, underscore, hyphen, dot, and forward slash are allowed"
           exit 1
         fi
 
-        if [[ "${{ inputs.base-ref }}" =~ [;&|$"'`] ]]; then
-          echo "::error::Invalid characters in base-ref"
+        if [[ ! "${{ inputs.base-ref }}" =~ ^[a-zA-Z0-9_\-\.\/]+$ ]]; then
+          echo "::error::Invalid characters in base-ref - only alphanumeric, underscore, hyphen, dot, and forward slash are allowed"
           exit 1
         fi
 
@@ -224,25 +224,37 @@ runs:
       id: verify
       shell: bash
       run: |
+        # Add error tracing for better debugging and recovery
+        trap 'echo "::error::Error occurred in verify docs changes at line $LINENO"' ERR
         # Helper functions for better error handling and path sanitization
         function handle_error() {
           echo "::error::$1"
           echo "docs_changed=false" >> $GITHUB_OUTPUT
           exit 1
         }
 
+        # More secure path sanitization with validation
         function sanitize_path() {
-          echo "$1" | sed 's/[;&|"`$]/\\&/g'
+          local path="$1"
+
+          # Check for path traversal attempts or absolute paths if needed
+          if [[ "$path" == *".."* || "$path" == "/"* ]]; then
+            echo "::error::Invalid path containing directory traversal patterns or absolute reference"
+            return 1
+          fi
+
+          # Sanitize the path - escape special characters
+          echo "$path" | sed 's/[;&|"`$]/\\&/g'
         }
 
         # Retry function for git operations to handle potential rate limiting
+        # Uses direct command execution instead of eval for better security
         function git_with_retry() {
           local max_retries=3
-          local cmd="$@"
           local retry_count=0
 
           while [[ $retry_count -lt $max_retries ]]; do
-            if eval "$cmd"; then
+            if "$@"; then  # Direct execution instead of eval
               return 0
             fi
 
@@ -251,7 +263,7 @@ runs:
             sleep $((retry_count * 2))
           done
 
-          echo "::warning::Git operation failed after $max_retries retries: $cmd"
+          echo "::warning::Git operation failed after $max_retries retries"
           return 1
         }
 
@@ -503,9 +515,22 @@ runs:
           set -x
         fi
 
+        # Add error tracing for better debugging and recovery
+        trap 'echo "::error::Error occurred in document structure analysis at line $LINENO"' ERR
+
         # Helper functions
+        # More secure path sanitization with validation
         function sanitize_path() {
-          echo "$1" | sed 's/[;&|"`$]/\\&/g'
+          local path="$1"
+
+          # Check for path traversal attempts or absolute paths if needed
+          if [[ "$path" == *".."* || "$path" == "/"* ]]; then
+            echo "::error::Invalid path containing directory traversal patterns or absolute reference"
+            return 1
+          fi
+
+          # Sanitize the path - escape special characters
+          echo "$path" | sed 's/[;&|"`$]/\\&/g'
         }
 
         function json_escape() {
@@ -656,9 +681,22 @@ print(json.dumps(doc_structure))
       if: steps.verify.outputs.docs_changed == 'true'
       shell: bash
       run: |
+        # Add error tracing for better debugging and recovery
+        trap 'echo "::error::Error occurred in finding changed files at line $LINENO"' ERR
+
         # Helper functions
+        # More secure path sanitization with validation
         function sanitize_path() {
-          echo "$1" | sed 's/[;&|"`$]/\\&/g'
+          local path="$1"
+
+          # Check for path traversal attempts or absolute paths if needed
+          if [[ "$path" == *".."* || "$path" == "/"* ]]; then
+            echo "::error::Invalid path containing directory traversal patterns or absolute reference"
+            return 1
+          fi
+
+          # Sanitize the path - escape special characters
+          echo "$path" | sed 's/[;&|"`$]/\\&/g'
         }
 
         # Enable debug output if requested
@@ -815,4 +853,22 @@ print(json.dumps(doc_structure))
         START_TIME="${{ steps.timing.outputs.start_time }}"
         DURATION=$((END_TIME - START_TIME))
         echo "duration=$DURATION" >> $GITHUB_OUTPUT
-        echo "Docs analysis completed in ${DURATION}s"
+
+        # Output for CI monitoring systems
+        if [[ $DURATION -gt 30 ]]; then
+          echo "::warning::Docs analysis took ${DURATION}s to complete - consider optimizing"
+        else
+          echo "::notice::Docs analysis completed in ${DURATION}s"
+        fi
+
+        # Create execution metrics JSON for potential monitoring integration
+        mkdir -p .github/temp
+        cat > .github/temp/docs-analysis-metrics.json << EOF
+        {
+          "execution_time": $DURATION,
+          "timestamp": "$(date -u +"%Y-%m-%dT%H:%M:%SZ")",
+          "repository": "${{ github.repository }}",
+          "workflow": "${{ github.workflow }}",
+          "action": "docs-analysis"
+        }
+        EOF