fix(security): stop leaking OAuth provider exception messages (#323)#335
fix(security): stop leaking OAuth provider exception messages (#323)#335dmytrocraft wants to merge 4 commits into
Conversation
OAuthExceptionListener built the client problem+json response with `detail => $exception->getMessage()`. For OAuthProviderException that message is `OAuth provider <p> error: <raw>` where <raw> is the verbatim getMessage() of the Guzzle/league exception caught in GitHubOAuthProvider during token exchange / profile fetch. Those messages routinely embed the outbound request URL (carrying client_id), provider error bodies, HTTP status lines, and internal hostnames. The listener is not debug-gated and `/api/auth/social` is PUBLIC_ACCESS, so an unauthenticated attacker who forces the provider exchange to fail learns the client_id, provider endpoints, and internal hostnames (CWE-209, information exposure). Fix: - Give every entry in ERROR_CODE_MAP a static, provider-agnostic `detail` string and emit that instead of the raw exception message. The response shape, status codes, error_code, and Content-Type are unchanged. - Inject Psr\Log\LoggerInterface (autowired) and log the full exception message + object server-side at error level, preserving diagnosability. Tests (tests/Unit/.../OAuthExceptionListenerTest.php): - Negative: a realistic raw Guzzle message with client_id, secret, provider URL, internal hostname, and 401 is asserted absent from the response detail (fails before the fix, passes after). - Edge: every handled exception type is checked to never echo its raw message; ignored exceptions are neither logged nor responded to. - Positive: full message is logged with error_code + exception context. Local verification (one-off containers): - phpunit (filtered): OK (20 tests, 92 assertions) - phpunit (full Unit): OK (2262 tests, 6252 assertions) - deptrac: Violations 0 - psalm: No errors found - php-cs-fixer: 0 of 2 files Closes #323 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 13 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 2/5
- There is a concrete security risk in
src/OAuth/Application/EventListener/OAuthExceptionListener.php: logging raw upstream OAuth exception messages can leak sensitive values likeclient_secretinto persistent logs. - Because this issue is high severity (7/10) with high confidence (9/10) and user/security impact, merge risk is elevated rather than a routine minor-fix PR.
- The issue in
specs/security-323-oauth-exception-message-leak/stories.mdappears lower impact and mostly affects reproducibility of verification steps, not runtime behavior. - Pay close attention to
src/OAuth/Application/EventListener/OAuthExceptionListener.php,specs/security-323-oauth-exception-message-leak/stories.md- prevent secret leakage in logs and make verification instructions environment-agnostic.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/OAuth/Application/EventListener/OAuthExceptionListener.php">
<violation number="1" location="src/OAuth/Application/EventListener/OAuthExceptionListener.php:91">
P1: Raw upstream OAuth exception messages are logged verbatim, which can persist secrets (e.g., `client_secret`) into logs.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as External Client (unauthenticated)
participant Kernel as Symfony HTTP Kernel
participant Listener as OAuthExceptionListener
participant Logger as PSR-3 Logger
participant Provider as GitHubOAuthProvider
Note over Client,Provider: Social Callback Flow with Error Handling
Client->>Kernel: GET /api/auth/social?code=invalid
Kernel->>Provider: exchangeCode(code)
Provider->>Provider: Guzzle POST to github.com/...?client_id=xxx
Provider-->>Kernel: OAuthProviderException("Client error: POST ... 401 ...")
Kernel->>Listener: __invoke(ExceptionEvent)
alt Handled Exception Type
Listener->>Listener: Lookup exception class in ERROR_CODE_MAP
Note over Listener: NEW: Uses static 'detail' from map instead of exception message
Listener->>Logger: error("OAuth flow failed: ...", error_code, exception)
Logger-->>Listener: Log written
Listener->>Kernel: Set JsonResponse (status, error_code, static detail)
Kernel-->>Client: 503 application/problem+json {"detail":"The authentication provider is currently unavailable..."}
else Unmapped Exception Type
Listener->>Listener: Class not in ERROR_CODE_MAP → return early
Listener-->>Kernel: No response set, no log written
Kernel-->>Client: Default Symfony error response
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| $this->logger->error('OAuth flow failed: ' . $exception->getMessage(), [ | ||
| 'error_code' => $mapping['error_code'], | ||
| 'exception' => $exception, | ||
| ]); |
There was a problem hiding this comment.
P1: Raw upstream OAuth exception messages are logged verbatim, which can persist secrets (e.g., client_secret) into logs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/OAuth/Application/EventListener/OAuthExceptionListener.php, line 91:
<comment>Raw upstream OAuth exception messages are logged verbatim, which can persist secrets (e.g., `client_secret`) into logs.</comment>
<file context>
@@ -65,13 +88,18 @@ public function __invoke(ExceptionEvent $event): void
return;
}
+ $this->logger->error('OAuth flow failed: ' . $exception->getMessage(), [
+ 'error_code' => $mapping['error_code'],
+ 'exception' => $exception,
</file context>
| $this->logger->error('OAuth flow failed: ' . $exception->getMessage(), [ | |
| 'error_code' => $mapping['error_code'], | |
| 'exception' => $exception, | |
| ]); | |
| $this->logger->error('OAuth flow failed.', [ | |
| 'error_code' => $mapping['error_code'], | |
| 'exception_class' => $exception::class, | |
| ]); |
There was a problem hiding this comment.
Acknowledged, but this is a deliberate, documented, test-enforced design rather than a leak. The security boundary for CWE-209 is the client response, where the raw message is fully suppressed (every 'detail' is a static, provider-agnostic string — see testProviderExceptionDoesNotLeakRawUpstreamMessage / testHandledExceptionsNeverLeakTheirRawMessage). Server-side logs are a trusted operator boundary; FR-3/FR-4 (Story 2) intentionally record the full message + exception for diagnosability, asserted by testFullExceptionMessageIsLoggedServerSide and testLoggedMessagePrefixesTheRawExceptionMessage. The specific concern (client_secret) does not hold in practice: the secret travels in the token-exchange request body, while league/oauth2-client's IdentityProviderException message is built from the provider response (error/error_description), not the request. Removing the server-side message would break the documented behavior and reduce diagnosability for a threat that is not realized here. No code change.
There was a problem hiding this comment.
Got it, thanks for the detailed clarification.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 551 551
Lines 9541 9547 +6
=========================================
+ Hits 9541 9547 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Vadym <kostiukdsfv@gmail.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Refactor OAuthExceptionListener to satisfy PHP Insights (extract response builder to cut function length, single-line detail to clear line-length, named log prefix constant) and add a unit test that pins the exact 'OAuth flow failed: ' + message log contract to kill the two escaped Infection mutants (concat-swap and prefix-removal), restoring MSI to 100%. Tidy the leak test to satisfy the tests phpinsights config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Replace machine-specific verification commands in the security story with portable, repository-standard equivalents (make unit-tests and a docker compose exec variant), removing hardcoded host paths and a branch-specific image tag so the steps are reproducible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Security fix — Closes #323
Vulnerability (CWE-209, MEDIUM)
OAuthExceptionListenerreturned the client-facingapplication/problem+jsonresponse withdetail => $exception->getMessage(). ForOAuthProviderExceptionthat message isOAuth provider <p> error: <raw>, where<raw>is the verbatimgetMessage()of the Guzzle/league exception caught inGitHubOAuthProviderduring token exchange / profile fetch. Those upstream messages routinely embed the outbound request URL (carryingclient_id), the provider's raw error body, HTTP status lines, and internal hostnames.The listener is not debug-gated, and
/api/auth/socialisPUBLIC_ACCESSinsecurity.yaml. An unauthenticated attacker who forces the provider exchange to fail (crafted/invalidcode) receives a503whosedetaildiscloses the configuredclient_id, the exact provider endpoints, internal error semantics, and internal hostnames — useful for reconnaissance.Fix
detailstring to every entry inERROR_CODE_MAPand emit that instead of the raw exception message. Response shape, HTTP status codes,error_code,type,title, andContent-Typeare unchanged (no client-contract break).Psr\Log\LoggerInterface(autowired via existing_defaults) and log the full exception message + object aterrorlevel with theerror_code, preserving server-side diagnosability.Tests
tests/Unit/OAuth/Application/EventListener/OAuthExceptionListenerTest.phpclient_id, secret, provider URL, internal hostname,401) is asserted absent from the responsedetail— fails before the fix, passes after.error_code+ exception context; existing status/contract assertions still pass.Local verification (one-off containers)
OAuthException|ExceptionListener): OK (20 tests, 92 assertions)BMAD spec
specs/security-323-oauth-exception-message-leak/(prd.md,stories.md)🤖 Generated with Claude Code
Summary by cubic
Stop leaking raw OAuth provider error messages to clients by returning static, safe
detailstrings in the problem+json response. The response shape and status codes stay the same; full errors are logged server-side for diagnosis.Bug Fixes
detailstrings fromOAuthExceptionListener::ERROR_CODE_MAPinstead of$exception->getMessage().Psr\Log\LoggerInterfaceand log the full exception (witherror_codeand context) at error level, prefixed withOAuth flow failed:.Refactors
buildProblemResponse()for clearer response construction and shorter listener method; introduceLOG_MESSAGE_PREFIXconstant.make unit-testsanddocker compose execvariants.Written for commit 9ab8c28. Summary will update on new commits.