Skip to content

Add a reason field before PAM account access

d72335b
Select commit
Loading
Failed to load commit list.
Open

feat: add a reason field before PAM account access #189

Add a reason field before PAM account access
d72335b
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Apr 20, 2026 in 8m 14s

Code review found 3 important issues

Found 6 candidates, confirmed 3. See review comments for details.

Details

Severity Count
πŸ”΄ Important 3
🟑 Nit 0
🟣 Pre-existing 0
Severity File:Line Issue
πŸ”΄ Important packages/pam/local/base-proxy.go:44-50 Reason not included in approval request payload
πŸ”΄ Important packages/pam/local/base-proxy.go:347-369 MFA flow bypassed after reason-required retry
πŸ”΄ Important packages/cmd/pam.go:13-26 Double reason prompt when account policy requires reason

Annotations

Check failure on line 50 in packages/pam/local/base-proxy.go

See this annotation in the file changed.

@claude claude / Claude Code Review

Reason not included in approval request payload

The `Reason` field is not propagated to the approval request payload. When an account is protected by an approval policy, `HandleApprovalWorkflow` calls `accessParams.ToApprovalRequestData(durationStr)`, which omits the user's stated reason entirely β€” approvers and audit logs for the approval workflow will never see why access was requested, defeating a core purpose of the new feature.

Check failure on line 369 in packages/pam/local/base-proxy.go

See this annotation in the file changed.

@claude claude / Claude Code Review

MFA flow bypassed after reason-required retry

The MFA flow is bypassed when an account requires both a reason and MFA. In `CallPAMAccessWithMFA`, after the user provides a reason (PAM_REASON_REQUIRED branch), the retry calls `api.CallPAMAccess` directly and returns any error raw β€” so if the server then responds with SESSION_MFA_REQUIRED, the CLI returns that error to the user instead of completing the MFA flow. Any account that enforces both a reason policy and MFA will be inaccessible via the interactive prompt flow; the fix is to call `Ca

Check failure on line 26 in packages/cmd/pam.go

See this annotation in the file changed.

@claude claude / Claude Code Review

Double reason prompt when account policy requires reason

When a user runs any PAM command interactively without `--reason`, `resolveReason()` eagerly prompts for an optional reason upfront; if the user presses Enter to skip, an empty reason is sent to the API. If the account policy mandates a reason, the server returns `PAM_REASON_REQUIRED` and `CallPAMAccessWithMFA` then prompts a second time with a mandatory "Reason for access" prompt β€” the user is asked the same question twice with no explanation for why the first answer was insufficient.