Secret Approval Request: check if user is a reviewer to trigger hasSecretReadValueOrDescribePermission

[carlosmonastyrski]

Description 📣

This PR fixes the scenario where users without read access to secrets and that are not reviewers could see secret values when a secret change request was created. From now on only reviewers will always be able to see the values of the before and after, but if the user is not a reviewer we will check for secret value read permissions.

Type ✨

• Bug fix
• New feature
• Improvement
• Breaking change
• Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

---

• I have read the contributing guide, agreed and acknowledged the code of conduct. 📝

[maidul98]

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

[greptile-apps]

Greptile Overview

Summary

This PR addresses a critical security vulnerability in the secret approval request system where users without read access to secrets could still view secret values if they had access to the approval request.

Key Changes:

• Modified getHasSecretReadAccess function to prioritize reviewer status check
• Reviewers can always see secret values (maintains existing behavior for legitimate reviewers)
• Non-reviewers now must pass standard secret read permission checks
• Removed dependency on shouldCheckSecretPermission policy flag
• Simplified function signature by removing unused parameter

Security Impact:

• Prevents unauthorized secret exposure to non-reviewer users
• Maintains proper access control for secret approval workflows
• Ensures consistent permission enforcement across all secret access paths

Confidence Score: 5/5

• This PR is safe to merge and fixes a critical security vulnerability
• Score reflects a well-implemented security fix that properly addresses unauthorized access without breaking existing reviewer functionality. The change is focused, logical, and improves the security posture significantly.
• No files require special attention - the single change is well-implemented

Important Files Changed

File Analysis

Filename	Score	Overview
backend/src/ee/services/secret-approval-request/secret-approval-request-service.ts	5/5	Fixed security vulnerability where non-reviewers could see secret values by adding reviewer check before permission validation

Sequence Diagram

sequenceDiagram
    participant User as User
    participant API as getSecretApprovalDetails
    participant Permission as Permission Service
    participant Policy as Approval Policy
    
    User->>API: Request secret approval details
    API->>Policy: Check if user is reviewer
    
    alt User is a reviewer
        Policy-->>API: User is reviewer
        API-->>User: Return secrets with values visible
    else User is NOT a reviewer
        Policy-->>API: User is not reviewer
        API->>Permission: Check secret read permissions
        
        alt User has read permissions
            Permission-->>API: User has read access
            API-->>User: Return secrets with values visible
        else User lacks read permissions
            Permission-->>API: User lacks read access
            API-->>User: Return secrets with values hidden (masked)
        end
    end

Loading

_{1 file reviewed, 1 comment}

_{Edit Code Review Bot Settings | Greptile}

[scott-ray-wilson]

LGTM!