Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
TL;DR — Replaces a domain-suffix trust check in Key changes
Summary ｜ 2 files ｜ 2 commits ｜ base: Strict origin validation for work app MCP URLs
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, correct security fix. The toolUrl.origin === base.origin check properly validates scheme + hostname + port in one comparison, eliminating the public-suffix bypass (co.uk, com.au, etc.) and the overly permissive subdomain matching. Tests cover the important vectors. No issues found.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Low
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
work-app-mcp.test.ts:55Missing test for undefined baseUrl parameter — guards against credential leakage during misconfiguration
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
work-app-mcp.test.ts:55Missing test for malformed URL input — defensive error handling coverage
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-executed security fix that correctly addresses the public-suffix bypass vulnerability. The implementation using URL.origin equality is sound — it properly validates scheme, hostname, and port together per the WHATWG URL Standard. The security reviewers confirmed no bypass vectors exist in the new implementation.
The two suggestions above are test coverage improvements that would protect these security-critical code paths from accidental regression. They're straightforward additions and don't block merge.
Great work hardening the MCP URL validation! 🔒
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
2 | 0 | 0 | 0 | 2 | 0 | 0 |
| Total | 2 | 0 | 0 | 0 | 2 | 0 | 0 |
Note: Security reviewer confirmed the origin equality check is robust — userinfo, query strings, and fragments do not affect origin comparison, and path traversal/encoding is normalized by the URL parser.
| 'https://api.example.com' | ||
| ) | ||
| ).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Missing test for undefined baseUrl parameter
Issue: The explicit early return if (!baseUrl) return false (line 11 of implementation) is untested.
Why: This branch is security-critical: if INKEEP_AGENTS_API_URL is undefined in production (misconfiguration), the function should reject ALL URLs to prevent credential leakage. Without a test, a future refactor could accidentally remove this guard.
Fix:
it('returns false when baseUrl is undefined', () => {
expect(
isTrustedWorkAppMcpUrl(
'https://api.example.com/work-apps/slack/mcp',
path,
undefined
)
).toBe(false);
});Refs:
| 'https://api.example.com' | ||
| ) | ||
| ).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🟡 Minor: Missing test for malformed URL input
Issue: The catch block (lines 16-17 of implementation) that handles URL parsing errors is untested.
Why: If an attacker supplies a malformed URL that causes URL parsing to throw, the function should return false (safe default). The current implementation does this correctly, but without a test, this defensive behavior could be accidentally removed during refactoring.
Fix:
it('returns false for malformed URL input', () => {
expect(
isTrustedWorkAppMcpUrl(
'not-a-valid-url',
path,
'https://api.example.com'
)
).toBe(false);
});Refs:
Ito Test Report ❌19 test cases ran. 2 failed, 17 passed. Overall, 19 test cases ran with 17 passing and 2 failing, indicating core MCP/manage functionality is largely stable: project and tool access PUT/GET persistence worked for GitHub/Slack, deep-link routing and refresh/back-forward remained stable, strict origin+path trust checks behaved correctly (including query/hash handling), spoofed URL attacks were treated as untrusted, invalid GitHub API keys returned 401, and cross-tenant/cross-project abuse attempts were blocked with no unauthorized state changes. ❌ Failed (2)
🟠 Slack MCP unauthorized requests return 500 instead of 401
Relevant code:
const authHeader = c.req.header('Authorization');
if (!authHeader) {
throw createApiError({
code: 'unauthorized',
message: 'Missing required header: Authorization',
extensions: {
parameter: {
in: 'header',
name: 'Authorization',
},
},
});
}
app.onError((err, c) => {
const message = err.message || 'Internal server error';
logger.error({ error: err }, 'Slack MCP error');
return c.json({ jsonrpc: '2.0', error: { code: -32603, message }, id: null }, 500);
});
const errorCodeToHttpStatus: Record<z.infer<typeof ErrorCode>, number> = {
bad_request: 400,
unauthorized: 401,
forbidden: 403,
not_found: 404,
conflict: 409,
too_many_requests: 429,
unprocessable_entity: 422,
internal_server_error: 500,
};🟠 Rapid double-submit on MCP creation does not create duplicates
Relevant code:
const handleSubmit = async () => {
if (!isFormValid) return;
setIsSubmitting(true);
try {
const slackMcpUrl = `${PUBLIC_INKEEP_AGENTS_API_URL}/work-apps/slack/mcp`;
const toolId = generateId();
const newTool = await createMCPTool(tenantId, projectId, {
id: toolId,
name: name.trim(),
const id = body.id || generateId();
const tool = await createTool(db)({
...body,
tenantId,
projectId,
id,
});
export const createTool = (db: AgentsManageDatabaseClient) => async (params: ToolInsert) => {
const now = new Date().toISOString();
const [created] = await db
.insert(tools)
.values({
...params,
createdAt: now,
updatedAt: now,
})
.returning();✅ Passed (17)
Commit: Tell us how we did: Give Ito Feedback |
|
Deployment failed with the following error: |
There was a problem hiding this comment.
PR Review Summary
(0) New Issues | Risk: Low
🕐 Pending Recommendations (2)
- 🟠
work-app-mcp.test.ts:55Missing test for undefined baseUrl parameter — guards against credential leakage during misconfiguration - 🟡
work-app-mcp.test.ts:55Missing test for malformed URL input — defensive error handling coverage
💡 APPROVE WITH SUGGESTIONS
Summary: This delta review covers only the merge commit (2464e3ff) — the actual PR files (work-app-mcp.ts and work-app-mcp.test.ts) are unchanged since the prior review. No new issues introduced.
The two test coverage suggestions from the previous review remain pending. They're straightforward additions that would protect security-critical code paths from regression, but don't block merge.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: No subagents dispatched — delta contained only a merge commit with no changes to PR files.
2 participants
Motivation
isTrustedWorkAppMcpUrlused a suffix-based hostname check derived from the last two labels of the base hostname which allowed public-suffix bypasses (e.g.,co.uk) and ignored scheme/port, risking leakage of privileged MCP API keys to attacker-controlled hosts.Description
isTrustedWorkAppMcpUrlwith an exact origin equality check usingtoolUrl.origin === base.originand require exact path match viatoolUrl.pathname === pathinpackages/agents-core/src/utils/work-app-mcp.ts.baseDomainderivation andendsWithchecks that permitted public-suffix or subdomain matches.packages/agents-core/src/utils/__tests__/work-app-mcp.test.tsthat assert a valid trusted origin passes and that attacker-controlled multi-part TLD domains, mismatched scheme, mismatched port, and mismatched path are rejected.Testing
biome checkon the modified files, which passed (biome check src/utils/work-app-mcp.ts src/utils/__tests__/work-app-mcp.test.ts).vitestfor the new test file, and all tests passed (pnpm --filter agents-core exec vitest --run src/utils/__tests__/work-app-mcp.test.ts: 1 test file, 5 tests, all passed).Codex Task