refactor: harden CLI auth/bridge/webfetch and PAT limits from deep re…#1141
refactor: harden CLI auth/bridge/webfetch and PAT limits from deep re…#1141cjroth wants to merge 1 commit into
Conversation
…view Review fixes for the CLI backend-auth work (#1064/#1135), split out of the settings-redesign branch to keep that PR design-only: - enable a generous per-key PAT rate limit (300/min) instead of disabling it - single-source the webfetch tool name across the permission gates and ACP kind mapping; document the auto-allow rationale - unify the CLI's two FetchFn types; interpolate the heartbeat banner interval - isolate bridge heartbeat tick failures and connection-close cleanup - replace object-boxed loop counters in the webfetch HTML scanner; narrow the DNS-validation types to drop a dead optional chain - export the 90-day PAT expiry as a named constant shared with test fixtures - fix the RFC 8628 citation scope and pat-lifecycle doc wording Co-authored-by: Cursor <cursoragent@cursor.com>
Semgrep Security ScanNo security issues found. |
|
Preview environment deployed 🚀
Stack: Auto-destroys on PR close/merge. Login via the bundled Keycloak realm — |
PR Metrics
Updated Fri, 24 Jul 2026 20:30:05 GMT · run #2438 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔭 thunder-deep-review (advisory)
Complements the other bots — surfaces only what they did not flag. Never approves, never requests changes, never gates merge.
head: 91eda76f9658 · mode: single · deferred 0 item(s) already reported by other bots (best-effort dedup)
| appUrl: string = resolveAppUrl(), | ||
| ): string => { | ||
| const settingsPath = config.protocol === 'acp' ? '/settings/agents' : '/settings/mcp-servers' | ||
| const settingsPath = config.protocol === 'acp' ? '/settings/agents' : '/settings/connections' |
There was a problem hiding this comment.
🚫 Blocking — MCP bridge banner now points users to a route that doesn't exist
Heads up — the non-ACP branch here now builds the pairing URL from /settings/connections, but I can't find that route anywhere in the frontend. src/app.tsx registers preferences, models, devices, mcp-servers, skills, agents, integrations (and dev-settings) under /settings, with a top-level * fallback to /not-found — there's no connections. So a user following this banner lands on a broken/empty page. It also looks like this wasn't a coordinated rename: the settings sidebar (settings-sidebar.tsx), the OAuth callback (oauth-callback.tsx), and the deep-link handler (use-deep-link-listener.ts) all still navigate to /settings/mcp-servers. The bridge test was updated to expect /settings/connections, so it happily passes and won't catch this. Could we either keep this pointing at /settings/mcp-servers, or — if a connections route really is coming — land that route first so the banner isn't a dead link in the meantime?
1 participant
…view
Review fixes for the CLI backend-auth work (#1064/#1135), split out of the settings-redesign branch to keep that PR design-only: