Fix: prevent TypeError when callback state is missing; accept nullabl… #61
Conversation
…e state and throw OAuthException
WalkthroughUpdated OAuth state validation in KindeClientSDK: the state parameter is now nullable, and validation requires both incoming and stored states to be non-empty and strictly equal. Tests updated accordingly to reflect the stricter checks and nullable signature. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant App as App (Callback Handler)
participant SDK as KindeClientSDK
participant Store as State Storage
Browser->>App: OAuth redirect with code & state
App->>SDK: handleCallback(state)
SDK->>Store: get(stored_state)
Store-->>SDK: stored_state
alt Missing/empty state or mismatch
SDK-->>App: throw OAuthException
App-->>Browser: Error response
else Strict match
SDK->>SDK: proceed with token exchange
SDK-->>App: success
App-->>Browser: Continue login flow
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/Sdk/KindeClientSDK.php (2)
509-516: Use constant-time comparison and add return typeReplace strict
!==withhash_equalsto avoid timing side-channels, and add an explicit: voidreturn type for clarity.Apply this diff:
- private function checkStateAuthentication(?string $stateServer) + private function checkStateAuthentication(?string $stateServer): void { $storageOAuthState = $this->storage->getState(); - if (empty($stateServer) || empty($storageOAuthState) || $stateServer !== $storageOAuthState) { + if ($stateServer === null + || $stateServer === '' + || empty($storageOAuthState) + || !hash_equals((string) $storageOAuthState, (string) $stateServer)) { throw new OAuthException("Authentication failed because it tries to validate state"); } }
513-515: Clarify the exception messageMake the failure mode explicit to aid debugging and log triage.
- throw new OAuthException("Authentication failed because it tries to validate state"); + throw new OAuthException("Invalid or missing OAuth state parameter");lib/KindeClientSDK.php (2)
841-848: Use constant-time compare and annotate return typeSwitch to
hash_equalsand add: voidto tighten the contract and reduce attack surface.- private function checkStateAuthentication(?string $stateServer) + private function checkStateAuthentication(?string $stateServer): void { $storageOAuthState = $this->storage->getState(); - if (empty($stateServer) || empty($storageOAuthState) || $stateServer !== $storageOAuthState) { + if ($stateServer === null + || $stateServer === '' + || empty($storageOAuthState) + || !hash_equals((string) $storageOAuthState, (string) $stateServer)) { throw new OAuthException("Authentication failed because it tries to validate state"); } }
845-847: Improve error message for operatorsA clearer message reduces support noise and helps distinguish real CSRF/state issues from crawler hits.
- throw new OAuthException("Authentication failed because it tries to validate state"); + throw new OAuthException("Invalid or missing OAuth state parameter");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/KindeClientSDK.php(1 hunks)test/Sdk/KindeClientSDK.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/Sdk/KindeClientSDK.php (3)
lib/KindeClientSDK.php (1)
checkStateAuthentication(841-848)lib/Sdk/Storage/Storage.php (1)
getState(78-81)test/Sdk/Storage/Storage.php (1)
getState(56-59)
lib/KindeClientSDK.php (1)
lib/Sdk/Storage/Storage.php (1)
getState(78-81)
🔇 Additional comments (2)
test/Sdk/KindeClientSDK.php (1)
509-516: Fix prevents fatal TypeError on missing callback state — LGTMAccepting
?stringand guarding for empty/mismatch fulfills the PR objective and avoids runtime TypeErrors when bots hit the callback without state.lib/KindeClientSDK.php (1)
837-848: State validation hardening — LGTMNullability, explicit empty checks, and strict comparison align with OAuth CSRF protection best practices and prevent the prior TypeError path.
Explain your changes
checkStateAuthenticationnow accepts?stringand throwsOAuthExceptionwhen state is missing/empty or mismatched, preventing the fatal TypeError seen when bots/crawlers hit/callbackwithout astate.Files:
lib/KindeClientSDK.phptest/Sdk/KindeClientSDK.phpChecklist