Let a browser session reach an MCP tool, not just the gate - #1180
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: This PR ensures MCP tool/resource calls can be authorized using the same browser session cookie that passed the HTTP gate (not just a bearer token), fixing cases where MCP internals (e.g., resolving Changes:
Technical Notes: Cookie forwarding is required because MCP handlers may resolve protected schemas/artifacts on behalf of the caller; using bearer-only credentials could incorrectly deny session-authenticated browser clients behind the gate. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| return `${prefix}.${signature}`; | ||
| } | ||
|
|
||
| const SESSION = sealSession({ policy: 'keycloak', subject: 'jane' }); |
There was a problem hiding this comment.
enterprise/e2e/auth-closed/playwright/mcp-session.spec.js:28 SESSION is minted once at module load with expiry = issued + 3600, so a long-running run (or a delayed retry) could cross the expiry boundary and make this suite flaky. Consider minting the session closer to each test execution to keep failures deterministic.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| SESSION | ||
| ); | ||
| expect(listed.status()).toBe(200); | ||
| const resources = (await listed.json()).result.resources; |
There was a problem hiding this comment.
enterprise/e2e/auth-closed/playwright/mcp-session.spec.js:78 This assumes resources/list returned a success envelope; MCP/JSON-RPC errors can still be returned with HTTP 200, which would fail here with a TypeError rather than a clear assertion. Consider asserting error is absent before dereferencing result.resources.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 25 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="enterprise/e2e/auth-closed/playwright/mcp-session.spec.js">
<violation number="1" location="enterprise/e2e/auth-closed/playwright/mcp-session.spec.js:78">
P3: This dereferences `result.resources` without first asserting that `error` is absent. Since JSON-RPC/MCP errors can be returned with HTTP 200, an unexpected error response would fail with an unclear `TypeError` instead of a clear assertion message. Consider asserting `body.error` is undefined before accessing `result.resources`.</violation>
<violation number="2" location="enterprise/e2e/auth-closed/playwright/mcp-session.spec.js:112">
P2: The forged-session IIFE (lines 112-126) duplicates the entire `sealSession` logic just to override the signing secret. If the session format changes, both paths need updating. Consider making `sealSession` accept an optional `secret` parameter so the forged test can call `sealSession({...}, 'a-secret-nobody-here-signs-with')` instead of maintaining a second copy.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Benchmark Index (community)
Details
| Benchmark suite | Current: 7dbce12 | Previous: ecddcf2 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
317 ms |
240 ms |
1.32 |
Add one schema (100 existing) |
32 ms |
24 ms |
1.33 |
Add one schema (1000 existing) |
97 ms |
82 ms |
1.18 |
Add one schema (10000 existing) |
869 ms |
821 ms |
1.06 |
Update one schema (1 existing) |
23 ms |
17 ms |
1.35 |
Update one schema (101 existing) |
33 ms |
24 ms |
1.38 |
Update one schema (1001 existing) |
101 ms |
81 ms |
1.25 |
Update one schema (10001 existing) |
811 ms |
828 ms |
0.98 |
Cached rebuild (1 existing) |
8 ms |
6 ms |
1.33 |
Cached rebuild (101 existing) |
10 ms |
8 ms |
1.25 |
Cached rebuild (1001 existing) |
35 ms |
26 ms |
1.35 |
Cached rebuild (10001 existing) |
287 ms |
221 ms |
1.30 |
Index 100 schemas |
603 ms |
419 ms |
1.44 |
Index 1000 schemas |
1506 ms |
1294 ms |
1.16 |
Index 10000 schemas |
13803 ms |
12490 ms |
1.11 |
Index 10000 schemas (custom meta-schema) |
16653 ms |
14748 ms |
1.13 |
Index 10000 schemas ($ref fan-out) |
16692 ms |
14514 ms |
1.15 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Benchmark Index (enterprise)
Details
| Benchmark suite | Current: 7dbce12 | Previous: ecddcf2 | Ratio |
|---|---|---|---|
Add one schema (0 existing) |
313 ms |
379 ms |
0.83 |
Add one schema (100 existing) |
105 ms |
105 ms |
1 |
Add one schema (1000 existing) |
248 ms |
148 ms |
1.68 |
Add one schema (10000 existing) |
847 ms |
664 ms |
1.28 |
Update one schema (1 existing) |
81 ms |
98 ms |
0.83 |
Update one schema (101 existing) |
87 ms |
102 ms |
0.85 |
Update one schema (1001 existing) |
249 ms |
147 ms |
1.69 |
Update one schema (10001 existing) |
857 ms |
659 ms |
1.30 |
Cached rebuild (1 existing) |
7 ms |
6 ms |
1.17 |
Cached rebuild (101 existing) |
9 ms |
7 ms |
1.29 |
Cached rebuild (1001 existing) |
28 ms |
21 ms |
1.33 |
Cached rebuild (10001 existing) |
227 ms |
168 ms |
1.35 |
Index 100 schemas |
580 ms |
645 ms |
0.90 |
Index 1000 schemas |
1303 ms |
1306 ms |
1.00 |
Index 10000 schemas |
17179 ms |
11372 ms |
1.51 |
Index 10000 schemas (custom meta-schema) |
15968 ms |
13995 ms |
1.14 |
Index 10000 schemas ($ref fan-out) |
16011 ms |
14129 ms |
1.13 |
This comment was automatically generated by workflow using github-action-benchmark.
1 participant
Signed-off-by: Juan Cruz Viotti jv@jviotti.com