Feat/sso oauth support #63
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>
…iles 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 7 minutes and 29 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 refactors the MCP server initialization and HTTP handling to a new consolidated Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPServer
participant OAuth2Handler
participant TokenValidator
participant TrinoMCPServer
Client->>HTTPServer: Request (with Bearer token)
HTTPServer->>TokenValidator: ValidateToken(token)
TokenValidator-->>HTTPServer: User info or error
alt Valid token
HTTPServer->>TrinoMCPServer: Handle MCP request (with user context)
TrinoMCPServer-->>HTTPServer: Response
HTTPServer-->>Client: Response
else Invalid token
HTTPServer-->>Client: 401 Unauthorized + OAuth metadata
end
sequenceDiagram
participant Client
participant HTTPServer
participant OAuth2Handler
participant OIDCProvider
Client->>HTTPServer: GET /oauth/authorize
HTTPServer->>OAuth2Handler: HandleAuthorize
OAuth2Handler-->>Client: Redirect to OIDCProvider
Client->>OIDCProvider: Authorize (login)
OIDCProvider-->>Client: Redirect (code)
Client->>HTTPServer: GET /oauth/callback?code=...
HTTPServer->>OAuth2Handler: HandleCallback
OAuth2Handler->>OIDCProvider: Exchange code for token
OIDCProvider-->>OAuth2Handler: Token(s)
OAuth2Handler-->>Client: Success page or proxy redirect
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
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: 15
🔭 Outside diff range comments (1)
docs/oauth.md (1)
30-38: Wrong well-known endpoint pathRFC 8414 specifies
/.well-known/oauth-authorization-server(or/.well-known/openid-configuration) for AS metadata.
The bullet at line 36 introduces a non-standard/.well-known/oauth-metadatapath that clients are unlikely to recognise.- - **OAuth Metadata Endpoint**: `/.well-known/oauth-metadata` endpoint for MCP client discovery + - **OAuth Metadata Endpoint**: `/.well-known/oauth-authorization-server` endpoint for MCP client discovery
🧹 Nitpick comments (8)
internal/config/config.go (1)
28-36: Consider using structured types for OAuth providersThe OAuth provider is stored as a string and validated through string comparison. Consider using an enum or constant set to ensure type safety and prevent invalid provider values.
+// OAuthProvider represents supported OAuth providers +type OAuthProvider string + +const ( + OAuthProviderHMAC OAuthProvider = "hmac" + OAuthProviderOkta OAuthProvider = "okta" + OAuthProviderGoogle OAuthProvider = "google" + OAuthProviderAzure OAuthProvider = "azure" +) type TrinoConfig struct { // OAuth mode configuration OAuthEnabled bool // Enable OAuth 2.1 authentication - OAuthProvider string // OAuth provider: "hmac", "okta", "google", "azure" + OAuthProvider OAuthProvider // OAuth provider type JWTSecret string // JWT signing secret for HMAC providerinternal/oauth/middleware.go (1)
156-184: Deprecate or remove unused JWT validation codeThe
validateTokenClaimsfunction is marked as deprecated but still present. If provider-based validation is the intended approach, this code should be removed to avoid confusion and reduce maintenance burden.Consider removing this entire function if it's truly deprecated and not used anywhere in the codebase.
internal/oauth/metadata.go (1)
186-187: Security: Avoid exposing empty client secretEven though the comment indicates it's a public client, explicitly setting an empty string for
client_secretin the response could be confusing. Consider omitting the field entirely for public clients.response := map[string]interface{}{ "client_id": h.config.ClientID, - "client_secret": "", // Public client, no secret "client_id_issued_at": time.Now().Unix(),internal/oauth/handlers.go (1)
71-74: HTTPS detection logic is incompleteThe scheme detection only checks for certificate files but doesn't consider other HTTPS configurations like reverse proxies or environment-specific settings.
// Determine scheme based on HTTPS configuration scheme := "http" if getEnv("HTTPS_CERT_FILE", "") != "" && getEnv("HTTPS_KEY_FILE", "") != "" { scheme = "https" } +// Also check for common HTTPS indicators +if getEnv("HTTPS", "") == "true" || getEnv("TLS", "") == "true" { + scheme = "https" +}docs/oauth.md (4)
309-317: Trailing colon in heading trips Markdown-lint
markdownlintflags MD026 here. Drop the trailing:to avoid a lint error and improve heading readability.-### 2. Bearer Token Validation (`internal/mcp/oauth.go`): +### 2. Bearer Token Validation (`internal/mcp/oauth.go`)
326-333: Same colon / MD026 issue for heading 4-### 4. HTTP Authentication Implementation (`internal/mcp/oauth.go`): +### 4. HTTP Authentication Implementation (`internal/mcp/oauth.go`)Also consider collapsing the six single-sentence bullets into one paragraph to reduce vertical bloat.
705-766: Function names missing from surrounding package
applyOAuthMiddlewarecallssetOAuthMiddleware, which is not declared in the snippet or earlier listings. Either rename to the exported setter that actually exists inmcp-go, or show its definition to avoid confusion for readers following the guide.
894-978: Hard-coded version string
"mcp_version": "1.0.0"and"server_version": "%s"are baked into the example.
Consider replacing the literal with aconstor param in real code snippets so readers don’t copy stale versions.- "mcp_version": "1.0.0", + "mcp_version": mcp.Version,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
cmd/main.go(2 hunks)docs/oauth.md(7 hunks)go.mod(1 hunks)internal/auth/oauth.go(0 hunks)internal/config/config.go(4 hunks)internal/mcp/handlers.go(2 hunks)internal/mcp/server.go(1 hunks)internal/oauth/config.go(1 hunks)internal/oauth/handlers.go(1 hunks)internal/oauth/metadata.go(1 hunks)internal/oauth/middleware.go(1 hunks)internal/oauth/providers.go(1 hunks)internal/server/http.go(0 hunks)
💤 Files with no reviewable changes (2)
- internal/auth/oauth.go
- internal/server/http.go
🧰 Additional context used
📓 Path-based instructions (2)
internal/config/config.go
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
cmd/main.go
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (12)
📓 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`.
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.
go.mod (6)
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 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/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/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/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.735Z
Learning: Applies to examples/test_query.go : The file `examples/test_query.go` should be used for HTTP API manual testing.
internal/mcp/handlers.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/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.
internal/oauth/config.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/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 : 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/oauth/providers.go (1)
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`.
internal/config/config.go (6)
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/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/config/config.go : Invalid timeout values for `TRINO_QUERY_TIMEOUT` must 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 : 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/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.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.
cmd/main.go (6)
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/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/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 : 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`.
internal/oauth/metadata.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/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.
internal/oauth/handlers.go (5)
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/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/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.735Z
Learning: Applies to examples/test_query.go : The file `examples/test_query.go` should be used for HTTP API manual testing.
internal/oauth/middleware.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/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.
internal/mcp/server.go (6)
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 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 : 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`.
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/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`.
docs/oauth.md (5)
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/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.735Z
Learning: Applies to examples/test_query.go : The file `examples/test_query.go` should 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 : 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 (5)
internal/mcp/handlers.go (1)
internal/mcp/server.go (1)
GetOAuthMiddleware(395-397)
internal/oauth/providers.go (2)
internal/oauth/middleware.go (1)
User(146-150)internal/config/config.go (1)
TrinoConfig(12-37)
cmd/main.go (1)
internal/mcp/server.go (2)
NewServer(30-46)ServeStdio(340-342)
internal/oauth/handlers.go (2)
cmd/main.go (1)
Version(16-16)internal/config/config.go (1)
TrinoConfig(12-37)
internal/oauth/middleware.go (1)
internal/oauth/providers.go (1)
TokenValidator(18-21)
🪛 markdownlint-cli2 (0.17.2)
docs/oauth.md
1044-1044: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
1045-1045: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
1046-1046: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
1047-1047: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
1048-1048: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
1049-1049: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
1050-1050: Unordered list indentation
Expected: 0; Actual: 3
(MD007, ul-indent)
1233-1233: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
1235-1235: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
1248-1248: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🔇 Additional comments (7)
go.mod (2)
6-10: ✅ Dependency Security Check PassedI’ve verified the OAuth/OIDC dependencies against GitHub’s Security Advisories:
- github.com/coreos/go-oidc/v3 v3.14.1 – no known vulnerabilities
- golang.org/x/oauth2 v0.30.0 – no known vulnerabilities
These versions are safe to use.
3-3: go.mod: Go version 1.24.2 is valid
- go.mod (line 3):
go 1.24.2is an existing patch release in the 1.24.x series.- The latest patch release is 1.24.5 (July 8, 2025). You may bump to 1.24.5 if you want the newest security and bug fixes.
Likely an incorrect or invalid review comment.
cmd/main.go (1)
9-9: Refactoring Approved: OAuth Initialization ConfirmedThe move to internal/mcp and the simplified
NewServersignature cleanly consolidates server setup. We’ve verified that:
NewServercreates anoauthHandleronly whentrinoConfig.OAuthEnabledis true.createMCPServerconditionally registers all OAuth endpoints on the HTTP mux (RFC 8414, RFC 9728, legacy metadata,/oauth/*, and/callback) when OAuth is enabled.No further changes required.
internal/mcp/handlers.go (1)
199-237: Well-implemented OAuth middleware integrationThe conditional application of OAuth middleware to all Trino tool handlers is implemented correctly. The approach maintains backward compatibility when OAuth is disabled while ensuring all tools are protected when enabled. The use of a helper function
applyMiddlewaremakes the code clean and maintainable.internal/oauth/providers.go (2)
94-136: Excellent OIDC security implementationThe OIDC validator implementation follows security best practices:
- Enforces TLS 1.2 minimum version
- Enables all security verification checks (audience, expiry, issuer)
- Uses appropriate timeouts for HTTP operations
- Properly configures connection pooling
76-84: Helper functions verified and no action neededBoth
validateTokenClaimsandgetStringClaimare implemented ininternal/oauth/middleware.go(lines 155–184 and 188–193). No further changes are required here.docs/oauth.md (1)
584-626: Undefined helper referenced in snippetThe sample
OAuthMiddlewarerefers to an externalGetOAuthToken(ctx)helper, but that helper is neither shown nor described anywhere in the doc. Readers cannot reproduce the code as-is.Add a short stub or at least a one-liner describing the expected behaviour / signature of
GetOAuthTokenso copy-paste compilation succeeds.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
README.md (2)
93-95: Feature bullet mixes JWT wording with new OAuth scope
StreamableHTTP support with JWT authentication (upgraded from SSE)reads as if JWT-only auth is available, yet the PR introduces full OAuth/OIDC with JWT validation. Recommend clarifying the bullet, e.g.:- ✅ StreamableHTTP support with JWT authentication (upgraded from SSE) + ✅ StreamableHTTP support with OAuth 2.0 / JWT bearer authentication (replaces legacy SSE)
876-884: Missing defaults & required flags for new OAuth variablesThe env-var table omits default values or “required” markers for
OAUTH_PROVIDER,OIDC_ISSUER,OIDC_AUDIENCE, andOIDC_CLIENT_ID. Omitting these can trip up users. Suggest marking them as “(required when TRINO_OAUTH_ENABLED=true)” and adding defaults where applicable.docs/jwt.md (2)
63-71: Variable naming still references OAuth, may confuse readersUsing
TRINO_OAUTH_ENABLEDto switch on pure HMAC-JWT auth feels counter-intuitive after the OAuth split. Consider an alias or note, e.g. “TRINO_OAUTH_ENABLED(also controls standalone JWT mode)”.
140-143: Fenced code block is missing a language identifier (MD040)Markdown-lint flags this; add
bash,json, or another appropriate tag to keep CI green.-```json +```jsondocs/oauth.md (2)
11-24: Add language tag to ASCII diagram to satisfy markdown-lintCI raises MD040; simply mark the block as plain text:
-``` +```text ┌─────────────────┐ ...
66-68: Typo: “backward compati…”Small grammar issue flagged by LanguageTool: “backward compatibility” is missing the full word.
- with backward compati... + with backward compatibility...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(5 hunks)docs/jwt.md(3 hunks)docs/oauth-config.md(0 hunks)docs/oauth.md(1 hunks)
💤 Files with no reviewable changes (1)
- docs/oauth-config.md
🧰 Additional context used
🧠 Learnings (4)
📓 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`.
docs/jwt.md (5)
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`.
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 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.
README.md (7)
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 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/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 : 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/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.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.
docs/oauth.md (7)
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 file `examples/test_query.go` should 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 : 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 : 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.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 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.
🪛 LanguageTool
docs/oauth.md
[grammar] ~66-~66: Ensure spelling is correct
Context: ...and other clients - HTTP Transport: StreamableHTTP endpoint (/mcp) with backward compati...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.17.2)
docs/oauth.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (3)
README.md (1)
40-44: “OAuth Middleware” label is misleading given the new server-level auth-hook architectureThe diagram still shows a dedicated
OAuth Middlewarenode, but the codebase moved validation into a server-level hook (no separate middleware layer). Updating the label to “Auth Hook (JWT/OIDC)” or similar would avoid confusion for future readers.docs/jwt.md (1)
55-58: Claims list incomplete vs. flow diagramThe security section lists required claims
(sub, exp, iat)but the flow diagram and examples referencepreferred_usernameanddocs/oauth.md (1)
52-57: Confirm PKCE support is truly implementedDoc advertises “Full OAuth 2.1 PKCE implementation”, but I can’t find PKCE verifier/challenge handling in
internal/oauth/*. If that work is planned but not merged, please tone down the claim or link to the code.
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
|
@claude review PR |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (8)
internal/oauth/metadata.go (3)
17-20: Duplicate HTTP status setting causes malformed responseThe code calls both
WriteHeaderandhttp.Errorwhich sets the status twice, resulting in a malformed response.Apply this fix:
if r.Method != "GET" { - w.WriteHeader(http.StatusMethodNotAllowed) http.Error(w, `{"error":"Method not allowed"}`, http.StatusMethodNotAllowed) return }
80-83: Duplicate HTTP status setting causes malformed responseSame issue as in HandleMetadata - calling both
WriteHeaderandhttp.Errorcauses duplicate status setting.Apply this fix:
if r.Method != "GET" { - w.WriteHeader(http.StatusMethodNotAllowed) http.Error(w, `{"error":"Method not allowed"}`, http.StatusMethodNotAllowed) return }
115-118: Duplicate HTTP status setting causes malformed responseSame pattern repeated again - calling both
WriteHeaderandhttp.Error.Apply this fix:
if r.Method != "GET" { - w.WriteHeader(http.StatusMethodNotAllowed) http.Error(w, `{"error":"Method not allowed"}`, http.StatusMethodNotAllowed) return }internal/mcp/server.go (3)
363-377: Global mutable state lacks thread safetyThe global map
serverMiddlewareis accessed without synchronization, creating potential race conditions in concurrent environments.Add proper synchronization:
+import "sync" + // Middleware storage for the MCP server -var serverMiddleware map[*mcpserver.MCPServer]func(mcpserver.ToolHandlerFunc) mcpserver.ToolHandlerFunc +var ( + serverMiddleware = make(map[*mcpserver.MCPServer]func(mcpserver.ToolHandlerFunc) mcpserver.ToolHandlerFunc) + serverMiddlewareMu sync.RWMutex +) -func init() { - serverMiddleware = make(map[*mcpserver.MCPServer]func(mcpserver.ToolHandlerFunc) mcpserver.ToolHandlerFunc) -} // setOAuthMiddleware stores the OAuth middleware for a server func setOAuthMiddleware(mcpServer *mcpserver.MCPServer, middleware func(mcpserver.ToolHandlerFunc) mcpserver.ToolHandlerFunc) { + serverMiddlewareMu.Lock() + defer serverMiddlewareMu.Unlock() serverMiddleware[mcpServer] = middleware } // GetOAuthMiddleware retrieves the OAuth middleware for a server func GetOAuthMiddleware(mcpServer *mcpserver.MCPServer) func(mcpserver.ToolHandlerFunc) mcpserver.ToolHandlerFunc { + serverMiddlewareMu.RLock() + defer serverMiddlewareMu.RUnlock() return serverMiddleware[mcpServer] }
253-256: Hardcoded scheme should use server configurationThe OAuth discovery response URLs hardcode the scheme instead of using the server's actual scheme configuration, which could cause issues in development environments.
Use the dynamic scheme:
"oauth": map[string]interface{}{ - "issuer": fmt.Sprintf("https://%s:%s", mcpHost, mcpPort), - "authorization_endpoint": fmt.Sprintf("https://%s:%s/oauth/authorize", mcpHost, mcpPort), - "token_endpoint": fmt.Sprintf("https://%s:%s/oauth/token", mcpHost, mcpPort), - "registration_endpoint": fmt.Sprintf("https://%s:%s/oauth/register", mcpHost, mcpPort), + "issuer": fmt.Sprintf("%s://%s:%s", s.getScheme(), mcpHost, mcpPort), + "authorization_endpoint": fmt.Sprintf("%s://%s:%s/oauth/authorize", s.getScheme(), mcpHost, mcpPort), + "token_endpoint": fmt.Sprintf("%s://%s:%s/oauth/token", s.getScheme(), mcpHost, mcpPort), + "registration_endpoint": fmt.Sprintf("%s://%s:%s/oauth/register", s.getScheme(), mcpHost, mcpPort),
238-240: Hardcoded HTTPS scheme in WWW-Authenticate headerThe WWW-Authenticate header always uses HTTPS scheme regardless of the actual server configuration.
Use the dynamic scheme:
w.Header().Set("WWW-Authenticate", fmt.Sprintf(`Bearer realm="%s", authorization_uri="%s/.well-known/oauth-authorization-server"`, mcpHost, - fmt.Sprintf("https://%s:%s", mcpHost, mcpPort))) + fmt.Sprintf("%s://%s:%s", s.getScheme(), mcpHost, mcpPort)))internal/oauth/handlers.go (2)
53-55: Hardcoded OAuth2 endpoints break provider compatibilityThe authorization and token URLs are hardcoded with
/oauth2/v1/paths which will not work for providers with different endpoint structures.Since you already use OIDC discovery in
internal/oauth/providers.go, extract the endpoints from the discovered provider:-Endpoint: oauth2.Endpoint{ - AuthURL: cfg.Issuer + "/oauth2/v1/authorize", - TokenURL: cfg.Issuer + "/oauth2/v1/token", -}, +// Use endpoints discovered via OIDC +Endpoint: provider.Endpoint(),
139-140: Security vulnerability in state parameter encodingUsing pipe character as delimiter is vulnerable if the state or redirect URI contains this character.
Use a more robust encoding scheme:
// If using fixed redirect URI, encode original client URI in state if h.config.RedirectURI != "" { - encodedState := fmt.Sprintf("%s|%s", state, clientRedirectURI) + // Encode state and redirect URI safely + stateData := map[string]string{"state": state, "redirect": clientRedirectURI} + jsonData, _ := json.Marshal(stateData) + encodedState := base64.URLEncoding.EncodeToString(jsonData) query.Set("state", encodedState)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/main.go(3 hunks)internal/config/config.go(4 hunks)internal/mcp/server.go(1 hunks)internal/oauth/config.go(1 hunks)internal/oauth/handlers.go(1 hunks)internal/oauth/metadata.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/oauth/config.go
- internal/config/config.go
- cmd/main.go
🧰 Additional context used
🧠 Learnings (4)
📓 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/oauth/metadata.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 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.735Z
Learning: Applies to examples/test_query.go : The file `examples/test_query.go` should 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-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.
internal/oauth/handlers.go (5)
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.
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.735Z
Learning: Applies to examples/test_query.go : The file `examples/test_query.go` should be used for HTTP API manual testing.
internal/mcp/server.go (7)
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 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 : 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`.
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/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.
🧬 Code Graph Analysis (2)
internal/oauth/metadata.go (2)
internal/oauth/handlers.go (1)
OAuth2Handler(21-24)cmd/main.go (1)
Version(16-16)
internal/oauth/handlers.go (2)
cmd/main.go (1)
Version(16-16)internal/config/config.go (1)
TrinoConfig(13-38)
🪛 GitHub Check: Code Quality
internal/oauth/handlers.go
[failure] 345-345:
Error return value of req.Body.Close is not checked (errcheck)
🪛 GitHub Actions: Build & Verify Pipeline
internal/oauth/handlers.go
[error] 345-345: golangci-lint: Error return value of req.Body.Close is not checked (errcheck)
🔇 Additional comments (5)
internal/oauth/metadata.go (1)
53-64: Good refactoring of duplicate codeThe code duplication issue for okta/google/azure providers has been properly addressed by consolidating them into a single case statement. This follows the DRY principle effectively.
internal/mcp/server.go (2)
29-46: Well-structured server initializationThe NewServer constructor properly initializes all components including conditional OAuth handler creation based on configuration. The separation of concerns is clear and the error handling is appropriate.
75-210: Robust HTTP server implementation with graceful shutdownThe ServeHTTP method implements comprehensive features including:
- TLS support with environment-based configuration
- Graceful shutdown with proper timeout handling
- Clear endpoint logging for debugging
- CORS support for cross-origin requests
internal/oauth/handlers.go (2)
91-150: Comprehensive OAuth2 authorization flow implementationThe HandleAuthorize method properly implements the OAuth2 authorization code flow with PKCE support, including parameter extraction, validation, and secure redirect handling.
201-306: Robust token exchange implementationThe HandleToken method correctly implements OAuth2 token exchange with PKCE support, proper validation, and comprehensive error handling. The custom transport for PKCE injection is a clever solution.
| response := map[string]interface{}{ | ||
| "client_id": h.config.ClientID, | ||
| "client_secret": "", // Public client, no secret | ||
| "client_id_issued_at": time.Now().Unix(), | ||
| "grant_types": []string{"authorization_code", "refresh_token"}, | ||
| "response_types": []string{"code"}, | ||
| "token_endpoint_auth_method": "none", | ||
| "application_type": "native", | ||
| "client_name": regRequest["client_name"], | ||
| } | ||
|
|
||
| // Use fixed redirect URI if configured, otherwise use client's redirect URIs | ||
| if h.config.RedirectURI != "" { | ||
| response["redirect_uris"] = []string{h.config.RedirectURI} | ||
| log.Printf("OAuth2: Registration response using fixed redirect URI: %s", h.config.RedirectURI) | ||
| } else { | ||
| response["redirect_uris"] = regRequest["redirect_uris"] | ||
| } | ||
|
|
||
| w.Header().Set("Content-Type", "application/json") | ||
| w.WriteHeader(http.StatusCreated) | ||
| if err := json.NewEncoder(w).Encode(response); err != nil { | ||
| log.Printf("OAuth2: Failed to encode registration response: %v", err) | ||
| http.Error(w, "Internal server error", http.StatusInternalServerError) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating client registration request
The registration handler accepts any client registration without validation. Consider adding basic validation for required fields like client_name and redirect_uris to ensure compliance with OAuth 2.0 Dynamic Client Registration.
// Parse the registration request
var regRequest map[string]interface{}
if err := json.NewDecoder(r.Body).Decode(®Request); err != nil {
log.Printf("OAuth2: Failed to parse registration request: %v", err)
http.Error(w, "Invalid request body", http.StatusBadRequest)
return
}
+// Validate required fields
+if _, ok := regRequest["client_name"]; !ok {
+ http.Error(w, "Missing required field: client_name", http.StatusBadRequest)
+ return
+}
+if _, ok := regRequest["redirect_uris"]; !ok {
+ http.Error(w, "Missing required field: redirect_uris", http.StatusBadRequest)
+ return
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/oauth/metadata.go around lines 162 to 187, the client registration
handler currently accepts all requests without validating required fields. Add
validation logic to check that essential fields like "client_name" and
"redirect_uris" are present and non-empty in the registration request. If
validation fails, respond with an appropriate HTTP error status and message to
enforce compliance with OAuth 2.0 Dynamic Client Registration requirements.
…e cache handling Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/oauth/handlers.go (1)
53-56: Hardcoded OAuth2 endpoints may break with different providersThe authorization and token URLs are hardcoded with
/oauth2/v1/paths, which may not be compatible with all OIDC providers. Many providers use different endpoint paths.Based on the past review comments, this issue was already identified. Consider using OIDC discovery to dynamically fetch the correct endpoints from the provider's well-known configuration instead of hardcoding these paths.
🧹 Nitpick comments (2)
internal/oauth/handlers.go (2)
68-76: HTTPS detection logic could be improvedThe scheme detection based on environment variables is fragile and doesn't account for scenarios where HTTPS might be terminated by a load balancer or proxy.
Consider adding more robust HTTPS detection:
// Determine scheme based on HTTPS configuration scheme := "http" -if getEnv("HTTPS_CERT_FILE", "") != "" && getEnv("HTTPS_KEY_FILE", "") != "" { +if getEnv("HTTPS_CERT_FILE", "") != "" && getEnv("HTTPS_KEY_FILE", "") != "" { scheme = "https" +} else if getEnv("FORCE_HTTPS", "") == "true" || getEnv("TLS_TERMINATION", "") == "proxy" { + scheme = "https" }
395-397: PKCE code verifier handling could be more defensiveThe code checks if
code_verifieris empty before adding it, but doesn't validate if an existing value should be overwritten or if there are conflicts.Consider adding more defensive handling:
// Add code_verifier if not already present -if values.Get("code_verifier") == "" && p.codeVerifier != "" { +if p.codeVerifier != "" { + if existing := values.Get("code_verifier"); existing != "" && existing != p.codeVerifier { + log.Printf("Warning: Overwriting existing code_verifier in token request") + } values.Set("code_verifier", p.codeVerifier) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/oauth/handlers.go(1 hunks)internal/oauth/metadata.go(1 hunks)internal/oauth/middleware.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/oauth/middleware.go
- internal/oauth/metadata.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/oauth/handlers.go (6)
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/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/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/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.735Z
Learning: Applies to examples/test_query.go : The file `examples/test_query.go` should be used for HTTP API manual testing.
🧬 Code Graph Analysis (1)
internal/oauth/handlers.go (2)
cmd/main.go (1)
Version(16-16)internal/config/config.go (1)
TrinoConfig(13-38)
🔇 Additional comments (4)
internal/oauth/handlers.go (4)
140-154: Improved state parameter encoding implementationThe state parameter encoding has been improved to use JSON + base64 encoding instead of the previously vulnerable pipe-delimited format. This addresses the security concern raised in past reviews.
The implementation properly handles encoding errors and provides appropriate logging for debugging.
195-211: Secure state parameter decoding with proper error handlingThe state parameter decoding correctly uses base64 + JSON decoding that matches the encoding implementation. The code properly handles decoding errors and validates the presence of required fields.
This implementation addresses the injection vulnerability that was identified in past reviews for the pipe-delimited format.
378-382: Proper resource cleanup with error handlingThe request body closing has been properly implemented with defer and error checking, addressing the resource leak issue identified in past reviews.
The implementation correctly uses defer with a closure to handle the close error and provides appropriate logging.
109-110: Authorization code logging poses security riskSimilar to the token endpoint, logging the authorization code (even truncated) in the authorize endpoint could expose sensitive information in logs.
Apply the same logging security fix:
-log.Printf("OAuth2: Authorization request - client_id: %s, redirect_uri: %s, code_challenge: %s", - clientID, clientRedirectURI, truncateString(codeChallenge, 10)) +log.Printf("OAuth2: Authorization request - client_id: %s, redirect_uri: %s, code_challenge: [REDACTED]", + clientID, clientRedirectURI)Likely an incorrect or invalid review comment.
| log.Printf("OAuth2: Token request - grant_type: %s, client_id: %s, redirect_uri: %s, code: %s", | ||
| grantType, clientID, clientRedirectURI, truncateString(code, 10)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential information leakage in logging
The authorization code is being logged with truncation, but this could still pose a security risk in production environments where logs might be accessible to unauthorized parties.
Consider removing or further masking the authorization code in logs:
-log.Printf("OAuth2: Token request - grant_type: %s, client_id: %s, redirect_uri: %s, code: %s",
- grantType, clientID, clientRedirectURI, truncateString(code, 10))
+log.Printf("OAuth2: Token request - grant_type: %s, client_id: %s, redirect_uri: %s, code: [REDACTED]",
+ grantType, clientID, clientRedirectURI)📝 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.
| log.Printf("OAuth2: Token request - grant_type: %s, client_id: %s, redirect_uri: %s, code: %s", | |
| grantType, clientID, clientRedirectURI, truncateString(code, 10)) | |
| log.Printf("OAuth2: Token request - grant_type: %s, client_id: %s, redirect_uri: %s, code: [REDACTED]", | |
| grantType, clientID, clientRedirectURI) |
🤖 Prompt for AI Agents
In internal/oauth/handlers.go around lines 257 to 258, the authorization code is
logged with partial truncation, which can still risk sensitive information
exposure. To fix this, remove the authorization code from the log statement
entirely or replace it with a fixed placeholder like "[REDACTED]" to prevent any
sensitive data from being written to logs.
| log.Printf("OAuth2: Callback received - code: %s, state: %s, error: %s", | ||
| truncateString(code, 10), state, errorParam) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Authorization code logging in callback handler
The callback handler also logs the authorization code, which should be redacted for security.
-log.Printf("OAuth2: Callback received - code: %s, state: %s, error: %s",
- truncateString(code, 10), state, errorParam)
+log.Printf("OAuth2: Callback received - code: [REDACTED], state: %s, error: %s",
+ state, errorParam)📝 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.
| log.Printf("OAuth2: Callback received - code: %s, state: %s, error: %s", | |
| truncateString(code, 10), state, errorParam) | |
| log.Printf("OAuth2: Callback received - code: [REDACTED], state: %s, error: %s", | |
| state, errorParam) |
🤖 Prompt for AI Agents
In internal/oauth/handlers.go around lines 176 to 177, the authorization code is
logged in the callback handler, which poses a security risk. Modify the log
statement to redact or omit the authorization code instead of printing it
directly. You can replace the code with a fixed placeholder like "[REDACTED]" or
remove it entirely from the log message to prevent sensitive information
exposure.
| "expires_in": int(time.Until(token.Expiry).Seconds()), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Token expiration calculation may return negative values
The expires_in calculation using time.Until(token.Expiry).Seconds() could return negative values if the token is already expired, which might confuse OAuth2 clients.
Add a check to ensure non-negative expiration times:
-"expires_in": int(time.Until(token.Expiry).Seconds()),
+"expires_in": int(max(0, time.Until(token.Expiry).Seconds())),Or use a helper function:
+func max(a, b float64) float64 {
+ if a > b {
+ return a
+ }
+ return b
+}📝 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.
| "expires_in": int(time.Until(token.Expiry).Seconds()), | |
| } | |
| "expires_in": int(max(0, time.Until(token.Expiry).Seconds())), | |
| } | |
| // max returns the larger of a and b. | |
| func max(a, b float64) float64 { | |
| if a > b { | |
| return a | |
| } | |
| return b | |
| } |
🤖 Prompt for AI Agents
In internal/oauth/handlers.go around lines 312 to 313, the calculation for
"expires_in" using time.Until(token.Expiry).Seconds() can yield negative values
if the token is already expired. To fix this, add a check to ensure the
expiration time is never negative by using a conditional or helper function that
returns zero if the calculated duration is negative, otherwise returns the
actual seconds until expiry.
…ling Signed-off-by: Tommy Nguyen <tuannvm@hotmail.com>
Summary
This PR introduces a robust OAuth token validation system supporting both HMAC and OIDC JWT tokens, along with a revamped HTTP server that handles MCP requests with proper CORS headers, graceful shutdown, and optional HTTPS support. Additionally, server initialization now includes OAuth hooks for request authentication, enhancing security and compliance.
Affected Modules
internal/oauth/providers.go: ImplementsHMACValidatorandOIDCValidatorfor token validation.internal/server/http.go: New file defining HTTP server with request handling, CORS, and signal handling.Key Details
HMACValidator).OIDCValidator)./mcpand/sseendpoints with shared handlers.Potential Impacts
/sseendpoint.Testing Steps
/mcpand/sseendpoints with correct and invalid tokens.This PR enhances the MCP server by integrating flexible OAuth support and robust HTTP handling, aligning with secure, scalable deployment practices.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores