[IMPLEMENTING] Auth for portalJS key #3
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
portaljs-mcp-server | 37dc878 | Oct 08 2025, 06:41 PM |
WalkthroughAdds a per-session State (apiKey, apiUrl), updates MyMCP to extend McpAgent<Env, State>, supplies initialState and getApiUrl() for secure URL resolution, introduces a runtime Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Agent as MyMCP
participant Store as State/Env
Note over Agent: Initialization — resolve apiUrl
User->>Agent: start/init
Agent->>Store: read state.apiUrl
alt apiUrl in state
Agent-->>User: init (using state.apiUrl)
else
Agent->>Store: read env.API_URL
alt apiUrl in env
Agent-->>User: init (using env.API_URL)
else
Agent-->>User: init (using default API URL)
end
end
sequenceDiagram
autonumber
actor User
participant Agent as MyMCP
participant State as State
Note over Agent: Runtime auth configuration
User->>Agent: tool: set_api_key(apiKey, apiUrl?)
Agent->>State: persist apiKey, apiUrl
Agent-->>User: "API key and URL stored"
sequenceDiagram
autonumber
actor User
participant Agent as MyMCP
participant API as CKAN-like API
Note over Agent: Authenticated dataset creation
User->>Agent: tool: create_dataset(params)
alt apiKey present
Agent->>API: POST getApiUrl()/api/3/action/package_create
Note right of API: Headers: Authorization: apiKey
alt success
API-->>Agent: 200 dataset payload
Agent-->>User: success (id/name/url)
else error
API-->>Agent: error payload
Agent-->>User: error + actionable tips
end
else no apiKey
Agent-->>User: error: authentication required — call set_api_key
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/index.ts (2)
38-50: Consider extracting header construction to reduce duplication.The header-building logic is duplicated between
searchandfetchtools (lines 38-45 and 107-114). Extract to a helper method for maintainability.Apply this refactor to reduce duplication:
+ private buildHeaders(): Record<string, string> { + const headers: Record<string, string> = { + "Content-Type": "application/json", + "User-Agent": "MCP-PortalJS-Server/1.0" + }; + if (this.apiKey) { + headers["Authorization"] = this.apiKey; + } + return headers; + } + async init() {Then use it in both tools:
- const headers: Record<string, string> = { - "Content-Type": "application/json", - "User-Agent": "MCP-PortalJS-Server/1.0" - }; - - if (this.apiKey) { - headers["Authorization"] = this.apiKey; - } - const response = await fetch(endpoint, { method: "GET", - headers + headers: this.buildHeaders() });
194-211: Implementation is correct. Consider security implications of URL-based API keys.The request cloning and header injection work correctly. However, passing API keys as URL query parameters has security implications:
- Logged in browser history, server logs, and proxy logs
- May appear in Referrer headers
- More vulnerable to MITM attacks on non-HTTPS connections
For production deployments, consider:
- Documenting that users should use HTTPS
- Warning users about the security trade-offs
- Exploring alternative auth methods if MCP clients support header-based authentication
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/index.ts (1)
worker-configuration.d.ts (1)
env(7787-7787)
⏰ 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: Workers Builds: portaljs-mcp-server
🔇 Additional comments (1)
src/index.ts (1)
16-25: LGTM! Clean API key extraction.The optional field and defensive header extraction with optional chaining handle missing API keys gracefully.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(2 hunks)
⏰ 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: Workers Builds: portaljs-mcp-server
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
573-586: apiKey/apiUrl from query are never propagated to the agent (breaks advertised URL auth flow).Extract
apiKey/apiUrlfrom the URL and inject asX-PortalJS-API-Key/X-PortalJS-API-Urlheaders before forwarding toMyMCP. Otherwise, runtime auth via?apiKey=...won’t work.Apply this diff:
export default { fetch(request: Request, env: Env, ctx: ExecutionContext) { const url = new URL(request.url); + // Inject apiKey/apiUrl from query into headers so the agent can read them + const apiKey = url.searchParams.get("apiKey") || request.headers.get("X-PortalJS-API-Key") || undefined; + const apiUrlParam = url.searchParams.get("apiUrl") || request.headers.get("X-PortalJS-API-Url") || undefined; + const headers = new Headers(request.headers); + if (apiKey) headers.set("X-PortalJS-API-Key", apiKey); + if (apiUrlParam) headers.set("X-PortalJS-API-Url", apiUrlParam); + const reqWithAuth = new Request(request, { headers }); if (url.pathname === "/sse" || url.pathname === "/sse/message") { - return MyMCP.serveSSE("/sse").fetch(request, env, ctx); + return MyMCP.serveSSE("/sse").fetch(reqWithAuth, env, ctx); } if (url.pathname === "/mcp") { - return MyMCP.serve("/mcp").fetch(request, env, ctx); + return MyMCP.serve("/mcp").fetch(reqWithAuth, env, ctx); } return new Response("Not found", { status: 404 }); }, };Optional: strip
apiKeyfrom the forwarded URL to avoid accidental logging. I can add that if desired.
🧹 Nitpick comments (4)
src/index.ts (4)
203-289: Auth header and error UX are solid; add CKAN compat header and clearer 401 messaging.
- Add
X-CKAN-API-Keyfor broader CKAN compatibility.- Handle 401/403 explicitly to guide users (invalid/expired key).
const response = await fetch(endpoint, { method: "POST", headers: { "Content-Type": "application/json", - "Authorization": this.state.apiKey, + "Authorization": this.state.apiKey, + "X-CKAN-API-Key": this.state.apiKey, "User-Agent": "MCP-PortalJS-Server/1.0" }, body: JSON.stringify(requestBody) }); - if (!response.ok) { + if (!response.ok) { + if (response.status === 401 || response.status === 403) { + return { + content: [{ type: "text", text: "Authentication failed. Check your API key or permissions." }] + }; + } return { content: [{ type: "text", text: `Error: API returned ${response.status} ${response.statusText}` }] }; }
503-569: Org update tool is fine; missing tools noted in PR summary.PR summary mentions create organization, add/remove user; they aren’t present here. Confirm scope or add follow-ups.
I can add
organization_create,member_create/member_deletetools consistent with this pattern if desired.
104-113: Return actual total count from search.Use CKAN’s
result.countinstead ofresults.length(which is bounded byrows).- text: JSON.stringify({ - query, - total_results: results.length, - results - }, null, 2) + text: JSON.stringify({ + query, + total_results: data.result?.count ?? results.length, + results + }, null, 2)
64-70: Minor: DropContent-Typeon GET requests.Not harmful, but unnecessary and occasionally confusing for proxies.
- headers: { - "Content-Type": "application/json", - "User-Agent": "MCP-PortalJS-Server/1.0" - } + headers: { + "User-Agent": "MCP-PortalJS-Server/1.0" + }Also applies to: 128-134, 311-317
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(4 hunks)
⏰ 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: Workers Builds: portaljs-mcp-server
🔇 Additional comments (8)
src/index.ts (8)
9-12: State shape looks good.
15-22: Generic agent with initialState is correct.
61-61: Good: tools now recompute API URL at runtime.
125-126: Good: runtime API URL used for fetch as well.
291-353: Verify organization endpoint fields; consider requesting full fields.CKAN often requires
all_fields=truefor full org objects. If you see only names returned, add the flag.- const endpoint = `${apiUrl}/api/3/action/organization_list_for_user`; + const endpoint = `${apiUrl}/api/3/action/organization_list_for_user?all_fields=true`;If your instance already returns full objects, ignore this.
355-428: Resource creation flow looks fine.
430-501: Update dataset flow looks fine.
27-31: Verify access to incoming headers on McpAgent
I couldn’t findthis.props.requestinMyMCP– please confirmMcpAgentexposesprops.request.headersor adjust to your agent’s API for seeding API key/url from request headers.
…s, get_organization_details, compare_datasets;
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/index.ts (2)
23-25: URL normalization still missing.This issue was previously flagged and remains unaddressed. Without normalization, user-provided URLs can produce malformed endpoints (
//api/...) and introduce SSRF risks.
38-41: api_url validation before persisting still missing.This was previously flagged. The incoming
api_urlis stored without validation, normalization, or HTTPS enforcement, allowing malformed or insecure URLs to be persisted.
🧹 Nitpick comments (1)
src/index.ts (1)
218-225: Extract repeated auth check into a helper.The auth validation pattern
if (!this.state.apiKey) return { content: [{ type: "text", text: "..." }] }is repeated in 8 tools. Consider extracting this into a reusable helper to reduce duplication and ensure consistent messaging.Example helper:
private requireAuth(): { content: Array<{ type: "text"; text: string }> } | null { if (!this.state.apiKey) { return { content: [{ type: "text", text: "Authentication required.\n\nPlease set your API key first." }] }; } return null; }Then in each tool:
const authError = this.requireAuth(); if (authError) return authError;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(4 hunks)
⏰ 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: Workers Builds: portaljs-mcp-server
🔇 Additional comments (6)
src/index.ts (6)
9-12: LGTM!The
Stateinterface correctly defines optional session credentials.
203-289: Excellent error handling with user guidance.The contextual error tips (lines 266-270) help users diagnose common issues like missing organization or duplicate names. The conditional request body construction is clean and correct.
622-662: LGTM!The limit capping (line 632) prevents excessive data fetching, and the error message (lines 640-643) clearly guides users when previews aren't available.
718-720: LGTM!The deduplication using
Mapby ID (line 718) and limiting to 10 results prevents duplicate/excessive data in responses.
788-833: LGTM!The parallel fetching with
Promise.all(line 799) and graceful per-dataset error handling (lines 805-807) make this efficient and resilient.
459-459: Clarify empty-tags behaviorAssigning
tags: []inpackage_patchreplaces and clears all existing tags. Confirm this is intentional; otherwise, guard for non-empty arrays:if (tags?.length) { requestBody.tags = tags.map(tag => ({ name: tag })); }
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/index.ts (1)
114-114: Consider extracting User-Agent to a constant.The User-Agent string
"MCP-PortalJS-Server/1.0"is repeated 12+ times. Extract it to a class constant for easier version updates.export class MyMCP extends McpAgent<Env, State> { server = new McpServer({ name: "PortalJS MCP Server", version: "1.0.0", }); + private readonly USER_AGENT = "MCP-PortalJS-Server/1.0"; + initialState: State = {};Then replace all occurrences:
- "User-Agent": "MCP-PortalJS-Server/1.0" + "User-Agent": this.USER_AGENTAlso applies to: 178-178, 292-292, 361-361, 440-440, 513-513, 581-581
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(4 hunks)
⏰ 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: Workers Builds: portaljs-mcp-server
🔇 Additional comments (2)
src/index.ts (2)
23-49: LGTM! URL normalization is thorough.The implementation correctly addresses URL normalization concerns: converts HTTP to HTTPS, validates protocol, strips trailing slashes, collapses duplicate slashes, and safely falls back to the default on parse errors.
250-879: Excellent tool coverage and UX guidance!The new authenticated tools provide comprehensive dataset/resource/organization operations with thoughtful user experience:
- Contextual error messages with actionable tips (e.g., lines 312-316 for org-related errors)
- Success messages guide next steps (e.g., line 331 suggests adding resources)
- Structured JSON responses for easy parsing
- Consistent patterns across tools
…andling in read-only tools
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/index.ts (2)
73-94: Add validation for API key and SSRF protection.Two security concerns remain unaddressed:
- Empty API key: No validation ensures
api_keyis non-empty. A whitespace-only key would be accepted.- SSRF risk: User-provided
api_urlcan target internal endpoints (e.g.,https://localhost,https://169.254.169.254), enabling Server-Side Request Forgery attacks against internal services or cloud metadata endpoints.Apply this diff to add validation:
async ({ api_key, api_url }) => { + if (!api_key || !api_key.trim()) { + return { + content: [{ + type: "text", + text: `❌ API key cannot be empty.` + }] + }; + } + let normalizedUrl: string; if (!api_url) { normalizedUrl = this.getApiUrl(); } else { try { const url = new URL(api_url); + + // Block localhost and internal IPs + const hostname = url.hostname.toLowerCase(); + if ( + hostname === "localhost" || + hostname.startsWith("127.") || + hostname.startsWith("169.254.") || + hostname.startsWith("10.") || + hostname.match(/^192\.168\./) || + hostname.match(/^172\.(1[6-9]|2[0-9]|3[0-1])\./) + ) { + return { + content: [{ + type: "text", + text: `❌ Internal/private URLs are not allowed for security reasons.` + }] + }; + } if (url.protocol === "http:") { url.protocol = "https:"; }
89-89: Preserve pathname to match getApiUrl() behavior.Line 89 extracts only
url.origin, discarding any pathname, butgetApiUrl()(line 45) returnsurl.origin + pathname. If a user provideshttps://example.com/api/v1, onlyhttps://example.comis stored, yet other sources (env or default) might include paths. This inconsistency breaks custom deployments with path-based routing.Apply this diff to preserve the pathname:
if (url.protocol !== "https:") { normalizedUrl = this.getApiUrl(); } else { - normalizedUrl = url.origin; + let pathname = url.pathname; + if (pathname.endsWith("/")) { + pathname = pathname.slice(0, -1); + } + pathname = pathname.replace(/\/+/g, "/"); + normalizedUrl = url.origin + pathname; }
🧹 Nitpick comments (1)
src/index.ts (1)
262-592: Consider adding try-catch for network errors in authenticated tools.The five authenticated write tools (create_dataset, list_organizations, create_resource, update_dataset, update_organization) make
fetchcalls without try-catch blocks. Network failures (timeouts, DNS errors, connection refused) will throw unhandled exceptions, crashing the tool instead of returning a user-friendly error.Note: The read-only tools (lines 595-901) correctly wrap their fetch calls in try-catch blocks - applying the same pattern here would improve consistency and user experience.
Example pattern to apply:
async ({ /* params */ }) => { const authError = this.requireAuth(); if (authError) return authError; try { const apiUrl = this.getApiUrl(); const endpoint = `${apiUrl}/api/3/action/...`; const response = await fetch(endpoint, { /* ... */ }); // ... rest of logic } catch (error) { return { content: [{ type: "text", text: `Error: Failed to complete operation. ${error instanceof Error ? error.message : 'Unknown error'}` }] }; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/index.ts(4 hunks)
⏰ 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: Workers Builds: portaljs-mcp-server
🔇 Additional comments (6)
src/index.ts (6)
5-21: LGTM! Clean interface definitions and state initialization.The State interface properly defines the session-level apiKey and apiUrl fields, and the class correctly extends McpAgent with both Env and State type parameters. The empty initialState is appropriate for session-based authentication.
23-49: LGTM! Robust URL normalization with security enforcement.The getApiUrl() method properly:
- Enforces HTTPS protocol (converting http or rejecting other protocols)
- Normalizes the pathname by removing trailing slashes and collapsing duplicates
- Returns a consistent origin+pathname format
- Falls back safely to the default URL on any parsing errors
51-61: LGTM! Excellent refactoring of authentication checks.The requireAuth() helper eliminates code duplication across multiple tools and provides a consistent authentication error message. Good use of a private method for this cross-cutting concern.
111-259: LGTM! Well-implemented public read operations.Both the search and fetch tools correctly:
- Use getApiUrl() for dynamic URL resolution
- Handle HTTP errors and API errors appropriately
- Return well-structured JSON responses
- Operate without authentication (appropriate for public data access)
595-901: LGTM! Excellent implementation of read-only analysis tools.All five analysis tools (get_dataset_stats, preview_resource, get_related_datasets, get_organization_details, compare_datasets) demonstrate best practices:
- Comprehensive try-catch blocks for network error handling
- Helpful, user-friendly error messages
- Well-structured JSON responses with relevant metadata
- Appropriate use of getApiUrl() for URL resolution
These serve as a good reference implementation for the authenticated tools.
905-919: LGTM! Clean request routing.The request handler correctly routes to SSE and MCP endpoints with appropriate 404 fallback for unknown paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
README.md (5)
18-18: Clarify server scaffolding and state/auth patterns.If code now uses McpAgent with per-session state (apiKey, apiUrl), requireAuth(), and getApiUrl(), mention these here (where to add tools, how URLs are built, and how auth is enforced).
30-31: Connection example: use HTTPS and portal-specific naming.In the Claude Desktop example, prefer:
- Server name like "portaljs" (not "calculator").
- HTTPS for remote Workers URL (Cloudflare defaults to HTTPS).
- Note that write ops need the set_api_key tool after connecting.
52-120: Fix markdown subheadings (MD036) and complete/auth-align the tool list.
- Replace bolded pseudo-headings (e.g., “Search for datasets”) with proper
####headings to satisfy markdownlint.- Add missing write tools per PR summary: create_organization, add_user_to_organization, remove_user_from_organization.
- Ensure all documented tool names match actual registrations; keep only supported tools.
Based on markdownlint hints
Proposed heading fixes within this section:
-**Set your API key** (required for write operations) +#### Set your API key (required for write operations) -**Search for datasets** +#### Search for datasets -**Get detailed dataset information** +#### Get detailed dataset information -**Get quick dataset statistics** +#### Get quick dataset statistics -**Preview data structure** +#### Preview data structure -**Find related datasets** +#### Find related datasets -**Compare multiple datasets** +#### Compare multiple datasets -**Get organization information** +#### Get organization information -**List your organizations** +#### List your organizations -**Create a new dataset** +#### Create a new dataset -**Add a resource to a dataset** +#### Add a resource to a dataset -**Update dataset metadata** +#### Update dataset metadata -**Update organization details** +#### Update organization detailsAlso consider adding:
Create an organization
Add user to organization
Remove user from organization
56-61: Authentication UX: note apiUrl handling and key privacy.Briefly state that set_api_key stores apiKey (and optional apiUrl) in per-session state, not persisted; advise not to share keys in logs/screenshots.
114-120: Tips: add HTTPS + org-ID guidance.
- Prefer HTTPS for remote URLs.
- Mention using list_organizations to get org IDs and that dataset names must be lowercase kebab-case (already noted).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(4 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
64-64: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
68-68: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
72-72: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
76-76: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
80-80: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
84-84: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
88-88: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
94-94: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
102-102: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
106-106: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
110-110: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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: Workers Builds: portaljs-mcp-server
🔇 Additional comments (1)
README.md (1)
52-120: Validate README tool names against code registrations. I couldn’t locate anyserver.tool()(or similar) definitions matching the listed tools; please ensure each tool in the README corresponds exactly to a registered tool in the code.
| # Building a Remote MCP Server on Cloudflare (Without Auth) | ||
|
|
||
| This example allows you to deploy a remote MCP server that doesn't require authentication on Cloudflare Workers. | ||
| This example allows you to deploy a remote MCP server that doesn't require authentication on Cloudflare Workers. |
There was a problem hiding this comment.
Docs contradict auth-enabled implementation; update intro and deployment references.
Text says "doesn't require authentication" and links/templates point to "remote-mcp-authless". Align with API-key auth, update title/intro, and replace authless template/URLs with this repo’s deployment path. Prefer HTTPS in examples.
🤖 Prompt for AI Agents
In README.md around line 3, the intro claims the example "doesn't require
authentication" and points to authless templates/URLs; update the title and
first paragraph to state this deployment uses API-key (auth-enabled) instead of
authless, replace any links/templates that reference "remote-mcp-authless" with
this repository's deployment path (use the canonical repo/deploy URLs), and
ensure all example URLs use HTTPS; keep wording concise and update any related
deployment reference text to mention API-key configuration steps instead of "no
auth".
1 participant
[IMPLEMENTED] New Tools & Auth API Key PortalJS
Now you can go into Claude/GPT and have the API Key connect and interact with your datasets
Note
Regarding the following Job Stories:
New Features bellow:
Summary by CodeRabbit
New Features
Improvements
Documentation