Feat/oauth support #61
Conversation
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
…plan Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
|
Warning Rate limit exceeded@tuannvm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 58 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 (3)
WalkthroughThis update introduces OAuth 2.1 (JWT) authentication to the MCP Trino server, adds comprehensive documentation for OAuth/JWT setup and security, and refactors the HTTP server logic to use a new abstraction. The Trino configuration now supports an OAuth-enabled mode, and all documentation, configuration, and handler registration are updated to reflect the new secure, modern HTTP transport and authentication approach. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP_Server
participant OAuth_Provider
participant Trino_DB
Client->>MCP_Server: HTTP request with Bearer JWT (to /mcp)
MCP_Server->>MCP_Server: Extract & validate JWT (middleware)
alt JWT valid
MCP_Server->>Trino_DB: Execute query (basic/anon auth)
Trino_DB-->>MCP_Server: Query result
MCP_Server-->>Client: Result (HTTP StreamableHTTP)
else JWT invalid/missing
MCP_Server-->>Client: 401 Unauthorized (OAuth error)
end
sequenceDiagram
participant Client
participant MCP_Server
Client->>MCP_Server: GET /.well-known/oauth-authorization-server
MCP_Server-->>Client: OAuth metadata (no auth required)
Possibly related PRs
Suggested labels
Poem
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. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
README.md (2)
617-617: Consider using proper headings instead of bold emphasis.The static analysis correctly identifies that these lines use emphasis for what appear to be method descriptions. Consider using proper markdown headings for better document structure.
-**Method 1: Using jwt.io (Development/Testing)** +##### Method 1: Using jwt.io (Development/Testing)Apply similar changes to lines 631, 647, 664, and 688.
Also applies to: 631-631, 647-647, 664-664, 688-688
907-915: Remove blank lines inside blockquotes for better formatting.The blockquote contains unnecessary blank lines that affect markdown rendering.
> **Note**: When `TRINO_SCHEME` is set to "https", `TRINO_SSL` is automatically set to true regardless of the provided value. - > **Important**: The default connection mode is HTTPS. If you're using an HTTP-only Trino server, you must set `TRINO_SCHEME=http` in your environment variables. - > **Security Note**: By default, only read-only queries (SELECT, SHOW, DESCRIBE, EXPLAIN) are allowed to prevent SQL injection. If you need to execute write operations or other non-read queries, set `TRINO_ALLOW_WRITE_QUERIES=true`, but be aware this bypasses this security protection. - > **For Web Client Integration**: When using with web clients, set `MCP_TRANSPORT=http` and connect to the `/mcp` endpoint for StreamableHTTP support. The `/sse` endpoint is maintained for backward compatibility. - > **JWT Authentication**: When `TRINO_OAUTH_ENABLED=true`, the server requires JWT tokens in the Authorization header (`Bearer <token>`). This is particularly useful for web-based MCP clients that need authentication. - > **HTTPS Support**: For production deployments, configure HTTPS by setting `HTTPS_CERT_FILE` and `HTTPS_KEY_FILE` environment variables. This is strongly recommended when using JWT authentication.docs/jwt.md (1)
384-388: Add language specification to the code block.The fenced code block should specify the language for proper syntax highlighting.
-``` +```log OAuth: Token extracted from request (length: 193) OAuth: Received token for request ID 1: eyJhbGciOiJub25lIi... OAuth: Authenticated user testuser for request ID: 1</blockquote></details> <details> <summary>docs/oauth.md (1)</summary><blockquote> `271-280`: **Add language specification to the HTTP request example.** The code block should specify the language for proper syntax highlighting. ```diff -``` +```http POST /oauth/token Content-Type: application/x-www-form-urlencoded grant_type=authorization_code& code=abc123& resource=https://mcp-server.com:8080& client_id=claude-code</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3ca9253bec289152f22dd7b3a5d259d519d6fbf6 and 054678a64ed252b824064ee743fe5ee91d8db607. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `go.sum` is excluded by `!**/*.sum` </details> <details> <summary>📒 Files selected for processing (10)</summary> * `CLAUDE.md` (4 hunks) * `README.md` (7 hunks) * `cmd/main.go` (3 hunks) * `docs/jwt.md` (1 hunks) * `docs/oauth-config.md` (1 hunks) * `docs/oauth.md` (1 hunks) * `go.mod` (1 hunks) * `internal/auth/oauth.go` (1 hunks) * `internal/config/config.go` (5 hunks) * `internal/handlers/trino_handlers.go` (7 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (3)</summary> <details> <summary>internal/config/config.go</summary> Instructions used from: **Sources:** 📄 **CodeRabbit Inference Engine** - CLAUDE.md </details> <details> <summary>internal/handlers/trino_handlers.go</summary> Instructions used from: **Sources:** 📄 **CodeRabbit Inference Engine** - CLAUDE.md </details> <details> <summary>cmd/main.go</summary> Instructions used from: **Sources:** 📄 **CodeRabbit Inference Engine** - CLAUDE.md </details> </details><details> <summary>🧠 Learnings (11)</summary> <details> <summary>📓 Common learnings</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.</details> <details> <summary>go.mod (7)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to .github/workflows/build.yml : Go mod tidy verification must be performed in the CI pipeline.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (execute_query,list_catalogs,list_schemas,list_tables,get_table_schema) must return JSON-formatted responses and handle parameter validation.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented ininternal/trino/client.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment variables such asTRINO_HOST,TRINO_PORT,TRINO_USER,TRINO_PASSWORD,TRINO_SCHEME,TRINO_SSL,TRINO_SSL_INSECURE,TRINO_ALLOW_WRITE_QUERIES,TRINO_QUERY_TIMEOUT,MCP_TRANSPORT,MCP_PORT, andMCP_HOSTmust be supported and validated in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to examples/test_query.go : The fileexamples/test_query.goshould be used for HTTP API manual testing.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to cmd/main.go : Main entry point for the server should be implemented incmd/main.gowith server initialization, Trino connection testing, transport selection (STDIO vs HTTP with SSE), graceful shutdown, and CORS support.</details> <details> <summary>internal/config/config.go (8)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment variables such asTRINO_HOST,TRINO_PORT,TRINO_USER,TRINO_PASSWORD,TRINO_SCHEME,TRINO_SSL,TRINO_SSL_INSECURE,TRINO_ALLOW_WRITE_QUERIES,TRINO_QUERY_TIMEOUT,MCP_TRANSPORT,MCP_PORT, andMCP_HOSTmust be supported and validated in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment-based configuration with validation, security defaults (HTTPS, read-only queries), and timeout configuration with validation should be implemented ininternal/config/config.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : HTTPS scheme must force SSL=true regardless of theTRINO_SSLsetting in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Invalid timeout values forTRINO_QUERY_TIMEOUTmust fall back to 30 seconds with a warning in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented ininternal/trino/client.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to internal/trino/client.go : Connection pool settings should be: 10 max open, 5 max idle, 5min max lifetime for Trino client connections.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to examples/test_query.go : The fileexamples/test_query.goshould be used for HTTP API manual testing.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : TheisReadOnlyQuery()function ininternal/trino/client.goshould allow only SELECT, SHOW, DESCRIBE, EXPLAIN, and WITH (CTEs) queries by default, blocking INSERT, UPDATE, DELETE, CREATE, DROP, and ALTER unlessTRINO_ALLOW_WRITE_QUERIES=trueis set.</details> <details> <summary>internal/handlers/trino_handlers.go (6)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (execute_query,list_catalogs,list_schemas,list_tables,get_table_schema) must return JSON-formatted responses and handle parameter validation.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented ininternal/trino/client.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to cmd/main.go : Main entry point for the server should be implemented incmd/main.gowith server initialization, Trino connection testing, transport selection (STDIO vs HTTP with SSE), graceful shutdown, and CORS support.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment variables such asTRINO_HOST,TRINO_PORT,TRINO_USER,TRINO_PASSWORD,TRINO_SCHEME,TRINO_SSL,TRINO_SSL_INSECURE,TRINO_ALLOW_WRITE_QUERIES,TRINO_QUERY_TIMEOUT,MCP_TRANSPORT,MCP_PORT, andMCP_HOSTmust be supported and validated in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to examples/test_query.go : The fileexamples/test_query.goshould be used for HTTP API manual testing.</details> <details> <summary>docs/oauth-config.md (1)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment variables such asTRINO_HOST,TRINO_PORT,TRINO_USER,TRINO_PASSWORD,TRINO_SCHEME,TRINO_SSL,TRINO_SSL_INSECURE,TRINO_ALLOW_WRITE_QUERIES,TRINO_QUERY_TIMEOUT,MCP_TRANSPORT,MCP_PORT, andMCP_HOSTmust be supported and validated in configuration logic.</details> <details> <summary>CLAUDE.md (14)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (execute_query,list_catalogs,list_schemas,list_tables,get_table_schema) must return JSON-formatted responses and handle parameter validation.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to cmd/main.go : Main entry point for the server should be implemented incmd/main.gowith server initialization, Trino connection testing, transport selection (STDIO vs HTTP with SSE), graceful shutdown, and CORS support.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented ininternal/trino/client.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment variables such asTRINO_HOST,TRINO_PORT,TRINO_USER,TRINO_PASSWORD,TRINO_SCHEME,TRINO_SSL,TRINO_SSL_INSECURE,TRINO_ALLOW_WRITE_QUERIES,TRINO_QUERY_TIMEOUT,MCP_TRANSPORT,MCP_PORT, andMCP_HOSTmust be supported and validated in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Multi-platform support must be implemented using GoReleaser for linux/darwin/windows on amd64/arm64/arm.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to examples/test_query.go : The fileexamples/test_query.goshould be used for HTTP API manual testing.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to .github/workflows/build.yml : Multi-platform builds must be performed via GoReleaser in the CI pipeline.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to internal/trino/client.go : Connection pool settings should be: 10 max open, 5 max idle, 5min max lifetime for Trino client connections.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Distribution must be performed via GitHub Releases, GHCR, and Homebrew tap (tuannvm/mcp).Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : HTTPS scheme must force SSL=true regardless of theTRINO_SSLsetting in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Invalid timeout values forTRINO_QUERY_TIMEOUTmust fall back to 30 seconds with a warning in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment-based configuration with validation, security defaults (HTTPS, read-only queries), and timeout configuration with validation should be implemented ininternal/config/config.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : TheisReadOnlyQuery()function ininternal/trino/client.goshould allow only SELECT, SHOW, DESCRIBE, EXPLAIN, and WITH (CTEs) queries by default, blocking INSERT, UPDATE, DELETE, CREATE, DROP, and ALTER unlessTRINO_ALLOW_WRITE_QUERIES=trueis set.</details> <details> <summary>cmd/main.go (7)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to cmd/main.go : Main entry point for the server should be implemented incmd/main.gowith server initialization, Trino connection testing, transport selection (STDIO vs HTTP with SSE), graceful shutdown, and CORS support.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (execute_query,list_catalogs,list_schemas,list_tables,get_table_schema) must return JSON-formatted responses and handle parameter validation.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented ininternal/trino/client.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to examples/test_query.go : The fileexamples/test_query.goshould be used for HTTP API manual testing.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment variables such asTRINO_HOST,TRINO_PORT,TRINO_USER,TRINO_PASSWORD,TRINO_SCHEME,TRINO_SSL,TRINO_SSL_INSECURE,TRINO_ALLOW_WRITE_QUERIES,TRINO_QUERY_TIMEOUT,MCP_TRANSPORT,MCP_PORT, andMCP_HOSTmust be supported and validated in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment-based configuration with validation, security defaults (HTTPS, read-only queries), and timeout configuration with validation should be implemented ininternal/config/config.go.</details> <details> <summary>README.md (9)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment variables such asTRINO_HOST,TRINO_PORT,TRINO_USER,TRINO_PASSWORD,TRINO_SCHEME,TRINO_SSL,TRINO_SSL_INSECURE,TRINO_ALLOW_WRITE_QUERIES,TRINO_QUERY_TIMEOUT,MCP_TRANSPORT,MCP_PORT, andMCP_HOSTmust be supported and validated in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to cmd/main.go : Main entry point for the server should be implemented incmd/main.gowith server initialization, Trino connection testing, transport selection (STDIO vs HTTP with SSE), graceful shutdown, and CORS support.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (execute_query,list_catalogs,list_schemas,list_tables,get_table_schema) must return JSON-formatted responses and handle parameter validation.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to internal/trino/client.go : Connection pool settings should be: 10 max open, 5 max idle, 5min max lifetime for Trino client connections.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented ininternal/trino/client.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : HTTPS scheme must force SSL=true regardless of theTRINO_SSLsetting in configuration logic.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment-based configuration with validation, security defaults (HTTPS, read-only queries), and timeout configuration with validation should be implemented ininternal/config/config.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : TheisReadOnlyQuery()function ininternal/trino/client.goshould allow only SELECT, SHOW, DESCRIBE, EXPLAIN, and WITH (CTEs) queries by default, blocking INSERT, UPDATE, DELETE, CREATE, DROP, and ALTER unlessTRINO_ALLOW_WRITE_QUERIES=trueis set.</details> <details> <summary>docs/jwt.md (4)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (execute_query,list_catalogs,list_schemas,list_tables,get_table_schema) must return JSON-formatted responses and handle parameter validation.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented ininternal/trino/client.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to cmd/main.go : Main entry point for the server should be implemented incmd/main.gowith server initialization, Trino connection testing, transport selection (STDIO vs HTTP with SSE), graceful shutdown, and CORS support.</details> <details> <summary>internal/auth/oauth.go (1)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.</details> <details> <summary>docs/oauth.md (2)</summary>Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented ininternal/handlers/trino_handlers.go.Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented ininternal/trino/client.go.</details> </details><details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>README.md</summary> 617-617: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 631-631: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 647-647: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 664-664: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 688-688: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 907-907: Blank line inside blockquote (MD028, no-blanks-blockquote) --- 909-909: Blank line inside blockquote (MD028, no-blanks-blockquote) --- 911-911: Blank line inside blockquote (MD028, no-blanks-blockquote) --- 913-913: Blank line inside blockquote (MD028, no-blanks-blockquote) --- 915-915: Blank line inside blockquote (MD028, no-blanks-blockquote) </details> <details> <summary>docs/jwt.md</summary> 384-384: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>docs/oauth.md</summary> 7-7: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 39-39: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 52-52: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 60-60: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 67-67: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 244-244: Trailing punctuation in heading Punctuation: ':' (MD026, no-trailing-punctuation) --- 255-255: Trailing punctuation in heading Punctuation: ':' (MD026, no-trailing-punctuation) --- 271-271: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 375-375: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 388-388: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 394-394: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 396-396: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 398-398: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 435-435: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 461-461: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 597-597: Trailing punctuation in heading Punctuation: ':' (MD026, no-trailing-punctuation) --- 604-604: Trailing punctuation in heading Punctuation: ':' (MD026, no-trailing-punctuation) --- 611-611: Trailing punctuation in heading Punctuation: ':' (MD026, no-trailing-punctuation) --- 616-616: Trailing punctuation in heading Punctuation: ':' (MD026, no-trailing-punctuation) --- 636-636: Emphasis used instead of a heading (MD036, no-emphasis-as-heading) --- 638-638: Trailing punctuation in heading Punctuation: ':' (MD026, no-trailing-punctuation) </details> </details> <details> <summary>🪛 LanguageTool</summary> <details> <summary>docs/jwt.md</summary> [uncategorized] ~331-~331: If this is a compound adjective that modifies the following noun, use a hyphen. Context: ...tion checking 4. **Rate Limiting**: Add rate limiting middleware after authentication 5. **Au... (EN_COMPOUND_ADJECTIVE_INTERNAL) </details> </details> </details> <details> <summary>🔇 Additional comments (21)</summary><blockquote> <details> <summary>CLAUDE.md (4)</summary> `11-15`: **LGTM: Dependency versions updated for OAuth support.** The version updates are well-aligned with the OAuth 2.1 implementation: - Go 1.24.2+ ensures compatibility with latest features - MCP protocol v0.33.0 supports the new StreamableHTTP transport - JWT v5.2.2 enables secure token validation --- `77-77`: **LGTM: Transport description accurately reflects the new architecture.** The update from REST API with SSE to StreamableHTTP with backward-compatible SSE support correctly describes the modernized HTTP handling approach. --- `103-103`: **LGTM: Port standardization to 8080 is a sensible default.** The change from 9097 to 8080 aligns with common HTTP server conventions and improves usability. --- `139-142`: **LGTM: Manual testing instructions updated for new endpoints.** The testing instructions correctly reflect the new StreamableHTTP endpoint structure and maintain backward compatibility guidance. </details> <details> <summary>go.mod (1)</summary> `6-7`: **LGTM: Dependencies appropriately updated for OAuth 2.1 support.** The additions are well-chosen: - `golang-jwt/jwt/v5` v5.2.2 provides robust JWT token validation - `mark3labs/mcp-go` v0.33.0 enables the new StreamableHTTP transport Both dependencies are from reputable sources and use current stable versions. </details> <details> <summary>internal/config/config.go (4)</summary> `27-27`: **LGTM: OAuth configuration field properly integrated.** The `OAuthEnabled` field is well-positioned and follows the existing configuration structure conventions. --- `37-37`: **LGTM: Environment variable parsing follows established patterns.** The OAuth environment variable parsing uses the same pattern as other boolean configurations, ensuring consistency. --- `67-69`: **LGTM: Operational logging provides good visibility.** The informational logging when OAuth is enabled helps with deployment verification and troubleshooting. --- `83-83`: **LGTM: Configuration field properly assigned.** The `OAuthEnabled` field assignment completes the configuration integration correctly. </details> <details> <summary>internal/handlers/trino_handlers.go (6)</summary> `10-10`: **LGTM: Import added for tool registration functionality.** The server package import enables the new centralized tool registration feature. --- `29-34`: **LGTM: Type assertion adds robustness to argument handling.** The type assertion prevents runtime panics and provides clear error messages when arguments are malformed. This follows the coding guidelines for parameter validation. --- `85-90`: **LGTM: Consistent argument validation across handlers.** The same type assertion pattern is applied consistently, improving code reliability and error handling across all handlers. --- `118-123`: **LGTM: Parameter validation implemented consistently.** The validation pattern maintains consistency with other handlers and follows the established error handling approach. --- `154-159`: **LGTM: Robust argument handling for table schema operations.** The type assertion ensures safe parameter extraction for the critical table schema functionality. --- `197-216`: **LGTM: Centralized tool registration improves maintainability.** The `RegisterTrinoTools` function provides excellent abstraction by: - Centralizing all tool definitions in one place - Properly defining required vs optional parameters - Using descriptive tool names and descriptions - Following MCP tool registration patterns This aligns with the coding guidelines for MCP tool implementations. </details> <details> <summary>docs/oauth-config.md (1)</summary> `1-253`: **LGTM: Comprehensive and well-structured OAuth configuration guide.** This documentation excellently supports the OAuth 2.1 implementation by providing: **Strengths:** - Complete setup instructions for major OAuth providers (Google, Azure AD, Okta) - Clear environment variable configurations matching the code implementation - Security considerations covering JWT validation, HTTPS requirements, and access control - Practical testing and troubleshooting guidance - Both development and production deployment scenarios **Security Coverage:** - JWT signature validation using provider public keys - Token expiration and audience validation - HTTPS enforcement for OAuth endpoints - Comprehensive logging for audit trails **Operational Value:** - Step-by-step provider registration instructions - Environment variable examples for different scenarios - Migration guidance from basic authentication - Debug logging and health check procedures The documentation aligns perfectly with the `TRINO_OAUTH_ENABLED` configuration and OAuth middleware implementation. </details> <details> <summary>cmd/main.go (1)</summary> `48-66`: **Clean refactoring of server initialization.** The refactoring successfully consolidates HTTP server setup and OAuth handling into internal packages, improving separation of concerns and maintainability. </details> <details> <summary>README.md (1)</summary> `573-830`: **Excellent documentation of OAuth/JWT authentication features.** The authentication and transport documentation is comprehensive, covering: - Clear distinction between transport methods - Multiple JWT token generation examples for different languages - Important security considerations and best practices - Production deployment guidance This will greatly help users implement secure authentication. </details> <details> <summary>docs/jwt.md (1)</summary> `1-401`: **Well-structured JWT authentication documentation.** This documentation provides excellent coverage of: - Clear authentication flow with detailed sequence diagrams - Implementation details with code examples - Backward compatibility with legacy endpoints - Comprehensive testing instructions - Migration guidance The documentation will be invaluable for users implementing JWT authentication. </details> <details> <summary>internal/auth/oauth.go (1)</summary> `35-175`: **Well-structured OAuth middleware implementation.** The OAuth implementation demonstrates good practices: - Clean separation between tool-level and request-level authentication - Proper context propagation for user information - Comprehensive error handling and logging - Support for both enabled and disabled authentication modes Once the JWT signature verification is implemented, this will provide a solid authentication foundation. </details> <details> <summary>docs/oauth.md (1)</summary> `1-644`: **Exceptional OAuth implementation documentation.** This comprehensive documentation provides: - Clear architectural overview with multiple deployment options - Detailed sequence diagrams for different authentication flows - MCP specification compliance requirements - Security considerations including resource indicators (RFC 8707) - Implementation guidance with environment variables and examples This is exemplary technical documentation that will greatly assist implementers. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| // Context keys are now imported from auth package | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove the outdated comment about context keys.
The comment states that context keys are imported from the auth package, but there's no actual import of the auth package or usage of context keys in this file. This comment appears to be outdated after the refactoring.
-// Context keys are now imported from auth package
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Context keys are now imported from auth package |
🤖 Prompt for AI Agents
In cmd/main.go around lines 19 to 20, remove the outdated comment about context
keys being imported from the auth package, as there is no import or usage of
context keys in this file after refactoring.
| sseServer.ServeHTTP(w, r) | ||
| } | ||
| }), | ||
| port := getEnv("MCP_PORT", "8080") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the HTTP port change from 9097 to 8080.
The default port has been changed from 9097 to 8080. Please confirm this is intentional and update any related documentation or deployment configurations.
🏁 Script executed:
#!/bin/bash
# Description: Check for references to the old port 9097 in the codebase
rg -A 2 "9097"Length of output: 786
Update remaining 9097 references to 8080 across scripts, docs, and Docker configs
Multiple files still default to port 9097 and need to be aligned with the new 8080 default:
• scripts/run.sh
• examples/test_query.go
• docker-compose.yml
• SOLUTION.md
• Dockerfile
• Makefile
Please update any hard-coded “9097” values (environment variables, EXPOSE/mapping, example code, and documentation) to “8080” so everything stays consistent with the change in cmd/main.go.
🤖 Prompt for AI Agents
In cmd/main.go at line 62, the default port is updated to 8080, but other files
still reference 9097. Search through scripts/run.sh, examples/test_query.go,
docker-compose.yml, SOLUTION.md, Dockerfile, and Makefile for any hard-coded
"9097" values and replace them with "8080" to maintain consistency across all
configurations, environment variables, example code, and documentation.
internal/auth/oauth.go
Outdated
| func validateJWT(tokenString string) (*User, error) { | ||
| // Remove Bearer prefix if present | ||
| tokenString = strings.TrimPrefix(tokenString, "Bearer ") | ||
|
|
||
| // Parse JWT without verification for now (simplified) | ||
| // In production, you should verify with proper key | ||
| token, _, err := new(jwt.Parser).ParseUnverified(tokenString, jwt.MapClaims{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse token: %w", err) | ||
| } | ||
|
|
||
| claims, ok := token.Claims.(jwt.MapClaims) | ||
| if !ok { | ||
| return nil, fmt.Errorf("invalid token claims") | ||
| } | ||
|
|
||
| // Extract user information | ||
| user := &User{ | ||
| Subject: getStringClaim(claims, "sub"), | ||
| Username: getStringClaim(claims, "preferred_username"), | ||
| Email: getStringClaim(claims, "email"), | ||
| } | ||
|
|
||
| if user.Subject == "" { | ||
| return nil, fmt.Errorf("missing subject in token") | ||
| } | ||
|
|
||
| return user, nil | ||
| } |
There was a problem hiding this comment.
Critical security vulnerability: JWT signature verification is disabled.
The validateJWT function uses ParseUnverified, which skips signature verification. This allows anyone to forge tokens and bypass authentication. This MUST be fixed before production deployment.
Implement proper JWT verification:
-func validateJWT(tokenString string) (*User, error) {
+func validateJWT(tokenString string, keyFunc jwt.Keyfunc) (*User, error) {
// Remove Bearer prefix if present
tokenString = strings.TrimPrefix(tokenString, "Bearer ")
- // Parse JWT without verification for now (simplified)
- // In production, you should verify with proper key
- token, _, err := new(jwt.Parser).ParseUnverified(tokenString, jwt.MapClaims{})
+ // Parse and verify JWT
+ token, err := jwt.Parse(tokenString, keyFunc, jwt.WithValidMethods([]string{"RS256", "ES256"}))
if err != nil {
return nil, fmt.Errorf("failed to parse token: %w", err)
}
+
+ if !token.Valid {
+ return nil, fmt.Errorf("invalid token")
+ }You'll need to implement a proper keyFunc that fetches the signing key from your OAuth provider's JWKS endpoint.
Would you like me to help implement proper JWT verification with JWKS support?
🤖 Prompt for AI Agents
In internal/auth/oauth.go around lines 82 to 110, the validateJWT function
currently uses ParseUnverified, which skips JWT signature verification and poses
a critical security risk. To fix this, replace ParseUnverified with jwt.Parse
and provide a keyFunc callback that retrieves the signing key from your OAuth
provider's JWKS endpoint for proper signature verification. Implement the
keyFunc to fetch and cache the JWKS keys, then use it in jwt.Parse to validate
the token's signature before extracting claims.
|
@claude review PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
README.md (1)
880-886: Same token placeholder issue as above.Replicate the fix here to avoid repeating the leak warning.
🧹 Nitpick comments (3)
Makefile (1)
79-79: Declare thealltarget as PHONY to silence checkmake.Static analysis flagged this; add the target to the existing
.PHONYlist.-.PHONY: build build-dxt pack-dxt test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test +.PHONY: build build-dxt pack-dxt test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test alldocker-compose.yml (1)
42-42: Strip trailing whitespace.YAMLlint flagged a stray space; keeping the file clean avoids noisy diffs later.
- driver: bridge + driver: bridgeREADME.md (1)
722-727: Avoid false Gitleaks hits in samplecurlcommands.The static scan is triggered by literal
Authorization: Bearer YOUR_JWT_TOKEN.
Switch to a placeholder that clearly isn’t a secret (e.g.<JWT_TOKEN>) or wrap it in back-ticks so secret-scanners ignore it.-Authorization: Bearer YOUR_JWT_TOKEN +Authorization: Bearer <JWT_TOKEN>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Dockerfile(1 hunks)Makefile(2 hunks)README.md(7 hunks)SOLUTION.md(2 hunks)docker-compose.yml(2 hunks)scripts/run.sh(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- SOLUTION.md
- Dockerfile
- scripts/run.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented in `internal/handlers/trino_handlers.go`.
README.md (9)
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to cmd/main.go : Main entry point for the server should be implemented in `cmd/main.go` with server initialization, Trino connection testing, transport selection (STDIO vs HTTP with SSE), graceful shutdown, and CORS support.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment variables such as `TRINO_HOST`, `TRINO_PORT`, `TRINO_USER`, `TRINO_PASSWORD`, `TRINO_SCHEME`, `TRINO_SSL`, `TRINO_SSL_INSECURE`, `TRINO_ALLOW_WRITE_QUERIES`, `TRINO_QUERY_TIMEOUT`, `MCP_TRANSPORT`, `MCP_PORT`, and `MCP_HOST` must be supported and validated in configuration logic.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented in `internal/handlers/trino_handlers.go`.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (`execute_query`, `list_catalogs`, `list_schemas`, `list_tables`, `get_table_schema`) must return JSON-formatted responses and handle parameter validation.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.735Z
Learning: Applies to internal/trino/client.go : Connection pool settings should be: 10 max open, 5 max idle, 5min max lifetime for Trino client connections.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented in `internal/trino/client.go`.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : HTTPS scheme must force SSL=true regardless of the `TRINO_SSL` setting in configuration logic.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment-based configuration with validation, security defaults (HTTPS, read-only queries), and timeout configuration with validation should be implemented in `internal/config/config.go`.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : The `isReadOnlyQuery()` function in `internal/trino/client.go` should allow only SELECT, SHOW, DESCRIBE, EXPLAIN, and WITH (CTEs) queries by default, blocking INSERT, UPDATE, DELETE, CREATE, DROP, and ALTER unless `TRINO_ALLOW_WRITE_QUERIES=true` is set.
🪛 checkmake (0.2.2)
Makefile
[warning] 79-79: Target "all" should be declared PHONY.
(phonydeclared)
🪛 Gitleaks (8.27.2)
README.md
722-723: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
880-882: Discovered a potential authorization token provided in a curl command header, which could compromise the curl accessed resource.
(curl-auth-header)
🪛 markdownlint-cli2 (0.17.2)
README.md
622-622: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
636-636: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
653-653: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
673-673: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
704-704: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
928-928: Blank line inside blockquote
(MD028, no-blanks-blockquote)
930-930: Blank line inside blockquote
(MD028, no-blanks-blockquote)
932-932: Blank line inside blockquote
(MD028, no-blanks-blockquote)
934-934: Blank line inside blockquote
(MD028, no-blanks-blockquote)
936-936: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🪛 YAMLlint (1.37.1)
docker-compose.yml
[error] 42-42: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
docker-compose.yml (1)
33-33: Double-checkMCP_PORTafter changing the host binding.If you adopt the fix above, the in-container port (still 8080) is fine, but external docs & env examples that point clients at
http://localhost:8080will need to be updated to9097(or whichever host port you pick). Make sure README and integration snippets stay consistent.
| @@ -52,7 +52,7 @@ run: | |||
| # Build and run Docker image | |||
| run-docker: build | |||
| docker build -t $(BINARY_NAME):$(VERSION) . | |||
| docker run -p 9097:9097 $(BINARY_NAME):$(VERSION) | |||
| docker run -p 8080:8080 $(BINARY_NAME):$(VERSION) | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make the published port configurable & verify downstream scripts.
Hard-coding the mapping to 8080:8080 breaks anyone who still relies on the previous 9097 default and makes it easy to introduce a clash with projects already listening on 8080.
Recommend exposing the port via a make variable (e.g. HOST_PORT ?= 8080) and verifying that no scripts / docs still reference 9097.
🏁 Script executed:
#!/bin/bash
# Find hard-coded 9097 references that may now be stale
rg -n --no-heading '9097'Length of output: 66
Parameterize Docker host port & update stale 9097 reference
To avoid hard-coding 8080:8080 (which breaks existing 9097 defaults and risks port clashes), introduce Makefile variables and update downstream references:
-
In
Makefile, add at the top:HOST_PORT ?= 8080 CONTAINER_PORT ?= 8080 -
Replace the
docker runline (around line 55) with:docker run -p $(HOST_PORT):$(CONTAINER_PORT) $(BINARY_NAME):$(VERSION) -
Update any hard-coded
9097usages, e.g. inexamples/test_query.go(line 15), change:port = "9097"to either use the new
HOST_PORTdefault or document the override.
Fixing these will ensure the port is configurable and no scripts/docs remain pointing at the old default.
🤖 Prompt for AI Agents
In the Makefile at line 55, replace the hard-coded docker port mapping
"8080:8080" with parameterized variables by first adding "HOST_PORT ?= 8080" and
"CONTAINER_PORT ?= 8080" at the top of the Makefile, then updating the docker
run command to use "$(HOST_PORT):$(CONTAINER_PORT)". Additionally, update any
hard-coded references to port 9097, such as in examples/test_query.go at line
15, to use the new HOST_PORT variable or document how to override it, ensuring
all port usages are consistent and configurable.
| ports: | ||
| - '9097:9097' | ||
| - '8080:8080' | ||
| environment: |
There was a problem hiding this comment.
Port collision: both services bind host :8080 – Docker Compose will fail.
trino already maps 8080:8080; mapping trino-mcp to the same host port causes a runtime error.
Choose a different host port for one of them (e.g. keep Trino on 8080, expose MCP on 9097 or 8081).
- - '8080:8080'
+ # Avoid clash with the Trino UI
+ - '9097:8080'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ports: | |
| - '9097:9097' | |
| - '8080:8080' | |
| environment: | |
| ports: | |
| # Avoid clash with the Trino UI | |
| - '9097:8080' | |
| environment: |
🤖 Prompt for AI Agents
In docker-compose.yml around lines 24 to 26, both the trino and trino-mcp
services map the host port 8080, causing a port collision. To fix this, change
the host port mapping for trino-mcp to a different port such as 9097 or 8081
while keeping the container port as 8080. This will prevent Docker Compose from
failing due to port conflicts.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitignore(1 hunks)internal/server/http.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented in `internal/handlers/trino_handlers.go`.
internal/server/http.go (4)
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to cmd/main.go : Main entry point for the server should be implemented in `cmd/main.go` with server initialization, Trino connection testing, transport selection (STDIO vs HTTP with SSE), graceful shutdown, and CORS support.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented in `internal/handlers/trino_handlers.go`.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (`execute_query`, `list_catalogs`, `list_schemas`, `list_tables`, `get_table_schema`) must return JSON-formatted responses and handle parameter validation.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/trino/client.go : Database connection management, connection pooling, SQL injection protection via read-only query enforcement, and context-based timeout handling for queries should be implemented in `internal/trino/client.go`.
🧬 Code Graph Analysis (1)
internal/server/http.go (4)
internal/config/config.go (1)
TrinoConfig(12-28)internal/auth/oauth.go (2)
CreateHTTPContextFunc(127-146)CreateRequestAuthHook(149-175)internal/trino/client.go (1)
Client(17-21)internal/handlers/trino_handlers.go (2)
NewTrinoHandlers(20-24)RegisterTrinoTools(198-216)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Security Scan
🔇 Additional comments (9)
internal/server/http.go (9)
1-16: LGTM! Clean package structure and appropriate imports.The imports are well-organized and all appear to be used in the implementation.
18-23: Well-structured server type definition.The HTTPServer struct properly encapsulates its dependencies with unexported fields, following Go best practices for encapsulation.
25-32: Clean constructor implementation.The constructor follows the standard Go pattern for creating new instances.
126-131: Clean status endpoint implementation.The status handler is simple and effective, providing basic health check functionality.
133-139: Proper signal handling for graceful shutdown.The signal handling correctly captures interrupt and termination signals.
141-155: Excellent OAuth status reporting with security awareness.The distinction between HTTPS and HTTP warnings for OAuth is well-implemented and helps maintain security awareness.
157-175: Well-structured MCP server initialization with conditional OAuth.The server creation properly integrates OAuth hooks when enabled and registers Trino tools appropriately.
177-180: Clean STDIO transport wrapper.Good to maintain support for STDIO transport alongside HTTP.
182-188: Standard environment variable helper.Clean implementation of a common utility pattern.
| func (s *HTTPServer) createMCPHandler(streamableServer *mcpserver.StreamableHTTPServer) http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| // Add CORS headers | ||
| w.Header().Set("Access-Control-Allow-Origin", "*") | ||
| w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS") | ||
| w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization") | ||
|
|
||
| if r.Method == "OPTIONS" { | ||
| w.WriteHeader(http.StatusOK) | ||
| return | ||
| } | ||
|
|
||
| log.Printf("MCP %s %s", r.Method, r.URL.Path) | ||
|
|
||
| // Handle MCP request using StreamableHTTP server | ||
| streamableServer.ServeHTTP(w, r) | ||
| } | ||
| } |
There was a problem hiding this comment.
Make CORS configuration environment-aware for production security.
The current CORS configuration allows all origins (*), which is a security risk in production. Consider making this configurable:
func (s *HTTPServer) createMCPHandler(streamableServer *mcpserver.StreamableHTTPServer) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
// Add CORS headers
- w.Header().Set("Access-Control-Allow-Origin", "*")
+ allowedOrigin := getEnv("CORS_ALLOWED_ORIGIN", "*")
+ w.Header().Set("Access-Control-Allow-Origin", allowedOrigin)
w.Header().Set("Access-Control-Allow-Methods", "GET, POST, DELETE, OPTIONS")
w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization")This allows restricting CORS in production while maintaining flexibility for development.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/server/http.go around lines 107 to 124, the CORS headers currently
allow all origins with "*", which is insecure for production. Modify the code to
read the allowed origins from an environment variable or configuration setting
instead of hardcoding "*". Use this configured value to set the
"Access-Control-Allow-Origin" header, enabling restriction of allowed origins in
production while keeping flexibility for development environments.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/auth/oauth.go (1)
176-182: Suggest defensive programming: Add nil check for claims map.The
getStringClaimfunction could be more robust by adding a nil check for the claims map parameter.func getStringClaim(claims jwt.MapClaims, key string) string { + if claims == nil { + return "" + } if val, ok := claims[key].(string); ok { return val } return "" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/auth/oauth.go(1 hunks)internal/config/config.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/config.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented in `internal/handlers/trino_handlers.go`.
internal/auth/oauth.go (3)
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented in `internal/handlers/trino_handlers.go`.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment-based configuration with validation, security defaults (HTTPS, read-only queries), and timeout configuration with validation should be implemented in `internal/config/config.go`.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : All MCP tools (`execute_query`, `list_catalogs`, `list_schemas`, `list_tables`, `get_table_schema`) must return JSON-formatted responses and handle parameter validation.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Verification
🔇 Additional comments (2)
internal/auth/oauth.go (2)
95-110: LGTM: JWT signature verification properly implemented.The JWT validation now correctly uses
jwt.Parsewith signature verification, addressing the critical security vulnerability from the previous review. The implementation properly validates the signing method and token validity.
136-166: Well-implemented: Standard JWT claims validation.The token claims validation properly checks expiration, not-before, and issued-at claims with appropriate time comparisons. The error messages are clear and specific.
internal/auth/oauth.go
Outdated
| // Log token for debugging (first 50 chars) | ||
| tokenPreview := tokenString | ||
| if len(tokenString) > 50 { | ||
| tokenPreview = tokenString[:50] + "..." | ||
| } | ||
| log.Printf("OAuth: Received token: %s", tokenPreview) |
There was a problem hiding this comment.
Security concern: Token logging exposes sensitive information.
Logging token previews, lengths, and authorization headers can expose sensitive authentication data in production logs, potentially creating security vulnerabilities.
Remove or conditionally disable token logging in production:
- // Log token for debugging (first 50 chars)
- tokenPreview := tokenString
- if len(tokenString) > 50 {
- tokenPreview = tokenString[:50] + "..."
- }
- log.Printf("OAuth: Received token: %s", tokenPreview)
+ // Log token presence only (no content)
+ log.Printf("OAuth: Token received for tool: %s", req.Params.Name)Apply similar changes to lines 200, 206, and 223-227 to avoid logging sensitive token data.
Also applies to: 200-200, 206-206, 223-227
🤖 Prompt for AI Agents
In internal/auth/oauth.go around lines 53 to 58, remove or conditionally disable
the logging of token previews to prevent exposing sensitive authentication data
in production logs. Implement an environment or configuration check to ensure
token logging only occurs in non-production environments. Apply the same
conditional logging approach to lines 200, 206, and 223-227 to avoid logging
sensitive token information throughout the file.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/auth/oauth.go (2)
64-69: Remove token logging to prevent security exposure.The code still logs token previews which can expose sensitive authentication data in production logs, creating security vulnerabilities.
Apply this diff to remove sensitive token logging:
- // Log token for debugging (first 50 chars) - tokenPreview := tokenString - if len(tokenString) > 50 { - tokenPreview = tokenString[:50] + "..." - } - log.Printf("OAuth: Received token for %s: %s", operation, tokenPreview) + // Log token presence only (no content) + log.Printf("OAuth: Token received for %s", operation)
221-228: Remove authorization header logging to prevent security exposure.Similar to the token preview logging, logging authorization headers can expose sensitive authentication data.
Apply this diff to remove sensitive header logging:
- } else if authHeader != "" { - preview := authHeader - if len(authHeader) > 30 { - preview = authHeader[:30] + "..." - } - log.Printf("OAuth: Invalid Authorization header format: %s", preview) - } + } else if authHeader != "" { + log.Printf("OAuth: Invalid Authorization header format detected") + }
🧹 Nitpick comments (2)
internal/auth/oauth.go (2)
164-194: Improve time comparison precision for JWT claims validation.The current time validation uses
time.Now().Unix()which provides second-level precision, but JWTexp,nbf, andiatclaims typically use Unix timestamps. Consider using higher precision or adding a small tolerance buffer to handle clock skew.Consider this improvement for more robust time validation:
func validateTokenClaims(claims jwt.MapClaims) error { + now := time.Now() + // Add 30 second tolerance for clock skew + tolerance := time.Duration(30) * time.Second + // Validate expiration if exp, ok := claims["exp"]; ok { if expTime, ok := exp.(float64); ok { - if time.Now().Unix() > int64(expTime) { + if now.After(time.Unix(int64(expTime), 0).Add(tolerance)) { return fmt.Errorf("token expired") } } } // Validate not before if nbf, ok := claims["nbf"]; ok { if nbfTime, ok := nbf.(float64); ok { - if time.Now().Unix() < int64(nbfTime) { + if now.Before(time.Unix(int64(nbfTime), 0).Add(-tolerance)) { return fmt.Errorf("token not yet valid") } } } // Validate issued at (should not be in the future) if iat, ok := claims["iat"]; ok { if iatTime, ok := iat.(float64); ok { - if time.Now().Unix() < int64(iatTime) { + if now.Before(time.Unix(int64(iatTime), 0).Add(-tolerance)) { return fmt.Errorf("token issued in the future") } } }
152-155: Consider making username extraction more robust.The current implementation extracts
preferred_usernamewhich may not be present in all JWT tokens. Consider falling back to other common username claims.Consider this more robust username extraction:
// Extract user information user := &User{ Subject: getStringClaim(claims, "sub"), - Username: getStringClaim(claims, "preferred_username"), + Username: getUsernameClaim(claims), Email: getStringClaim(claims, "email"), }And add a helper function:
// getUsernameClaim extracts username from various possible claims func getUsernameClaim(claims jwt.MapClaims) string { // Try preferred_username first (OIDC standard) if username := getStringClaim(claims, "preferred_username"); username != "" { return username } // Fall back to common alternatives if username := getStringClaim(claims, "username"); username != "" { return username } if username := getStringClaim(claims, "name"); username != "" { return username } // Last resort: use subject return getStringClaim(claims, "sub") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/auth/oauth.go(1 hunks)internal/server/http.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/server/http.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented in `internal/handlers/trino_handlers.go`.
internal/auth/oauth.go (2)
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/handlers/trino_handlers.go : MCP tool implementations with JSON response formatting, parameter validation, error handling, and consistent logging for debugging should be implemented in `internal/handlers/trino_handlers.go`.
Learnt from: CR
PR: tuannvm/mcp-trino#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-01T05:50:01.734Z
Learning: Applies to internal/config/config.go : Environment-based configuration with validation, security defaults (HTTPS, read-only queries), and timeout configuration with validation should be implemented in `internal/config/config.go`.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Verification
🔇 Additional comments (4)
internal/auth/oauth.go (4)
112-162: Excellent security improvement: JWT signature verification is now properly implemented.The JWT validation now uses proper signature verification with
jwt.Parseinstead ofParseUnverified, addressing the critical security vulnerability from previous reviews. The implementation correctly validates the signing method, checks token validity, and validates standard claims.
55-82: Good architectural improvement: Authentication logic is now consolidated.The shared
authenticateRequestfunction successfully eliminates code duplication betweenOAuthMiddlewareandCreateRequestAuthHook, addressing the architectural concern from previous reviews.
32-42: Excellent security improvement: JWT secret is now properly cached and validated.The JWT secret caching with
sync.Onceand mandatory configuration requirement addresses the performance and security issues from previous reviews. This eliminates the inefficient environment variable lookups and removes the insecure default fallback.
233-240: Server-level hook only used—no duplication
CreateRequestAuthHook is the only OAuth enforcement path (registered in internal/server/http.go when OAuthEnabled is true). The tool-level OAuthMiddleware is defined in internal/auth/oauth.go but never applied in RegisterTrinoTools, so there’s no overlap or conflict.
| token, err := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) { | ||
| // Validate signing method | ||
| if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { | ||
| return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) | ||
| } | ||
| return []byte(secret), nil | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider supporting multiple JWT signing algorithms.
The current implementation only accepts HMAC signing methods. For production OAuth 2.1 implementations, RSA and ECDSA algorithms (RS256, ES256) are more commonly used and provide better security properties.
Consider supporting additional signing algorithms:
// Parse and validate JWT with signature verification
token, err := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) {
- // Validate signing method
- if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
- return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
- }
- return []byte(secret), nil
+ // Validate signing method
+ switch token.Method.(type) {
+ case *jwt.SigningMethodHMAC:
+ return []byte(secret), nil
+ case *jwt.SigningMethodRSA:
+ // Load RSA public key from JWKS endpoint or configuration
+ return loadRSAPublicKey()
+ case *jwt.SigningMethodECDSA:
+ // Load ECDSA public key from JWKS endpoint or configuration
+ return loadECDSAPublicKey()
+ default:
+ return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"])
+ }
})Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/auth/oauth.go around lines 124 to 130, the JWT parsing function
currently only supports HMAC signing methods, which limits security options.
Modify the function to also accept RSA and ECDSA algorithms such as RS256 and
ES256 by checking the token's signing method against these types and returning
the appropriate public key for verification. This will involve adding
conditional checks for these algorithms and handling their key retrieval
accordingly.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
PR: Implement OAuth 2.1 Authentication and Enhance Trino Tool Integration
Summary
This pull request introduces comprehensive OAuth 2.1 authentication support into the MCP server. Key updates include secure token extraction from HTTP requests, token validation, user context management, and configuration for enabling/disabling OAuth. Additionally, the Trino tool handlers are enhanced to robustly parse and validate parameters, facilitating flexible and error-resistant interactions.
Affected Modules
GetOAuthToken,OAuthMiddleware,validateJWT,CreateHTTPContextFunc,CreateRequestAuthHook, and related user context management.internal/config/config.goto includeOAuthEnabledflag.internal/handlers/trino_handlers.gowith better argument parsing and parameter validation.RegisterTrinoToolsupdated to properly define expected parameters with descriptions and requirements.Key Details
OAuthEnabledconfiguration flag toggled by environment variableTRINO_OAUTH_ENABLED.OAuthMiddlewareto intercept requests, extract and validate OAuth tokens, and inject user info into context.Authorizationheaders during HTTP requests.map[string]interface{}with null checks, reducing runtime errors.Potential Impacts
Testing Steps
TRINO_OAUTH_ENABLED=true, restart server.execute_query,list_catalogs,list_schemas,list_tables, andget_table_schemawith correct and incorrect parameters.This comprehensive update improves security, flexibility, and robustness of MCP Trino integration with OAuth 2.1 support.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores