Skip to content

Conversation

@sheensantoscapadngan
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan commented Nov 19, 2025

Description 📣

This PR adds MFA handling support for PAM account access, enabling secure multi-factor authentication when users connect to SSH or database proxies. When the API returns a SESSION_MFA_REQUIRED error, the CLI opens a browser for MFA verification and polls the session status until completion.

Type ✨

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

Tests 🛠️

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

@sheensantoscapadngan sheensantoscapadngan marked this pull request as ready for review November 21, 2025 15:39
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 21, 2025

Greptile Overview

Greptile Summary

This PR adds MFA handling support for PAM account access, enabling secure multi-factor authentication when users connect to SSH or database proxies. When the API returns a SESSION_MFA_REQUIRED error, the CLI opens a browser for MFA verification and polls the session status until completion.

Key Changes

  • Added CallGetMFASessionStatus API endpoint to poll MFA session verification status
  • Extended APIError with Name field to identify specific error types like SESSION_MFA_REQUIRED
  • Created CallPAMAccessWithMFA wrapper that detects MFA requirements and handles the verification flow
  • Added HandleMFASession utility to open browser and poll for MFA completion with 5-minute timeout
  • Updated both database and SSH proxies to use the new MFA-aware access function

Issues Found

  • URL Manipulation Vulnerability: The mfaSessionId from API error response is inserted directly into browser URL without validation, allowing path traversal if malicious value is returned
  • Error Parsing Bug: In errors.go:145-147, when JSON unmarshal fails, the code incorrectly returns errorResponse.Name which will be empty since the struct wasn't populated

Confidence Score: 3/5

  • This PR has security vulnerabilities that should be addressed before merging
  • Score reflects two critical issues: URL manipulation vulnerability in MFA session handling allows potential redirection attacks, and error parsing bug that could cause incorrect error handling behavior
  • Pay close attention to packages/util/helper.go (URL manipulation vulnerability) and packages/api/errors.go (error parsing bug)

Important Files Changed

File Analysis

Filename Score Overview
packages/api/errors.go 5/5 Added Name field to APIError to capture error type from API responses for MFA error handling
packages/pam/local/base-proxy.go 4/5 Implemented CallPAMAccessWithMFA to handle MFA challenges, retry PAM access after verification, includes MFA session polling
packages/util/helper.go 4/5 Added HandleMFASession to open browser for MFA verification and poll status, plus OpenBrowser utility function

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@sheensantoscapadngan sheensantoscapadngan merged commit 9154105 into main Jan 6, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants