Skip to content

Conversation

@EnkiP
Copy link
Member

@EnkiP EnkiP commented Dec 10, 2025

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

@qltysh
Copy link

qltysh bot commented Dec 10, 2025

11 new issues

Tool Category Rule Count
qlty Structure Function with high complexity (count = 6): routes 6
qlty Structure Function with many parameters (count = 4): create 5

'availableCollections' => 'read'
}.freeze

def self.create(forest_server_url, auth_info, action, extra = {})
Copy link

Choose a reason for hiding this comment

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

Found 2 issues:

1. Function with many parameters (count = 4): create [qlty:function-parameters]


2. Function with high complexity (count = 5): create [qlty:function-complexity]

JWT.encode(payload, @auth_secret, 'HS256')
end

def build_refresh_token(client, user, rendering_id, forest_refresh_token, decoded)
Copy link

Choose a reason for hiding this comment

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

Function with many parameters (count = 5): build_refresh_token [qlty:function-parameters]


private

def error_redirect(redirect_uri, error, error_description, state)
Copy link

Choose a reason for hiding this comment

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

Function with many parameters (count = 4): error_redirect [qlty:function-parameters]

@EnkiP
Copy link
Member Author

EnkiP commented Dec 10, 2025

Code review

Found 2 issues:

  1. Missing require for error classes causes NameError at runtime - The OAuthProvider class raises custom errors (UnsupportedTokenTypeError, InvalidTokenError, InvalidClientError) but never requires errors.rb. With Zeitwerk autoloading, these error classes won't be loaded because the file is named errors.rb (expecting an Errors constant) while the code references individual error class names. This will cause NameError: uninitialized constant when any of these errors are raised.

require 'faraday'
require 'jwt'
require 'json'

Fix: Add require_relative 'errors' at the top of oauth_provider.rb and token_generator.rb, or split error classes into individual files following Zeitwerk naming conventions.

  1. Open redirect vulnerability in OAuth authorize endpoint - The redirect_uri parameter is used directly in error redirects without validating it against the client's registered redirect URIs. Per OAuth 2.0 (RFC 6749 Section 3.1.2.2), the authorization server MUST validate redirect_uri against pre-registered URIs. An attacker can set redirect_uri to a malicious URL, and if client validation fails, the error response will redirect to the attacker's site.

# Validate client exists
client = oauth_provider.get_client(client_id)
return error_redirect(redirect_uri, 'invalid_client', 'Client not found', state) unless client

Fix: Before using redirect_uri in error_redirect, validate it against client['redirect_uris'] returned by get_client(). If the URI doesn't match any registered URI, return an error response directly instead of redirecting.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

uri.to_s
end

def exchange_authorization_code(client, authorization_code, code_verifier, redirect_uri)
Copy link

Choose a reason for hiding this comment

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

Function with many parameters (count = 4): exchange_authorization_code [qlty:function-parameters]


private

def authorization_code_payload(client, code, verifier, redirect_uri)
Copy link

Choose a reason for hiding this comment

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

Function with many parameters (count = 4): authorization_code_payload [qlty:function-parameters]

@qltysh
Copy link

qltysh bot commented Dec 11, 2025

Coverage Impact

⬇️ Merging this pull request will decrease total coverage on main by 0.50%.

Modified Files with Diff Coverage (12)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: B
packages/forest_admin_agent/lib/forest_admin_agent/http/router.rb50.0%79-83
New file Coverage rating: A
...min_agent/lib/forest_admin_agent/routes/mcp/oauth_authorize.rb97.5%12
New file Coverage rating: F
...rest_admin_agent/lib/forest_admin_agent/mcp/token_generator.rb32.6%8-9, 13-22, 28-30...
New file Coverage rating: A
...dmin_agent/lib/forest_admin_agent/mcp/invalid_request_error.rb100.0%
New file Coverage rating: A
..._admin_agent/lib/forest_admin_agent/mcp/invalid_token_error.rb100.0%
New file Coverage rating: A
...orest_admin_agent/lib/forest_admin_agent/mcp/oauth_provider.rb97.0%114-115
New file Coverage rating: A
...dmin_agent/lib/forest_admin_agent/routes/mcp/oauth_metadata.rb92.3%12
New file Coverage rating: F
...est_admin_agent/lib/forest_admin_agent/mcp/protocol_handler.rb34.1%7-9, 13-32, 39-46...
New file Coverage rating: A
...admin_agent/lib/forest_admin_agent/mcp/invalid_client_error.rb100.0%
New file Coverage rating: A
...t_admin_agent/lib/forest_admin_agent/routes/mcp/oauth_token.rb93.8%12, 48-56
New file Coverage rating: A
..._admin_agent/lib/forest_admin_agent/routes/mcp/mcp_endpoint.rb97.8%12
New file Coverage rating: A
...ent/lib/forest_admin_agent/mcp/unsupported_token_type_error.rb100.0%
Total77.9%
🤖 Increase coverage with AI coding...
In the `feat/mcp-server` branch, add test coverage for this new code:

- `packages/forest_admin_agent/lib/forest_admin_agent/http/router.rb` -- Line 79-83
- `packages/forest_admin_agent/lib/forest_admin_agent/mcp/oauth_provider.rb` -- Line 114-115
- `packages/forest_admin_agent/lib/forest_admin_agent/mcp/protocol_handler.rb` -- Lines 7-9, 13-32, 39-46, 52, 65, 70-77, 82, 87, 93, and 101
- `packages/forest_admin_agent/lib/forest_admin_agent/mcp/token_generator.rb` -- Lines 8-9, 13-22, 28-30, 34-44, 53-54, 59-66, 70-71, and 75-87
- `packages/forest_admin_agent/lib/forest_admin_agent/routes/mcp/mcp_endpoint.rb` -- Line 12
- `packages/forest_admin_agent/lib/forest_admin_agent/routes/mcp/oauth_authorize.rb` -- Line 12
- `packages/forest_admin_agent/lib/forest_admin_agent/routes/mcp/oauth_metadata.rb` -- Line 12
- `packages/forest_admin_agent/lib/forest_admin_agent/routes/mcp/oauth_token.rb` -- Lines 12 and 48-56

🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

fetched_at: now
}

{ collections: collections }
Copy link

Choose a reason for hiding this comment

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

Function with high complexity (count = 5): fetch_forest_schema [qlty:function-complexity]

rescue UnauthorizedError => e
jsonrpc_error_response(-32_603, e.message, 401)
rescue ForbiddenError => e
jsonrpc_error_response(-32_603, e.message, 403)
Copy link

Choose a reason for hiding this comment

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

Function with high complexity (count = 5): handle_mcp [qlty:function-complexity]

rescue StandardError => e
raise unless redirect_uri

error_redirect(redirect_uri, 'server_error', e.message, state)
Copy link

Choose a reason for hiding this comment

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

Function with high complexity (count = 8): handle_authorize [qlty:function-complexity]

error_description: e.message
},
status: 400
}
Copy link

Choose a reason for hiding this comment

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

Function with high complexity (count = 6): handle_token [qlty:function-complexity]

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.

2 participants