Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds alias-based tenant routing and per-alias partner metadata: new AliasModule, AliasService, PartnerMetaService, Redis config/env, and wiring; MCP and auth routes now accept Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MCP as MCP_Controller
participant AliasSvc as AliasService
participant Redis as Alias_Redis
participant MetaSvc as PartnerMetaService
participant ServerFactory as MCP_ServerFactory
participant WidgetSvc as WidgetService
Client->>MCP: POST /mcp/:alias (initialize/request)
MCP->>AliasSvc: resolveAlias(alias)
AliasSvc->>Redis: GET {prefix}:{alias}
Redis-->>AliasSvc: projectApiKey | null
alt alias found
AliasSvc-->>MCP: projectApiKey
MCP->>MetaSvc: getAliasMeta(alias)
MetaSvc->>Redis: GET {prefix}-meta:{alias}
Redis-->>MetaSvc: metaJSON | null
MetaSvc-->>MCP: AliasMeta | null
MCP->>ServerFactory: getOrCreateServer(projectApiKey, meta)
ServerFactory-->>MCP: McpServer (name/instructions from meta)
MCP->>WidgetSvc: register/read widgets (widgetName, meta)
WidgetSvc-->>MCP: widget HTML with __PARTNER__
MCP-->>Client: 200 / session established
else alias missing
AliasSvc-->>MCP: null
MCP-->>Client: 404 JSON-RPC (unknown alias)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/mcp.controller.ts (1)
164-185:⚠️ Potential issue | 🔴 CriticalBind follow-up requests to the project captured at session initialization.
Once
mcpSessionIdis present, these paths only check whether the transport exists; they never verify that the current alias resolution matchessessionProjectMap. That allows a live session to be replayed through a different alias/project path, andhandleMcpDelete()can even delete the wrong project-scoped Redis record after an alias remap.As per coding guidelines, "Multi-tenant architecture must isolate each project's session and API key scope — never allow cross-project data access."
Also applies to: 266-293, 317-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp.controller.ts` around lines 164 - 185, The current reuse of an existing transport (when mcpSessionId is present) doesn't verify the transport's original project binding, allowing cross-project replay; update the reuse logic in the blocks around mcpSessionId handling (the code using this.transports.get(mcpSessionId) and transport.handleRequest) to compare the resolved project API key/alias (the value returned by resolveAlias or projectApiKey) against the stored session-to-project mapping in sessionProjectMap for that mcpSessionId, and if they differ reject the request (return an error/403 and log) instead of reusing the transport; apply the same check before any session-scoped operations such as handleMcpDelete to ensure the sessionProjectMap entry matches the current project context so sessions cannot be replayed across projects.
🧹 Nitpick comments (3)
src/alias/alias.service.ts (1)
4-4: Switch this internal import to the@/alias.This new file starts with a relative internal import instead of the repo-standard
@/*path.As per coding guidelines, "Use path alias
@/*→src/*for all internal imports."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/alias/alias.service.ts` at line 4, The import of ALIAS_REDIS_CLIENT in alias.service.ts uses a relative path; change it to the repo-standard path alias by importing ALIAS_REDIS_CLIENT from the "@/alias/alias.constants" module instead of './alias.constants' so all internal imports use the `@/`* → src/* alias (update the import statement referencing ALIAS_REDIS_CLIENT accordingly).src/alias/alias.module.ts (1)
4-5: Convert these internal imports to@/paths.Both new local imports here bypass the repo's TypeScript import convention.
As per coding guidelines, "Use path alias
@/*→src/*for all internal imports."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/alias/alias.module.ts` around lines 4 - 5, The two internal imports in alias.module.ts use relative paths; change them to the repo path-alias form: replace the './alias.constants' import of ALIAS_REDIS_CLIENT with '@/alias/alias.constants' and replace the './alias.service' import of AliasService with '@/alias/alias.service' so both imports follow the "@/..." → "src/..." convention used across the codebase.src/app.module.ts (1)
15-15: Use the repo alias for this new internal import.
./alias/alias.moduleadds another relativesrcimport where the repo expects@/*paths for internal TypeScript modules.As per coding guidelines, "Use path alias
@/*→src/*for all internal imports."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app.module.ts` at line 15, Replace the relative import of AliasModule (import { AliasModule } from './alias/alias.module') with the repo path-alias import using @ (import { AliasModule } from '@/alias/alias.module'), ensuring the AliasModule import uses the `@/`* → src/* alias; verify tsconfig/paths already contains the alias so the new import resolves correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 41-46: The compose file lacks readiness checks for the app and
Redis even though new ALIAS_REDIS_* env vars introduce a runtime dependency; add
healthcheck blocks for the redis service (use redis-cli PING or equivalent, with
sensible interval/timeout/retries/start_period) and for the app service (HTTP
GET /health or startup probe) and set restart: unless-stopped (or always on
prod) for both; update the app's depends_on to wait for redis to be healthy (use
depends_on with condition: service_healthy if your Compose version supports it,
otherwise implement an entrypoint wait-for-redis that polls
ALIAS_REDIS_HOST/ALIAS_REDIS_PORT before starting) so the app only starts when
Redis is ready.
In `@src/mcp/mcp.controller.ts`:
- Around line 164-169: The code currently calls resolveAlias(...) before doing
the cheap JWT Bearer check, allowing unauthenticated callers to hit external
Redis; move the generic Bearer validation (extract and decode the token with
jwt.decode() and ensure a valid userId) into McpController before any call to
resolveAlias, splitting validateAuth into two steps: a token-only check that
returns 401 on failure and a project-scoped authorization that runs after
resolveAlias; update the call sites that call resolveAlias then validateAuth
(including the other two similar places) to perform jwt.decode()/token
validation first and only then call resolveAlias(...) and the project-specific
validateAuth(...) so unauthenticated requests are rejected immediately.
- Around line 58-75: Wrap the call to aliasService.resolveAlias inside
resolveAlias in a try/catch so any Redis or other exceptions are caught and
returned as a JSON-RPC error (code -32603) instead of bubbling as an HTTP 500;
if resolveAlias returns no apiKey, respond with a JSON-RPC error object (use
code -32603 and a clear "Not Found: Unknown alias '<alias>'" message) and ensure
the id uses (body as Record<string, unknown>)?.id ?? null so values like 0 are
preserved. Reference: resolveAlias and aliasService.resolveAlias; convert all
alias lookup failures and thrown errors to the MCP JSON-RPC error contract with
code -32603 and do not rethrow.
---
Outside diff comments:
In `@src/mcp/mcp.controller.ts`:
- Around line 164-185: The current reuse of an existing transport (when
mcpSessionId is present) doesn't verify the transport's original project
binding, allowing cross-project replay; update the reuse logic in the blocks
around mcpSessionId handling (the code using this.transports.get(mcpSessionId)
and transport.handleRequest) to compare the resolved project API key/alias (the
value returned by resolveAlias or projectApiKey) against the stored
session-to-project mapping in sessionProjectMap for that mcpSessionId, and if
they differ reject the request (return an error/403 and log) instead of reusing
the transport; apply the same check before any session-scoped operations such as
handleMcpDelete to ensure the sessionProjectMap entry matches the current
project context so sessions cannot be replayed across projects.
---
Nitpick comments:
In `@src/alias/alias.module.ts`:
- Around line 4-5: The two internal imports in alias.module.ts use relative
paths; change them to the repo path-alias form: replace the './alias.constants'
import of ALIAS_REDIS_CLIENT with '@/alias/alias.constants' and replace the
'./alias.service' import of AliasService with '@/alias/alias.service' so both
imports follow the "@/..." → "src/..." convention used across the codebase.
In `@src/alias/alias.service.ts`:
- Line 4: The import of ALIAS_REDIS_CLIENT in alias.service.ts uses a relative
path; change it to the repo-standard path alias by importing ALIAS_REDIS_CLIENT
from the "@/alias/alias.constants" module instead of './alias.constants' so all
internal imports use the `@/`* → src/* alias (update the import statement
referencing ALIAS_REDIS_CLIENT accordingly).
In `@src/app.module.ts`:
- Line 15: Replace the relative import of AliasModule (import { AliasModule }
from './alias/alias.module') with the repo path-alias import using @ (import {
AliasModule } from '@/alias/alias.module'), ensuring the AliasModule import uses
the `@/`* → src/* alias; verify tsconfig/paths already contains the alias so the
new import resolves correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2d29c5b-1772-4662-9904-efd84f6fe20c
📒 Files selected for processing (8)
.env.exampledocker-compose.staging.ymldocker-compose.ymlsrc/alias/alias.constants.tssrc/alias/alias.module.tssrc/alias/alias.service.tssrc/app.module.tssrc/mcp/mcp.controller.ts
Comment on lines
+41
to
+46
| # Alias Redis (External - partner alias → API key lookup) | ||
| ALIAS_REDIS_HOST: ${ALIAS_REDIS_HOST} | ||
| ALIAS_REDIS_PORT: ${ALIAS_REDIS_PORT:-6379} | ||
| ALIAS_REDIS_PASSWORD: ${ALIAS_REDIS_PASSWORD:-} | ||
| ALIAS_REDIS_DB: ${ALIAS_REDIS_DB:-0} | ||
| ALIAS_REDIS_KEY_PREFIX: ${ALIAS_REDIS_KEY_PREFIX:-alias} |
There was a problem hiding this comment.
Add production health checks before wiring more runtime dependencies.
This file still has no health checks for either the app or Redis service, so Compose only guarantees startup order, not readiness. That gets riskier now that MCP requests also depend on the new alias Redis path.
As per coding guidelines, "docker-compose.yml: Docker Compose for production must include both app and Redis services with proper health checks and restart policies."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` around lines 41 - 46, The compose file lacks readiness
checks for the app and Redis even though new ALIAS_REDIS_* env vars introduce a
runtime dependency; add healthcheck blocks for the redis service (use redis-cli
PING or equivalent, with sensible interval/timeout/retries/start_period) and for
the app service (HTTP GET /health or startup probe) and set restart:
unless-stopped (or always on prod) for both; update the app's depends_on to wait
for redis to be healthy (use depends_on with condition: service_healthy if your
Compose version supports it, otherwise implement an entrypoint wait-for-redis
that polls ALIAS_REDIS_HOST/ALIAS_REDIS_PORT before starting) so the app only
starts when Redis is ready.
src/mcp/mcp.controller.ts
Outdated
Comment on lines
+58
to
+75
| private async resolveAlias( | ||
| alias: string, | ||
| body: unknown, | ||
| res: Response, | ||
| ): Promise<string | null> { | ||
| const apiKey = await this.aliasService.resolveAlias(alias); | ||
| if (!apiKey) { | ||
| res.status(HttpStatus.NOT_FOUND).json({ | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: -32004, | ||
| message: `Not Found: Unknown alias '${alias}'`, | ||
| }, | ||
| id: (body as Record<string, unknown>)?.id || null, | ||
| }); | ||
| return null; | ||
| } | ||
| return apiKey; |
There was a problem hiding this comment.
Keep alias lookup failures inside the repo's JSON-RPC error contract.
resolveAlias() introduces -32004, and a Redis exception from aliasService.resolveAlias() would currently bubble out as a framework 500 instead of a JSON-RPC error. Also use ?? null for id here so valid JSON-RPC ids like 0 are preserved on the error path.
As per coding guidelines, "MCP responses must follow JSON-RPC 2.0 error format: { jsonrpc: '2.0', error: { code, message }, id } for errors" and "MCP error codes must be: -32001 (Unauthorized), -32603 (Internal error)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mcp.controller.ts` around lines 58 - 75, Wrap the call to
aliasService.resolveAlias inside resolveAlias in a try/catch so any Redis or
other exceptions are caught and returned as a JSON-RPC error (code -32603)
instead of bubbling as an HTTP 500; if resolveAlias returns no apiKey, respond
with a JSON-RPC error object (use code -32603 and a clear "Not Found: Unknown
alias '<alias>'" message) and ensure the id uses (body as Record<string,
unknown>)?.id ?? null so values like 0 are preserved. Reference: resolveAlias
and aliasService.resolveAlias; convert all alias lookup failures and thrown
errors to the MCP JSON-RPC error contract with code -32603 and do not rethrow.
src/mcp/mcp.controller.ts
Outdated
Comment on lines
164
to
169
| // Resolve alias → actual project API key | ||
| const projectApiKey = await this.resolveAlias(alias, req.body, res); | ||
| if (!projectApiKey) return; | ||
|
|
||
| // Validate auth | ||
| const authResult = this.validateAuth(authorization, projectApiKey, req.body, res); |
There was a problem hiding this comment.
Do the cheap Bearer validation before resolving the alias.
POST/GET/DELETE now hit external Redis before any auth check, which lets anonymous callers distinguish registered aliases via the 404/401 split and spends Redis capacity on requests that should be rejected immediately. Split the generic Bearer validation from the project-scoped bits so unauthenticated requests fail before resolveAlias().
As per coding guidelines, "McpController must extract userId from JWT Bearer token via jwt.decode() and validate token before processing requests."
Also applies to: 266-271, 317-322
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mcp.controller.ts` around lines 164 - 169, The code currently calls
resolveAlias(...) before doing the cheap JWT Bearer check, allowing
unauthenticated callers to hit external Redis; move the generic Bearer
validation (extract and decode the token with jwt.decode() and ensure a valid
userId) into McpController before any call to resolveAlias, splitting
validateAuth into two steps: a token-only check that returns 401 on failure and
a project-scoped authorization that runs after resolveAlias; update the call
sites that call resolveAlias then validateAuth (including the other two similar
places) to perform jwt.decode()/token validation first and only then call
resolveAlias(...) and the project-specific validateAuth(...) so unauthenticated
requests are rejected immediately.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/auth/templates/login-page.template.ts (1)
159-166:⚠️ Potential issue | 🟠 MajorOAuth parameters should be HTML-escaped to prevent XSS.
Hidden input values like
redirect_uri,state, andscopeare interpolated directly from query parameters. Malicious values could break out of the attribute and inject HTML/JS.🛡️ Proposed fix with HTML escaping
+const escapeAttr = (str: string | undefined) => + str?.replace(/&/g, '&').replace(/"/g, '"').replace(/</g, '<').replace(/>/g, '>') ?? ''; + <!-- Hidden OAuth parameters --> - <input type="hidden" name="client_id" value="${oauthParams.client_id}"> - <input type="hidden" name="redirect_uri" value="${oauthParams.redirect_uri}"> - <input type="hidden" name="state" value="${oauthParams.state}"> - <input type="hidden" name="code_challenge" value="${oauthParams.code_challenge}"> - <input type="hidden" name="code_challenge_method" value="${oauthParams.code_challenge_method}"> - ${oauthParams.scope ? `<input type="hidden" name="scope" value="${oauthParams.scope}">` : ''} - ${oauthParams.response_type ? `<input type="hidden" name="response_type" value="${oauthParams.response_type}">` : ''} - ${oauthParams.resource ? `<input type="hidden" name="resource" value="${oauthParams.resource}">` : ''} + <input type="hidden" name="client_id" value="${escapeAttr(oauthParams.client_id)}"> + <input type="hidden" name="redirect_uri" value="${escapeAttr(oauthParams.redirect_uri)}"> + <input type="hidden" name="state" value="${escapeAttr(oauthParams.state)}"> + <input type="hidden" name="code_challenge" value="${escapeAttr(oauthParams.code_challenge)}"> + <input type="hidden" name="code_challenge_method" value="${escapeAttr(oauthParams.code_challenge_method)}"> + ${oauthParams.scope ? `<input type="hidden" name="scope" value="${escapeAttr(oauthParams.scope)}">` : ''} + ${oauthParams.response_type ? `<input type="hidden" name="response_type" value="${escapeAttr(oauthParams.response_type)}">` : ''} + ${oauthParams.resource ? `<input type="hidden" name="resource" value="${escapeAttr(oauthParams.resource)}">` : ''}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/templates/login-page.template.ts` around lines 159 - 166, The hidden inputs in login-page.template.ts interpolate oauthParams directly (client_id, redirect_uri, state, code_challenge, code_challenge_method, scope, response_type, resource) and must be HTML-escaped to prevent XSS; add or reuse a utility function (e.g., escapeHtml or htmlEscape) and replace each interpolation in the template with escapeHtml(oauthParams.<field>) for all fields present (including conditional scope/response_type/resource) so every value is safely escaped before injection into the value attribute.src/mcp/mcp.controller.ts (1)
174-176:⚠️ Potential issue | 🔴 CriticalSession reuse is not scoped to the resolved project (cross-tenant risk).
Handlers only check
transports.has(sessionId)and never verifysessionProjectMap.get(sessionId) === projectApiKey. A valid session ID from another project can be reused across aliases.🔒 Suggested guard pattern
+ private isSessionInProject(sessionId: string, projectApiKey: string): boolean { + return this.sessionProjectMap.get(sessionId) === projectApiKey; + } @@ - if (mcpSessionId && this.transports.has(mcpSessionId)) { + if (mcpSessionId && this.transports.has(mcpSessionId)) { + if (!this.isSessionInProject(mcpSessionId, projectApiKey)) { + res.status(HttpStatus.UNAUTHORIZED).json({ + jsonrpc: '2.0', + error: { code: -32001, message: 'Unauthorized: Session does not belong to this project' }, + id: (req.body as Record<string, unknown>)?.id ?? null, + }); + return; + } const transport = this.transports.get(mcpSessionId)!; @@ - if (!mcpSessionId || !this.transports.has(mcpSessionId)) { + if ( + !mcpSessionId || + !this.transports.has(mcpSessionId) || + !this.isSessionInProject(mcpSessionId, projectApiKey) + ) {As per coding guidelines, "Multi-tenant architecture must isolate each project's session and API key scope — never allow cross-project data access."
Also applies to: 273-286, 321-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp.controller.ts` around lines 174 - 176, The transport reuse is missing a tenant scope check: before reusing a cached transport from this.transports for mcpSessionId, verify that sessionProjectMap.get(mcpSessionId) === projectApiKey (or otherwise matches the resolved project's API key); if it does not match, treat the session as invalid for this project (do not reuse — create a new transport and update sessionProjectMap). Apply the same guard to the other reuse sites that reference this.transports and mcpSessionId (the other handler blocks using the same pattern) and ensure sessionProjectMap is set when creating a new transport (e.g., set sessionProjectMap.set(mcpSessionId, projectApiKey) at creation).
🧹 Nitpick comments (2)
docs/specs/partner-meta.md (1)
93-116: Documentation example doesn't reflect the actual implementation.The code example shows direct
JSON.parse(raw) as AliasMetawithout Zod validation, but the actual implementation insrc/alias/partner-meta.service.tscorrectly usesAliasMetaSchema.safeParse(parsed). Consider updating the spec to match the implementation.📝 Suggested update to align with implementation
async getAliasMeta(alias: string): Promise<AliasMeta | null> { const prefix = this.configService.get<string>('ALIAS_REDIS_KEY_PREFIX', 'alias'); const key = `${prefix}-meta:${alias}`; const raw = await this.redis.get(key); if (!raw) return null; - try { - return JSON.parse(raw) as AliasMeta; - } catch { - this.logger.warn(`Failed to parse alias meta for ${alias}`); + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + this.logger.warn(`Failed to parse alias meta JSON for ${alias}`); + return null; + } + const result = AliasMetaSchema.safeParse(parsed); + if (!result.success) { + this.logger.warn(`Invalid alias meta for ${alias}`); return null; } + return result.data; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/specs/partner-meta.md` around lines 93 - 116, The docs example for PartnerMetaService.getAliasMeta is out of date: update the snippet to parse and validate Redis data using the same Zod schema logic as the real implementation — after reading raw from Redis (key built from ALIAS_REDIS_KEY_PREFIX and alias) JSON.parse it, then validate with AliasMetaSchema.safeParse(parsed) and return the parsed data only if safeParse.success is true; on validation failure call this.logger.warn with a helpful message and return null, matching the actual getAliasMeta behavior.src/auth/templates/login-page.template.ts (1)
32-34: Consider sanitizinglogovalue to prevent XSS.While
AliasMetavalidation constrainsloginLogoto emoji strings or https:// URLs, the value is directly interpolated into HTML. If validation is bypassed or expanded, this could become an XSS vector.🛡️ Suggested sanitization
+const escapeHtml = (str: string) => + str.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>').replace(/"/g, '"'); + const logoHtml = logo.startsWith('https://') - ? `<img src="${logo}" alt="${title}" style="max-height:48px;margin-bottom:8px;">` - : `<span style="font-size:36px;">${logo}</span>`; + ? `<img src="${escapeHtml(logo)}" alt="${escapeHtml(title)}" style="max-height:48px;margin-bottom:8px;">` + : `<span style="font-size:36px;">${escapeHtml(logo)}</span>`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auth/templates/login-page.template.ts` around lines 32 - 34, The logoHtml interpolation is vulnerable to XSS because logo (used in the img src, alt and span content) is inserted directly; sanitize and canonicalize it before use in the template. Update the code that builds logoHtml (the logoHtml variable) to: validate/allow only https URLs for the img branch (reject or fallback on invalid schemes), percent-encode or run the URL through a sanitizeUrl helper for the src attribute, and HTML-escape any user-controlled values used in attributes or text (e.g., title and the non-https logo used inside the span). Add or reuse small helpers like sanitizeUrl(url) and escapeHtml(str) and call them from the code that computes logoHtml to ensure neither the src, alt nor span contents can inject HTML/JS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth/auth.controller.ts`:
- Line 23: Replace the relative import for PartnerMetaService with the project
path alias import; update the import statement that currently imports
PartnerMetaService to use "@/alias/partner-meta.service" (i.e., import {
PartnerMetaService } from "@/alias/partner-meta.service") so the file uses the
configured src/* alias instead of a relative path.
- Around line 58-59: The code calls
partnerMetaService.getAliasMeta(projectApiKey) but getAliasMeta expects an
alias; fix both occurrences by first resolving alias = await
this.partnerResolver.resolveAlias(projectApiKey) (or equivalent method used
elsewhere) and then call await this.partnerMetaService.getAliasMeta(alias);
update auth.controller handlers that currently pass projectApiKey into
getAliasMeta to use the resolved alias, and preserve existing error/undefined
handling as in mcp.controller’s usage of getAliasMeta.
In `@src/auth/templates/login-page.template.ts`:
- Around line 29-30: The current code sets both accent and accentDark to
meta?.loginAccentColor when provided, collapsing the gradient; change accentDark
to use either a separately supplied meta?.loginAccentColorDark or compute a
darker variant from meta?.loginAccentColor (e.g. via a small helper like
darkenHexColor(color, amount)), and fall back to the existing default '#764ba2'
if neither is present; update the variables accent and accentDark so accent =
meta?.loginAccentColor ?? '#667eea' and accentDark = meta?.loginAccentColorDark
?? darkenHexColor(meta?.loginAccentColor, 0.15) ?? '#764ba2', adding the
darkenHexColor helper and referencing AliasMeta if you add a new
loginAccentColorDark field.
In `@src/mcp/mcp.controller.ts`:
- Around line 200-208: The code is creating two server instances by calling
serverFactory.createServer(...) both inside the onSessionInitialized callback
and again before server.connect(transport); fix by creating a single server
instance and reusing it: call serverFactory.createServer(projectApiKey, meta ??
undefined) once, store it in a local variable, pass that same instance into
sessionManager.createSession(...) inside the onSessionInitialized handler (or
capture it in the closure) and then call server.connect(transport) on that same
instance so sessionManager.createSession and server.connect operate on the same
Server object.
- Around line 21-22: The imports for AliasService and PartnerMetaService in
mcp.controller.ts currently use relative paths; update them to use the project
path-alias by replacing "../alias/alias.service" with "@/alias/alias.service"
and "../alias/partner-meta.service" with "@/alias/partner-meta.service" so that
the named imports AliasService and PartnerMetaService are imported via the
'@/...' alias convention.
In `@src/mcp/services/mcp-server.factory.ts`:
- Line 5: Change the import of AliasMeta to use the repo path-alias style:
replace the relative import that references '../../alias/partner-meta.service'
with the '@/alias/partner-meta.service' form so the symbol AliasMeta is imported
via the path-alias consistent with repo rules; update the import statement that
declares AliasMeta accordingly.
- Around line 87-90: getOrCreateServer currently always calls createServer and
bypasses the required in-memory caching; change getOrCreateServer to first check
a local Map<string, McpServer> cache for an existing McpServer keyed by
projectApiKey (or combined key if AliasMeta affects identity), return the cached
instance if present, otherwise call createServer(projectApiKey, meta), store the
returned McpServer in that Map, and then return it; ensure only non-serializable
McpServer instances are kept in the Map while any serializable metadata
continues to be persisted to Redis as before.
In `@src/resources/services/resource-registry.service.ts`:
- Line 15: Replace the relative import for AliasMeta with the project path
alias: update the import statement that currently references
'../../alias/partner-meta.service' to use the '@/alias/partner-meta.service'
path; ensure the symbol AliasMeta (imported in resource-registry.service.ts) is
imported from the aliased module and that TypeScript resolves the alias
(tsconfig paths already configured).
In `@src/tools/definitions/widget-control-device.tool.ts`:
- Around line 16-20: The tool currently defines WidgetControlDeviceParamsSchema
and exports WidgetControlDeviceParams, but the repo convention requires using a
ParamsSchema name and exporting a canonical Params type; rename the schema to
ParamsSchema (or create a const ParamsSchema = WidgetControlDeviceParamsSchema)
while keeping the z.object() and .describe() calls intact, then replace the
export with export type Params = z.infer<typeof ParamsSchema> so the file
exports the standard Params type expected by tool definitions.
In `@src/widgets/services/widget.service.ts`:
- Line 13: Replace the relative import of AliasMeta with the repository
path-alias form: locate the import statement that references AliasMeta (import {
AliasMeta } from '../../alias/partner-meta.service';) and change it to use the
"@/..." alias (e.g., import { AliasMeta } from '@/alias/partner-meta.service';)
so the module follows the project's TypeScript path-alias convention.
- Around line 73-83: The embedded JSON in partnerScript and localeScript can be
broken out via hostile input (e.g. "</script>"); update the generation of
partnerScript and localeScript in widget.service.ts to sanitize/escape the JSON
string output before embedding into <script> tags (use a helper like
escapeJsonForScript that takes JSON.stringify(partner) and
JSON.stringify(workingLocales) and replaces dangerous sequences such as
"</script>" -> "<\/script>" and "</" -> "<\/" and optionally converts
U+2028/U+2029), then use those escaped strings when creating partnerScript and
localeScript and continue to insert them via html.replace as before.
---
Outside diff comments:
In `@src/auth/templates/login-page.template.ts`:
- Around line 159-166: The hidden inputs in login-page.template.ts interpolate
oauthParams directly (client_id, redirect_uri, state, code_challenge,
code_challenge_method, scope, response_type, resource) and must be HTML-escaped
to prevent XSS; add or reuse a utility function (e.g., escapeHtml or htmlEscape)
and replace each interpolation in the template with
escapeHtml(oauthParams.<field>) for all fields present (including conditional
scope/response_type/resource) so every value is safely escaped before injection
into the value attribute.
In `@src/mcp/mcp.controller.ts`:
- Around line 174-176: The transport reuse is missing a tenant scope check:
before reusing a cached transport from this.transports for mcpSessionId, verify
that sessionProjectMap.get(mcpSessionId) === projectApiKey (or otherwise matches
the resolved project's API key); if it does not match, treat the session as
invalid for this project (do not reuse — create a new transport and update
sessionProjectMap). Apply the same guard to the other reuse sites that reference
this.transports and mcpSessionId (the other handler blocks using the same
pattern) and ensure sessionProjectMap is set when creating a new transport
(e.g., set sessionProjectMap.set(mcpSessionId, projectApiKey) at creation).
---
Nitpick comments:
In `@docs/specs/partner-meta.md`:
- Around line 93-116: The docs example for PartnerMetaService.getAliasMeta is
out of date: update the snippet to parse and validate Redis data using the same
Zod schema logic as the real implementation — after reading raw from Redis (key
built from ALIAS_REDIS_KEY_PREFIX and alias) JSON.parse it, then validate with
AliasMetaSchema.safeParse(parsed) and return the parsed data only if
safeParse.success is true; on validation failure call this.logger.warn with a
helpful message and return null, matching the actual getAliasMeta behavior.
In `@src/auth/templates/login-page.template.ts`:
- Around line 32-34: The logoHtml interpolation is vulnerable to XSS because
logo (used in the img src, alt and span content) is inserted directly; sanitize
and canonicalize it before use in the template. Update the code that builds
logoHtml (the logoHtml variable) to: validate/allow only https URLs for the img
branch (reject or fallback on invalid schemes), percent-encode or run the URL
through a sanitizeUrl helper for the src attribute, and HTML-escape any
user-controlled values used in attributes or text (e.g., title and the non-https
logo used inside the span). Add or reuse small helpers like sanitizeUrl(url) and
escapeHtml(str) and call them from the code that computes logoHtml to ensure
neither the src, alt nor span contents can inject HTML/JS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc7761b8-c39c-4311-ae94-aa49d2b592fa
📒 Files selected for processing (20)
docs/specs/partner-meta.mdsrc/alias/alias.module.tssrc/alias/partner-meta.service.tssrc/auth/auth.controller.tssrc/auth/templates/login-page.template.tssrc/mcp/mcp.controller.tssrc/mcp/services/mcp-protocol-handler.service.tssrc/mcp/services/mcp-server.factory.tssrc/resources/services/resource-registry.service.tssrc/tools/definitions/get-device.tool.tssrc/tools/definitions/list-devices.tool.tssrc/tools/definitions/widget-control-device.tool.tssrc/tools/definitions/widget-get-device.tool.tssrc/tools/definitions/widget-list-devices.tool.tssrc/tools/services/tool-executor.service.tssrc/tools/services/tool-registry.service.tssrc/widgets/definitions/device-app.widget.tssrc/widgets/services/widget.service.tssrc/widgets/widget-preview.controller.tsviews/widgets/device-app.html
✅ Files skipped from review due to trivial changes (5)
- src/tools/services/tool-executor.service.ts
- src/tools/services/tool-registry.service.ts
- src/widgets/definitions/device-app.widget.ts
- src/widgets/widget-preview.controller.ts
- src/tools/definitions/widget-get-device.tool.ts
| import { TokenRequestDto } from './dto/token-request.dto'; | ||
| import { TokenResponseDto } from './dto/token-response.dto'; | ||
| import { generateLoginPage } from './templates/login-page.template'; | ||
| import { PartnerMetaService } from '../alias/partner-meta.service'; |
There was a problem hiding this comment.
Use @/ alias for the new internal import.
The new PartnerMetaService import should use the project alias instead of a relative path.
As per coding guidelines, "Use path alias @/* pointing to src/* for imports in TypeScript files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/auth/auth.controller.ts` at line 23, Replace the relative import for
PartnerMetaService with the project path alias import; update the import
statement that currently imports PartnerMetaService to use
"@/alias/partner-meta.service" (i.e., import { PartnerMetaService } from
"@/alias/partner-meta.service") so the file uses the configured src/* alias
instead of a relative path.
Comment on lines
+29
to
+30
| const accent = meta?.loginAccentColor ?? '#667eea'; | ||
| const accentDark = meta?.loginAccentColor ?? '#764ba2'; |
There was a problem hiding this comment.
Gradient will be flat when loginAccentColor is provided.
Both accent and accentDark will have the same value when meta?.loginAccentColor is set, resulting in a solid color instead of a gradient. The dark variant should be computed or separately specified.
🐛 Proposed fix to compute darker accent
const accent = meta?.loginAccentColor ?? '#667eea';
- const accentDark = meta?.loginAccentColor ?? '#764ba2';
+ // Compute darker shade or use default gradient end
+ const accentDark = meta?.loginAccentColor
+ ? darkenHexColor(meta.loginAccentColor, 0.2)
+ : '#764ba2';You'll need to add a helper function to darken the hex color, or alternatively add a separate loginAccentColorDark field to AliasMeta.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/auth/templates/login-page.template.ts` around lines 29 - 30, The current
code sets both accent and accentDark to meta?.loginAccentColor when provided,
collapsing the gradient; change accentDark to use either a separately supplied
meta?.loginAccentColorDark or compute a darker variant from
meta?.loginAccentColor (e.g. via a small helper like darkenHexColor(color,
amount)), and fall back to the existing default '#764ba2' if neither is present;
update the variables accent and accentDark so accent = meta?.loginAccentColor ??
'#667eea' and accentDark = meta?.loginAccentColorDark ??
darkenHexColor(meta?.loginAccentColor, 0.15) ?? '#764ba2', adding the
darkenHexColor helper and referencing AliasMeta if you add a new
loginAccentColorDark field.
Comment on lines
+21
to
+22
| import { AliasService } from '../alias/alias.service'; | ||
| import { PartnerMetaService } from '../alias/partner-meta.service'; |
There was a problem hiding this comment.
Use @/ aliases for new internal imports.
The newly added alias service imports should follow the @/* import convention.
As per coding guidelines, "Use path alias @/* pointing to src/* for imports in TypeScript files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mcp.controller.ts` around lines 21 - 22, The imports for AliasService
and PartnerMetaService in mcp.controller.ts currently use relative paths; update
them to use the project path-alias by replacing "../alias/alias.service" with
"@/alias/alias.service" and "../alias/partner-meta.service" with
"@/alias/partner-meta.service" so that the named imports AliasService and
PartnerMetaService are imported via the '@/...' alias convention.
Comment on lines
+200
to
208
| const server = this.serverFactory.createServer(projectApiKey, meta ?? undefined); | ||
| await this.sessionManager.createSession(projectApiKey, userId, server, sessionId); | ||
| this.logger.log(`Session initialized - SessionId: ${sessionId}, UserId: ${userId}`); | ||
| }, | ||
| }); | ||
|
|
||
| // Connect McpServer to the transport | ||
| const server = this.serverFactory.createServer(projectApiKey); | ||
| const server = this.serverFactory.createServer(projectApiKey, meta ?? undefined); | ||
| await server.connect(transport); |
There was a problem hiding this comment.
Server is instantiated twice during initialize flow.
You create one server in onsessioninitialized and another before connect(). This can desync persisted session state from the connected transport.
🔧 Suggested fix (single server instance)
- const transport = new StreamableHTTPServerTransport({
+ const server = this.serverFactory.createServer(projectApiKey, meta ?? undefined);
+ const transport = new StreamableHTTPServerTransport({
sessionIdGenerator: () => randomUUID(),
onsessioninitialized: async (sessionId: string) => {
@@
- const server = this.serverFactory.createServer(projectApiKey, meta ?? undefined);
await this.sessionManager.createSession(projectApiKey, userId, server, sessionId);
this.logger.log(`Session initialized - SessionId: ${sessionId}, UserId: ${userId}`);
},
});
@@
- const server = this.serverFactory.createServer(projectApiKey, meta ?? undefined);
await server.connect(transport);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/mcp.controller.ts` around lines 200 - 208, The code is creating two
server instances by calling serverFactory.createServer(...) both inside the
onSessionInitialized callback and again before server.connect(transport); fix by
creating a single server instance and reusing it: call
serverFactory.createServer(projectApiKey, meta ?? undefined) once, store it in a
local variable, pass that same instance into sessionManager.createSession(...)
inside the onSessionInitialized handler (or capture it in the closure) and then
call server.connect(transport) on that same instance so
sessionManager.createSession and server.connect operate on the same Server
object.
Comment on lines
+87
to
+90
| getOrCreateServer(projectApiKey: string, meta?: AliasMeta): McpServer { | ||
| // For PoC: always create new server per session | ||
| // Future: implement caching/pooling if needed | ||
| return this.createServer(projectApiKey); | ||
| return this.createServer(projectApiKey, meta); |
There was a problem hiding this comment.
getOrCreateServer() still bypasses local server caching.
This method still always creates a new instance, which conflicts with the required local Map<string, McpServer> caching behavior.
♻️ Suggested direction
export class McpServerFactory {
private readonly logger = new Logger(McpServerFactory.name);
+ private readonly servers = new Map<string, McpServer>();
@@
getOrCreateServer(projectApiKey: string, meta?: AliasMeta): McpServer {
- // For PoC: always create new server per session
- // Future: implement caching/pooling if needed
- return this.createServer(projectApiKey, meta);
+ const cacheKey = projectApiKey;
+ const existing = this.servers.get(cacheKey);
+ if (existing) return existing;
+ const server = this.createServer(projectApiKey, meta);
+ this.servers.set(cacheKey, server);
+ return server;
}
}Based on learnings: "Applies to src/mcp/services/{session-manager,mcp-server.factory}.service.ts : McpServer instances from modelcontextprotocol/sdk must be stored in local Map<string, McpServer> cache (non-serializable) while serializable metadata goes in Redis".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp/services/mcp-server.factory.ts` around lines 87 - 90,
getOrCreateServer currently always calls createServer and bypasses the required
in-memory caching; change getOrCreateServer to first check a local Map<string,
McpServer> cache for an existing McpServer keyed by projectApiKey (or combined
key if AliasMeta affects identity), return the cached instance if present,
otherwise call createServer(projectApiKey, meta), store the returned McpServer
in that Map, and then return it; ensure only non-serializable McpServer
instances are kept in the Map while any serializable metadata continues to be
persisted to Redis as before.
| import { OVERVIEW_RESOURCE } from '../definitions/overview.resource'; | ||
| import { DEVICE_APP_WIDGET } from '../../widgets/definitions/device-app.widget'; | ||
| import { WidgetService } from '../../widgets/services/widget.service'; | ||
| import { AliasMeta } from '../../alias/partner-meta.service'; |
There was a problem hiding this comment.
Use @/ alias for AliasMeta import.
Please switch this internal import to the configured @/* alias style.
As per coding guidelines, "Use path alias @/* pointing to src/* for imports in TypeScript files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/resources/services/resource-registry.service.ts` at line 15, Replace the
relative import for AliasMeta with the project path alias: update the import
statement that currently references '../../alias/partner-meta.service' to use
the '@/alias/partner-meta.service' path; ensure the symbol AliasMeta (imported
in resource-registry.service.ts) is imported from the aliased module and that
TypeScript resolves the alias (tsconfig paths already configured).
Comment on lines
+16
to
+20
| const WidgetControlDeviceParamsSchema = z.object({ | ||
| uuid: z.string().describe('Device UUID'), | ||
| }); | ||
|
|
||
| export type WidgetControlDeviceParams = z.infer<typeof WidgetControlDeviceParamsSchema>; |
There was a problem hiding this comment.
Use the standard Params inferred type export in tool definitions.
Line 20 exports WidgetControlDeviceParams, but this repo’s tool-definition convention requires export type Params = z.infer<typeof ParamsSchema>.
♻️ Suggested alignment with repo convention
-const WidgetControlDeviceParamsSchema = z.object({
+const ParamsSchema = z.object({
uuid: z.string().describe('Device UUID'),
});
-export type WidgetControlDeviceParams = z.infer<typeof WidgetControlDeviceParamsSchema>;
+export type Params = z.infer<typeof ParamsSchema>;
+export type WidgetControlDeviceParams = Params;
@@
- schema: WidgetControlDeviceParamsSchema,
+ schema: ParamsSchema,As per coding guidelines: “Define parameter types using Zod z.object() with .describe() for each field, and export the inferred type as export type Params = z.infer<typeof ParamsSchema> in tool definition files”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tools/definitions/widget-control-device.tool.ts` around lines 16 - 20,
The tool currently defines WidgetControlDeviceParamsSchema and exports
WidgetControlDeviceParams, but the repo convention requires using a ParamsSchema
name and exporting a canonical Params type; rename the schema to ParamsSchema
(or create a const ParamsSchema = WidgetControlDeviceParamsSchema) while keeping
the z.object() and .describe() calls intact, then replace the export with export
type Params = z.infer<typeof ParamsSchema> so the file exports the standard
Params type expected by tool definitions.
| import * as Handlebars from 'handlebars'; | ||
| import { readFile, readdir } from 'fs/promises'; | ||
| import { join } from 'path'; | ||
| import { AliasMeta } from '../../alias/partner-meta.service'; |
There was a problem hiding this comment.
Use @/ alias for the new internal import.
The newly added AliasMeta import should follow the repo alias convention.
As per coding guidelines, "Use path alias @/* pointing to src/* for imports in TypeScript files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/widgets/services/widget.service.ts` at line 13, Replace the relative
import of AliasMeta with the repository path-alias form: locate the import
statement that references AliasMeta (import { AliasMeta } from
'../../alias/partner-meta.service';) and change it to use the "@/..." alias
(e.g., import { AliasMeta } from '@/alias/partner-meta.service';) so the module
follows the project's TypeScript path-alias convention.
Comment on lines
+73
to
+83
| const partner = { | ||
| footerText: meta?.widgetFooterText ?? meta?.brandName ?? null, | ||
| logoUrl: meta?.widgetLogoUrl ?? null, | ||
| }; | ||
| const partnerScript = `<script>window.__PARTNER__=${JSON.stringify(partner)}</script>\n`; | ||
|
|
||
| if (Object.keys(workingLocales).length > 0) { | ||
| const localeScript = `<script>window.__I18N__=${JSON.stringify(workingLocales)}</script>\n`; | ||
| html = html.replace('<script>', localeScript + partnerScript + '<script>'); | ||
| } else { | ||
| html = html.replace('<script>', partnerScript + '<script>'); |
There was a problem hiding this comment.
Escape JSON before embedding it in <script> tags.
Current inline serialization allows </script> breakout if partner metadata contains hostile input.
🔧 Suggested hardening patch
export class WidgetService {
@@
+ private serializeForInlineScript(value: unknown): string {
+ return JSON.stringify(value).replace(/</g, '\\u003c').replace(/>/g, '\\u003e');
+ }
+
async readStaticHtml(widgetName: string, meta?: AliasMeta): Promise<string> {
@@
- const partnerScript = `<script>window.__PARTNER__=${JSON.stringify(partner)}</script>\n`;
+ const partnerScript = `<script>window.__PARTNER__=${this.serializeForInlineScript(partner)}</script>\n`;
@@
- const localeScript = `<script>window.__I18N__=${JSON.stringify(workingLocales)}</script>\n`;
+ const localeScript = `<script>window.__I18N__=${this.serializeForInlineScript(workingLocales)}</script>\n`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/widgets/services/widget.service.ts` around lines 73 - 83, The embedded
JSON in partnerScript and localeScript can be broken out via hostile input (e.g.
"</script>"); update the generation of partnerScript and localeScript in
widget.service.ts to sanitize/escape the JSON string output before embedding
into <script> tags (use a helper like escapeJsonForScript that takes
JSON.stringify(partner) and JSON.stringify(workingLocales) and replaces
dangerous sequences such as "</script>" -> "<\/script>" and "</" -> "<\/" and
optionally converts U+2028/U+2029), then use those escaped strings when creating
partnerScript and localeScript and continue to insert them via html.replace as
before.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/discovery/discovery.controller.ts (1)
30-47: Consider removing or updating commented-out root discovery endpoints.The commented-out root discovery endpoints still reference the old
:projectApiKeypattern (e.g.,/auth/:projectApiKey/authorize). If these endpoints are planned for future use, update them to use:alias; otherwise, consider removing the dead code to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/discovery/discovery.controller.ts` around lines 30 - 47, The commented-out getRootAuthServerMetadata() block references the old :projectApiKey path and should be either updated or removed to avoid confusion; either delete the dead code or change all occurrences of ":projectApiKey" to ":alias" and update the documentation string to mention path-aware discovery using "/.well-known/oauth-authorization-server/auth/{alias}", ensuring the function name getRootAuthServerMetadata and the authorization/token/registration endpoint templates are the targeted edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/auth/auth.controller.ts`:
- Around line 52-66: The resolveAlias method currently writes a JSON-RPC error
body for missing aliases; change it to use the same HTTP/REST error handling as
the token flow by throwing or returning an HTTP Not Found error instead: replace
the manual res.status(...).json(...) JSON-RPC response in resolveAlias with
throwing a NestJS NotFoundException (or otherwise returning a conventional REST
error) so callers like authorize and login receive a standard 404 error
consistent with token; locate resolveAlias and aliasService.resolveAlias to
implement this and mirror the token endpoint's behavior (e.g., use
NotFoundException as used in token).
---
Nitpick comments:
In `@src/discovery/discovery.controller.ts`:
- Around line 30-47: The commented-out getRootAuthServerMetadata() block
references the old :projectApiKey path and should be either updated or removed
to avoid confusion; either delete the dead code or change all occurrences of
":projectApiKey" to ":alias" and update the documentation string to mention
path-aware discovery using
"/.well-known/oauth-authorization-server/auth/{alias}", ensuring the function
name getRootAuthServerMetadata and the authorization/token/registration endpoint
templates are the targeted edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13f39ddd-687d-4c0d-9661-36a4c1e1b531
📒 Files selected for processing (3)
src/auth/auth.controller.tssrc/auth/services/discovery.service.tssrc/discovery/discovery.controller.ts
Comment on lines
+52
to
+66
| private async resolveAlias(alias: string, body: unknown, res: Response): Promise<string | null> { | ||
| const apiKey = await this.aliasService.resolveAlias(alias); | ||
| if (!apiKey) { | ||
| res.status(HttpStatus.NOT_FOUND).json({ | ||
| jsonrpc: '2.0', | ||
| error: { | ||
| code: -32004, | ||
| message: `Not Found: Unknown alias '${alias}'`, | ||
| }, | ||
| id: (body as Record<string, unknown>)?.id || null, | ||
| }); | ||
| return null; | ||
| } | ||
| return apiKey; | ||
| } |
There was a problem hiding this comment.
Inconsistent error response format for HTTP endpoints.
The resolveAlias helper returns a JSON-RPC 2.0 error structure (jsonrpc: '2.0', error.code: -32004), but this controller serves OAuth/HTTP endpoints, not JSON-RPC. The authorize and login endpoints calling this helper are standard HTTP flows where clients expect HTTP status codes and conventional error bodies, not JSON-RPC format.
Consider using a standard REST error response or throwing NotFoundException (as done in the token endpoint at line 229) for consistency.
🔧 Suggested approach
private async resolveAlias(alias: string, body: unknown, res: Response): Promise<string | null> {
const apiKey = await this.aliasService.resolveAlias(alias);
if (!apiKey) {
- res.status(HttpStatus.NOT_FOUND).json({
- jsonrpc: '2.0',
- error: {
- code: -32004,
- message: `Not Found: Unknown alias '${alias}'`,
- },
- id: (body as Record<string, unknown>)?.id || null,
- });
+ res.status(HttpStatus.NOT_FOUND).json({
+ statusCode: HttpStatus.NOT_FOUND,
+ message: `Unknown alias '${alias}'`,
+ error: 'Not Found',
+ });
return null;
}
return apiKey;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/auth/auth.controller.ts` around lines 52 - 66, The resolveAlias method
currently writes a JSON-RPC error body for missing aliases; change it to use the
same HTTP/REST error handling as the token flow by throwing or returning an HTTP
Not Found error instead: replace the manual res.status(...).json(...) JSON-RPC
response in resolveAlias with throwing a NestJS NotFoundException (or otherwise
returning a conventional REST error) so callers like authorize and login receive
a standard 404 error consistent with token; locate resolveAlias and
aliasService.resolveAlias to implement this and mirror the token endpoint's
behavior (e.g., use NotFoundException as used in token).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/mcp.controller.ts (1)
174-185:⚠️ Potential issue | 🔴 CriticalEnforce session-to-project binding before reusing transport (tenant isolation).
Current checks only verify session existence, not ownership. A valid user could reuse another project's session id across aliases. Gate every existing-session path by matching
sessionProjectMap.get(mcpSessionId)to resolvedprojectApiKey.Suggested fix pattern
if (!mcpSessionId || !this.transports.has(mcpSessionId)) { // existing bad-request path } +const sessionProjectApiKey = this.sessionProjectMap.get(mcpSessionId); +if (sessionProjectApiKey !== projectApiKey) { + res.status(HttpStatus.UNAUTHORIZED).json({ + jsonrpc: '2.0', + error: { + code: -32001, + message: 'Unauthorized: Session does not belong to this alias.', + }, + id: (req.body as Record<string, unknown>)?.id ?? null, + }); + return; +} const transport = this.transports.get(mcpSessionId)!;As per coding guidelines, "Multi-tenant architecture must isolate each project's session and API key scope — never allow cross-project data access."
Also applies to: 273-290, 321-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp.controller.ts` around lines 174 - 185, The current reuse of transports only checks this.transports.has(mcpSessionId) but doesn't verify ownership; update the reuse guard to also verify sessionProjectMap.get(mcpSessionId) === projectApiKey (where projectApiKey is the resolved API key for the incoming request) before reusing the transport from this.transports.get(mcpSessionId); if the projectApiKey does not match, do not reuse the transport (treat as a new session or reject), and ensure the same check is applied to the other reuse sites that call handleRequest (the other existing-session paths with similar transport reuse logic).
♻️ Duplicate comments (4)
src/mcp/mcp.controller.ts (4)
21-22:⚠️ Potential issue | 🟡 MinorUse
@/imports for new internal services.These new imports still use relative paths; please switch to path aliases.
Suggested fix
-import { AliasService } from '../alias/alias.service'; -import { PartnerMetaService } from '../alias/partner-meta.service'; +import { AliasService } from '@/alias/alias.service'; +import { PartnerMetaService } from '@/alias/partner-meta.service';As per coding guidelines, "Use path alias
@/*pointing tosrc/*for imports in TypeScript files."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp.controller.ts` around lines 21 - 22, Replace the relative imports for the new internal services with the project path-alias form: change the imports of AliasService and PartnerMetaService in mcp.controller.ts to use the "@/..." alias (e.g., import { AliasService } from '@/alias/alias.service' and import { PartnerMetaService } from '@/alias/partner-meta.service') so they resolve via the tsconfig path mapping rather than relative paths.
200-208:⚠️ Potential issue | 🟠 MajorCreate one server instance for initialize flow.
createServer(...)is called twice (Line 200 and Line 207). This can desync the connected server from the server persisted in session state.Suggested fix
+const server = this.serverFactory.createServer(projectApiKey, meta ?? undefined); const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: () => randomUUID(), onsessioninitialized: async (sessionId: string) => { @@ - const server = this.serverFactory.createServer(projectApiKey, meta ?? undefined); await this.sessionManager.createSession(projectApiKey, userId, server, sessionId); }, }); - -const server = this.serverFactory.createServer(projectApiKey, meta ?? undefined); await server.connect(transport);Based on learnings, "The controller manages three local Maps... maintain consistency across these ephemeral data structures during session lifecycle operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp.controller.ts` around lines 200 - 208, createServer(...) is being called twice which can store a different McpServer instance in session state than the one actually connected; instantiate the server once via serverFactory.createServer(projectApiKey, meta ?? undefined), store it in a local variable, pass that same variable to sessionManager.createSession(projectApiKey, userId, server, sessionId) and then call server.connect(transport) so the persisted session server and the connected server are identical (update the code around the createServer, sessionManager.createSession, and server.connect calls).
163-170:⚠️ Potential issue | 🟠 MajorValidate Bearer token before alias resolution in all handlers.
Auth is currently checked after
resolveAlias(), which lets unauthenticated requests hit Redis and infer alias existence by response differences.Suggested fix pattern (apply to POST/GET/DELETE)
-const projectApiKey = await this.resolveAlias(alias, req.body, res); -if (!projectApiKey) return; - -const authResult = this.validateAuth(authorization, alias, req.body, res); +const authResult = this.validateAuth(authorization, alias, req.body, res); if (!authResult) return; +const projectApiKey = await this.resolveAlias(alias, req.body, res); +if (!projectApiKey) return;As per coding guidelines, "McpController must extract
userIdfrom JWT Bearer token viajwt.decode()and validate token before processing requests."Also applies to: 265-270, 314-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp.controller.ts` around lines 163 - 170, Move authentication to run before any alias resolution calls: in McpController handlers that currently call resolveAlias(alias, req.body, res) (e.g., the POST/GET/DELETE handlers where resolveAlias is used), first extract and validate the Bearer JWT (use jwt.decode() to extract userId and validate the token) and call validateAuth(authorization, alias, req.body, res) (or inline its checks) before calling resolveAlias; update the same pattern in the other handlers that use resolveAlias/validateAuth (the additional handlers flagged in the review) so unauthenticated requests never hit Redis or resolveAlias.
60-70:⚠️ Potential issue | 🟠 MajorKeep alias-resolution failures in the MCP JSON-RPC contract.
Line 66 uses
-32004(not allowed here), and Line 69 uses|| null, which drops valid ids like0. Also,aliasService.resolveAliasshould be guarded so Redis exceptions don't escape as framework 500s.Suggested fix
private async resolveAlias(alias: string, body: unknown, res: Response): Promise<string | null> { - const apiKey = await this.aliasService.resolveAlias(alias); - if (!apiKey) { - res.status(HttpStatus.NOT_FOUND).json({ - jsonrpc: '2.0', - error: { - code: -32004, - message: `Not Found: Unknown alias '${alias}'`, - }, - id: (body as Record<string, unknown>)?.id || null, - }); - return null; - } - return apiKey; + try { + const apiKey = await this.aliasService.resolveAlias(alias); + if (!apiKey) { + res.status(HttpStatus.NOT_FOUND).json({ + jsonrpc: '2.0', + error: { + code: -32603, + message: `Not Found: Unknown alias '${alias}'`, + }, + id: (body as Record<string, unknown>)?.id ?? null, + }); + return null; + } + return apiKey; + } catch (error) { + this.logger.error(`Alias resolve failed for alias ${alias}: ${(error as Error).message}`); + res.status(HttpStatus.INTERNAL_SERVER_ERROR).json({ + jsonrpc: '2.0', + error: { code: -32603, message: 'Internal error: failed to resolve alias' }, + id: (body as Record<string, unknown>)?.id ?? null, + }); + return null; + } }As per coding guidelines, "MCP responses must follow JSON-RPC 2.0 error format..." and "MCP error codes must be:
-32001(Unauthorized),-32603(Internal error)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp/mcp.controller.ts` around lines 60 - 70, The resolveAlias flow currently returns a non-allowed error code (-32004), drops valid id values by using ||, and lets Redis exceptions bubble up; update private async resolveAlias to wrap the call to this.aliasService.resolveAlias(alias) in a try/catch, on exception return a JSON-RPC error with code -32603 (Internal error) and a safe message, for a missing alias respond with the allowed MCP code -32001 (Unauthorized) instead of -32004, and preserve numeric ids by using (body as Record<string, unknown>)?.id ?? null (nullish coalescing) when building the JSON-RPC error response; refer to resolveAlias and aliasService.resolveAlias when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/mcp/mcp.controller.ts`:
- Around line 174-185: The current reuse of transports only checks
this.transports.has(mcpSessionId) but doesn't verify ownership; update the reuse
guard to also verify sessionProjectMap.get(mcpSessionId) === projectApiKey
(where projectApiKey is the resolved API key for the incoming request) before
reusing the transport from this.transports.get(mcpSessionId); if the
projectApiKey does not match, do not reuse the transport (treat as a new session
or reject), and ensure the same check is applied to the other reuse sites that
call handleRequest (the other existing-session paths with similar transport
reuse logic).
---
Duplicate comments:
In `@src/mcp/mcp.controller.ts`:
- Around line 21-22: Replace the relative imports for the new internal services
with the project path-alias form: change the imports of AliasService and
PartnerMetaService in mcp.controller.ts to use the "@/..." alias (e.g., import {
AliasService } from '@/alias/alias.service' and import { PartnerMetaService }
from '@/alias/partner-meta.service') so they resolve via the tsconfig path
mapping rather than relative paths.
- Around line 200-208: createServer(...) is being called twice which can store a
different McpServer instance in session state than the one actually connected;
instantiate the server once via serverFactory.createServer(projectApiKey, meta
?? undefined), store it in a local variable, pass that same variable to
sessionManager.createSession(projectApiKey, userId, server, sessionId) and then
call server.connect(transport) so the persisted session server and the connected
server are identical (update the code around the createServer,
sessionManager.createSession, and server.connect calls).
- Around line 163-170: Move authentication to run before any alias resolution
calls: in McpController handlers that currently call resolveAlias(alias,
req.body, res) (e.g., the POST/GET/DELETE handlers where resolveAlias is used),
first extract and validate the Bearer JWT (use jwt.decode() to extract userId
and validate the token) and call validateAuth(authorization, alias, req.body,
res) (or inline its checks) before calling resolveAlias; update the same pattern
in the other handlers that use resolveAlias/validateAuth (the additional
handlers flagged in the review) so unauthenticated requests never hit Redis or
resolveAlias.
- Around line 60-70: The resolveAlias flow currently returns a non-allowed error
code (-32004), drops valid id values by using ||, and lets Redis exceptions
bubble up; update private async resolveAlias to wrap the call to
this.aliasService.resolveAlias(alias) in a try/catch, on exception return a
JSON-RPC error with code -32603 (Internal error) and a safe message, for a
missing alias respond with the allowed MCP code -32001 (Unauthorized) instead of
-32004, and preserve numeric ids by using (body as Record<string, unknown>)?.id
?? null (nullish coalescing) when building the JSON-RPC error response; refer to
resolveAlias and aliasService.resolveAlias when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7653515-65ee-4874-b8e2-dffe1ebfd2a1
📒 Files selected for processing (1)
src/mcp/mcp.controller.ts
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation