Fix/auth controller state check#38
Conversation
|
Warning Rate limit exceeded@brettchaldecott has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…to validate the id token nonce. There are currently problems retrieving the ID token. The standard Oauth2 ruby library drops the ID token if found.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
lib/kinde_sdk/controllers/auth_controller.rb (8)
63-63: Consider using route helper instead of hardcoded path.For consistency with line 60, consider using main_app.root_path here as well.
- redirect_to "/", alert: "Authentication failed" + redirect_to main_app.root_path, alert: "Authentication failed"
88-88: Consider using route helper instead of hardcoded path.For consistency with line 60, consider using main_app.root_path here as well.
- redirect_to "/" + redirect_to main_app.root_path
93-125: Refactor validate_state method to reduce complexity.The static analysis indicates this method has high complexity. Consider breaking it down into smaller, focused methods for better maintainability.
def validate_state unless valid_session_state? redirect_with_alert("Invalid authentication state") return end unless valid_returned_state? redirect_with_alert("Invalid authentication state") return end if state_expired? redirect_with_alert("Authentication session expired") return end end private def valid_session_state? if session[:auth_nonce] && session[:auth_state] true else Rails.logger.warn("Missing session state or nonce") false end end def valid_returned_state? returned_state = params[:state] stored_state = session[:auth_state] stored_url = stored_state["redirect_url"] # Extract the state from the stored redirect_url Rails.logger.info("The uri is : [#{stored_url}]") parsed_url = URI.parse(stored_url) query_params = CGI.parse(parsed_url.query || "") stored_state_from_url = query_params["state"]&.first if returned_state.present? && returned_state == stored_state_from_url true else Rails.logger.warn("State validation failed: returned=#{returned_state}, expected=#{stored_state_from_url}") false end end def state_expired? Time.current.to_i - session[:auth_state]["requested_at"] > 900 end def redirect_with_alert(message) redirect_to main_app.root_path, alert: message end🧰 Tools
🪛 RuboCop (1.73)
[convention] 93-125: Assignment Branch Condition size for validate_state is too high. [<6, 30, 9> 31.89/23]
(Metrics/AbcSize)
[convention] 93-125: Cyclomatic complexity for
validate_stateis too high. [8/7](Metrics/CyclomaticComplexity)
97-97: Consider using route helper instead of hardcoded path.For consistency with line 60, consider using main_app.root_path here as well.
- redirect_to "/", alert: "Invalid authentication state" + redirect_to main_app.root_path, alert: "Invalid authentication state"
115-115: Consider using route helper instead of hardcoded path.For consistency with line 60, consider using main_app.root_path here as well.
- redirect_to "/", alert: "Invalid authentication state" + redirect_to main_app.root_path, alert: "Invalid authentication state"
122-122: Consider using route helper instead of hardcoded path.For consistency with line 60, consider using main_app.root_path here as well.
- redirect_to "/", alert: "Authentication session expired" + redirect_to main_app.root_path, alert: "Authentication session expired"
151-153: Improve error handling in validate_nonce method.The rescue block catches all standard errors without specific handling for expected JWT exceptions, and the error information is not preserved in the logs.
- rescue StandardError => e - puts "Error: #{e.message}" - false + rescue JWT::VerificationError => e + Rails.logger.error("JWT verification error: #{e.message}") + false + rescue JWT::ExpiredSignature => e + Rails.logger.error("JWT token expired: #{e.message}") + false + rescue JWT::DecodeError => e + Rails.logger.error("JWT decode error: #{e.message}") + false + rescue StandardError => e + Rails.logger.error("Unexpected error during nonce validation: #{e.message}") + false
107-107: Consider removing or limiting URI debug logging in production.Logging full URIs might expose sensitive information in log files. Consider limiting this to development or debug level.
- Rails.logger.info("The uri is : [#{stored_url}]") + Rails.logger.debug("Processing stored redirect URI") if Rails.env.development?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/kinde_sdk.rb(2 hunks)lib/kinde_sdk/controllers/auth_controller.rb(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/kinde_sdk.rb
🧰 Additional context used
🪛 RuboCop (1.73)
lib/kinde_sdk/controllers/auth_controller.rb
[convention] 93-125: Assignment Branch Condition size for validate_state is too high. [<6, 30, 9> 31.89/23]
(Metrics/AbcSize)
[convention] 93-125: Cyclomatic complexity for validate_state is too high. [8/7]
(Metrics/CyclomaticComplexity)
🔇 Additional comments (3)
lib/kinde_sdk/controllers/auth_controller.rb (3)
2-6: LGTM: Required imports for enhanced state validation.The additional imports are necessary for the new functionality: URI/CGI for URL parsing, Net/HTTP and JSON for JWKS fetching, and JWT for token validation.
11-11: LGTM: Before action updated to use the new validation method.The before_action has been appropriately changed from validate_nonce to validate_state, which aligns with the PR objective to enhance state validation.
60-60: LGTM: Using Rails route helper for redirection.Using main_app.root_path instead of a hardcoded path improves maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/kinde_sdk/controllers/auth_controller.rb (1)
129-155:⚠️ Potential issueNew validate_nonce method implements proper JWT validation.
The method properly fetches JWKS, validates the JWT with appropriate checks, and compares the nonce from the token with the stored nonce.
Two issues need addressing:
- Error logging should use Rails logger instead of puts:
rescue StandardError => e - puts "Error: #{e.message}" + Rails.logger.error("Nonce validation error: #{e.message}") false
- Add timeout to HTTP request to prevent hanging:
- jwks_response = Net::HTTP.get(jwks_uri) + http = Net::HTTP.new(jwks_uri.host, jwks_uri.port) + http.use_ssl = (jwks_uri.scheme == 'https') + http.open_timeout = 5 # seconds + http.read_timeout = 10 # seconds + jwks_response = http.get(jwks_uri.request_uri).body
- Avoid logging sensitive token information:
- Rails.logger.info("The nonce from the token is: #{nonce_from_token}") - Rails.logger.info("The original nonce is: #{original_nonce}") + Rails.logger.debug("Nonce validation performed") if Rails.env.development?
🧹 Nitpick comments (3)
lib/kinde_sdk/controllers/auth_controller.rb (3)
60-61: Consider making redirect URLs configurable.The hardcoded "/" URLs for redirects after success, failure, and logout may not be appropriate for all installations of the SDK. Consider making these URLs configurable.
- redirect_to "/" + redirect_to KindeSdk.config.default_redirect_url || "/"Also applies to: 64-64, 89-89
94-126: New validate_state method implements comprehensive state validation.The method now performs three important security checks:
- Verifies both nonce and state exist in session
- Validates the returned state against the state extracted from stored redirect URL
- Enforces a 15-minute expiration on the authentication state
This is a significant security improvement, but the method is quite complex. Consider breaking it into smaller, focused methods.
The static analysis tools flagged this method for high complexity. You could refactor it into smaller methods:
def validate_state # Check if nonce and state exist in session unless session_has_required_auth_data? handle_invalid_state("Missing session state or nonce") return end # Verify state matches unless states_match? handle_invalid_state("Invalid authentication state") return end # Check state age if state_expired? handle_expired_state return end end private def session_has_required_auth_data? session[:auth_nonce] && session[:auth_state] end def states_match? returned_state = params[:state] stored_state_from_url = extract_state_from_stored_url returned_state.present? && returned_state == stored_state_from_url end def extract_state_from_stored_url stored_url = session[:auth_state]["redirect_url"] Rails.logger.info("The uri is : [#{stored_url}]") parsed_url = URI.parse(stored_url) query_params = CGI.parse(parsed_url.query || "") query_params["state"]&.first end def state_expired? Time.current.to_i - session[:auth_state]["requested_at"] > 900 end def handle_invalid_state(message) Rails.logger.warn(message) redirect_to "/", alert: "Invalid authentication state" end def handle_expired_state Rails.logger.warn("Authentication state expired") redirect_to "/", alert: "Authentication session expired" end🧰 Tools
🪛 RuboCop (1.73)
[convention] 94-126: Assignment Branch Condition size for validate_state is too high. [<6, 30, 9> 31.89/23]
(Metrics/AbcSize)
[convention] 94-126: Cyclomatic complexity for
validate_stateis too high. [8/7](Metrics/CyclomaticComplexity)
131-131: Add error handling for JWKS retrieval.The JWKS retrieval doesn't handle network errors specifically, which could lead to confusing error messages.
- jwks_uri = URI.parse("#{issuer}/.well-known/jwks.json") - jwks_response = Net::HTTP.get(jwks_uri) - jwks = JSON.parse(jwks_response) + jwks_uri = URI.parse("#{issuer}/.well-known/jwks.json") + begin + jwks_response = Net::HTTP.get(jwks_uri) + jwks = JSON.parse(jwks_response) + rescue SocketError, Net::OpenTimeout, Net::ReadTimeout => e + Rails.logger.error("Failed to retrieve JWKS: #{e.message}") + return false + rescue JSON::ParserError => e + Rails.logger.error("Failed to parse JWKS response: #{e.message}") + return false + end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/kinde_sdk.rb(3 hunks)lib/kinde_sdk/controllers/auth_controller.rb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/kinde_sdk.rb
🧰 Additional context used
🧬 Code Definitions (1)
lib/kinde_sdk/controllers/auth_controller.rb (1)
lib/kinde_sdk.rb (1) (1)
client(104-107)
🪛 RuboCop (1.73)
lib/kinde_sdk/controllers/auth_controller.rb
[convention] 94-126: Assignment Branch Condition size for validate_state is too high. [<6, 30, 9> 31.89/23]
(Metrics/AbcSize)
[convention] 94-126: Cyclomatic complexity for validate_state is too high. [8/7]
(Metrics/CyclomaticComplexity)
🔇 Additional comments (4)
lib/kinde_sdk/controllers/auth_controller.rb (4)
2-6: Required dependencies added for enhanced state validation.The additional requires are necessary for the new functionality: URI and CGI for URL parsing, Net/HTTP and JSON for API requests, and JWT for token validation.
11-11: Updated before_action to use new validate_state method.This change reflects the shift from just validating the nonce to a more comprehensive state validation approach, which is the main goal of this PR.
35-49: Added ID token validation to verify nonce.The enhanced callback now retrieves an ID token and performs validation of the nonce, adding an important security check to prevent replay attacks. This implementation properly extracts key parameters and handles validation failures.
52-53: Tokens properly filtered before storage in session.Using slice to select only necessary token information (access_token, refresh_token, expires_at) helps minimize session size and avoid storing sensitive information.
|
Good afternoon, Brett. I had the chance to run through these fixes and the namespace is being picked up and I can see it's working correctly, thank you! |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
spec/kinde_sdk_spec.rb (1)
89-91:⚠️ Potential issueUpdate size expectation to match new implementation
This test is failing because it expects 5 keys but the
fetch_tokensmethod now returns 6 keys (including the newid_token).- expect(described_class.fetch_tokens(code).keys.size).to eq(5) + expect(described_class.fetch_tokens(code).keys.size).to eq(6)Alternatively, if you want to be more defensive against future changes, you could consider:
expect(described_class.fetch_tokens(code).keys).to include(:access_token, :id_token, :expires_at)🧰 Tools
🪛 GitHub Actions: Ruby
[error] 90-90: KindeSdk#fetch_tokens with redefined callback_url calls /token url with proper body and headers failed: expected size 5 but got 4.
🧹 Nitpick comments (1)
spec/kinde_sdk_spec.rb (1)
57-84: Add a test for the id_token functionalityThe test coverage for the
fetch_tokensmethod should be expanded to explicitly verify that theid_tokenvalue is properly extracted from the token params.Consider adding a dedicated test that mocks a response with an
id_tokenvalue and verifies it's correctly included in the returned hash:it "includes id_token from token params" do stub_request(:post, "#{domain}/oauth2/token") .to_return( status: 200, body: { "access_token": "eyJ", "id_token": "id_token_value", "expires_in": 86399, "scope": "", "token_type": "bearer" }.to_json, headers: { "content-type" => "application/json;charset=UTF-8" } ) result = described_class.fetch_tokens(code) expect(result[:id_token]).to eq("id_token_value") endThis ensures the new
id_tokenfunctionality is properly tested beyond just checking for the presence of the key.🧰 Tools
🪛 GitHub Actions: Ruby
[error] 83-83: KindeSdk#fetch_tokens calls /token url with proper body and headers failed: expected keys ['scope', 'token_type', 'access_token', 'id_token', 'refresh_token', 'expires_at'] but got [:access_token, :expires_at, :scope, :token_type].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/kinde_sdk_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
spec/kinde_sdk_spec.rb (1)
lib/kinde_sdk.rb (1) (1)
fetch_tokens(65-93)
🪛 GitHub Actions: Ruby
spec/kinde_sdk_spec.rb
[error] 83-83: KindeSdk#fetch_tokens calls /token url with proper body and headers failed: expected keys ['scope', 'token_type', 'access_token', 'id_token', 'refresh_token', 'expires_at'] but got [:access_token, :expires_at, :scope, :token_type].
[error] 90-90: KindeSdk#fetch_tokens with redefined callback_url calls /token url with proper body and headers failed: expected size 5 but got 4.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/kinde_sdk_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
spec/kinde_sdk_spec.rb (2)
lib/kinde_sdk.rb (2) (2)
fetch_tokens(65-93)refresh_token(147-159)lib/kinde_sdk/client.rb (1) (1)
refresh_token(20-25)
🪛 RuboCop (1.73)
spec/kinde_sdk_spec.rb
[fatal] 83-83: unexpected token tCOLON
(Using Ruby 3.0 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🪛 GitHub Actions: Ruby
spec/kinde_sdk_spec.rb
[error] 83-83: Syntax error, unexpected ':', expecting ']' in line 83. The expected format is incorrect in the array of symbols.
🔇 Additional comments (3)
spec/kinde_sdk_spec.rb (3)
77-77: Good update to the mock response.You've correctly updated the stubbed response body to include the new
id_tokenandrefresh_tokenfields, which matches the actual implementation infetch_tokens. This ensures the test accurately reflects the real behavior of the method.
90-90: Good update to check for the correct number of keys.This change correctly validates that the
fetch_tokensmethod now returns 6 keys, which aligns with the implementation that returnsaccess_token,id_token,expires_at,refresh_token,scope, andtoken_type.
83-83: Consider usingcontain_exactlyinstead ofeq.Since the order of keys in a hash isn't guaranteed in all Ruby versions, using
contain_exactlywould make the test more robust by checking that all expected keys are present without enforcing a specific order.- expect(described_class.fetch_tokens(code).keys).to eq([:access_token :id_token :expires_at :refresh_token :scope :token_type]) + expect(described_class.fetch_tokens(code).keys).to contain_exactly(:access_token, :id_token, :expires_at, :refresh_token, :scope, :token_type)This change aligns with the previous comment in this PR that suggested using
contain_exactly.🧰 Tools
🪛 RuboCop (1.73)
[fatal] 83-83: unexpected token tCOLON
(Using Ruby 3.0 parser; configure usingTargetRubyVersionparameter, underAllCops)(Lint/Syntax)
🪛 GitHub Actions: Ruby
[error] 83-83: Syntax error, unexpected ':', expecting ']' in line 83. The expected format is incorrect in the array of symbols.
2 participants
Explain your changes
Added extra logic to completely validate the state
Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.