fix(gateway): auto-approve loopback scope upgrades#22365
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const isSilentPairing = | ||
| isLocalLoopbackClient || (isLocalClient && reason === "not-paired"); |
There was a problem hiding this comment.
Silent pairing bypasses proxy-header guard
isLocalLoopbackClient is isLoopbackAddress(remoteAddr), which only checks whether the TCP socket's remote address is loopback. It does not account for untrusted proxy headers. When the gateway runs behind a local reverse proxy (e.g., nginx on the same host), remoteAddr is 127.0.0.1 even though the actual client is remote. The existing isLocalClient (isLocalDirectRequest) was designed to prevent exactly this — it rejects loopback connections that carry proxy headers from untrusted sources.
With this change, a remote client connecting through an untrusted local reverse proxy would get isSilentPairing = true for scope-upgrade and role-upgrade reasons, silently auto-approving privilege escalation that should require interactive approval.
Consider gating on isLocalClient instead (which already accounts for proxies), or adding a check for !hasUntrustedProxyHeaders:
| const isSilentPairing = | |
| isLocalLoopbackClient || (isLocalClient && reason === "not-paired"); | |
| const isSilentPairing = | |
| (isLocalLoopbackClient && !hasUntrustedProxyHeaders) || (isLocalClient && reason === "not-paired"); |
This preserves the intended loopback auto-approve behavior while maintaining the proxy-header safety net.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server/ws-connection/message-handler.ts
Line: 658-659
Comment:
**Silent pairing bypasses proxy-header guard**
`isLocalLoopbackClient` is `isLoopbackAddress(remoteAddr)`, which only checks whether the TCP socket's remote address is loopback. It does not account for untrusted proxy headers. When the gateway runs behind a local reverse proxy (e.g., nginx on the same host), `remoteAddr` is `127.0.0.1` even though the actual client is remote. The existing `isLocalClient` (`isLocalDirectRequest`) was designed to prevent exactly this — it rejects loopback connections that carry proxy headers from untrusted sources.
With this change, a remote client connecting through an untrusted local reverse proxy would get `isSilentPairing = true` for scope-upgrade and role-upgrade reasons, silently auto-approving privilege escalation that should require interactive approval.
Consider gating on `isLocalClient` instead (which already accounts for proxies), or adding a check for `!hasUntrustedProxyHeaders`:
```suggestion
const isSilentPairing =
(isLocalLoopbackClient && !hasUntrustedProxyHeaders) || (isLocalClient && reason === "not-paired");
```
This preserves the intended loopback auto-approve behavior while maintaining the proxy-header safety net.
How can I resolve this? If you propose a fix, please make it concise.| const ws2 = new WebSocket(`ws://127.0.0.1:${port}`, { | ||
| headers: { host: "gateway.public.example" }, | ||
| }); |
There was a problem hiding this comment.
Test connects via loopback, contradicting expected failure
This test connects to ws://127.0.0.1:${port}, so the socket's remoteAddr will be 127.0.0.1 (or ::ffff:127.0.0.1). With the new logic at line 658-659 in message-handler.ts, isLocalLoopbackClient = isLoopbackAddress(remoteAddr) = true, making isSilentPairing = true regardless of the Host header override. The scope upgrade would be silently auto-approved, and res.ok should be true — but line 966 asserts res.ok to be false.
Overriding the Host header alone doesn't change the TCP remoteAddr. To properly test that non-local-host connections require interactive pairing, the test would need to simulate a non-loopback remoteAddr (or the production logic needs to also check the Host header when deciding isSilentPairing).
The PR description notes the e2e tests could not be run in the author's environment, which may explain why this wasn't caught.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server.auth.e2e.test.ts
Line: 956-958
Comment:
**Test connects via loopback, contradicting expected failure**
This test connects to `ws://127.0.0.1:${port}`, so the socket's `remoteAddr` will be `127.0.0.1` (or `::ffff:127.0.0.1`). With the new logic at line 658-659 in `message-handler.ts`, `isLocalLoopbackClient = isLoopbackAddress(remoteAddr) = true`, making `isSilentPairing = true` regardless of the Host header override. The scope upgrade would be silently auto-approved, and `res.ok` should be `true` — but line 966 asserts `res.ok` to be `false`.
Overriding the Host header alone doesn't change the TCP `remoteAddr`. To properly test that non-local-host connections require interactive pairing, the test would need to simulate a non-loopback `remoteAddr` (or the production logic needs to also check the Host header when deciding `isSilentPairing`).
The PR description notes the e2e tests could not be run in the author's environment, which may explain why this wasn't caught.
How can I resolve this? If you propose a fix, please make it concise.
|
you have been detected be spamming with unwarranted prs and issues and your issues and prs have been automatically closed. please read contributing guide Contributing.md. |
3 participants
What changed
remoteAddris loopback), allowing role/scope upgrade handshakes to auto-approve without interactive pairing.src/gateway/server.auth.e2e.test.tsto verify loopback paired-device scope upgrades can proceed without prompts.Why this fixes it
Subagent/session calls in loopback token mode can require higher privileges than a previously paired read-scope session used earlier, causing
pairing requiredhandshakes duringsessions_spawn. For loopback connections, the server now treats those upgrades as local auto-approve paths, matching operator expectations for localhost tooling.Tests
pnpm vitest run src/gateway/auth.test.tspnpm vitest run --config vitest.e2e.config.ts src/gateway/server.auth.e2e.test.ts -t "scope upgrades"@discordjs/voice), so full e2e pairing assertion could not complete here.Edge cases handled
Greptile Summary
This PR aims to auto-approve device pairing scope/role upgrades for loopback connections, addressing friction when subagent/session calls in loopback token mode require higher privileges. The approach introduces
isLocalLoopbackClient(checking onlyisLoopbackAddress(remoteAddr)) alongside the existingisLocalClient(isLocalDirectRequest) to broaden which connections get silent (auto-approved) pairing.Key issues found:
Proxy-header bypass in
isSilentPairinglogic: The newisLocalLoopbackClientvariable does not account for untrusted proxy headers, unlike the existingisLocalClientwhich explicitly guards against this. When the gateway runs behind a local reverse proxy,remoteAddris loopback even for remote clients, meaning scope/role-upgrade pairing would be silently auto-approved. Adding a!hasUntrustedProxyHeadersguard to theisLocalLoopbackClientbranch would preserve the intended behavior while maintaining the existing security invariant.Test likely fails with the new logic: The modified test "requires pairing for scope upgrades from non-local host" connects to
ws://127.0.0.1:${port}with only a Host header override. SinceremoteAddrremains loopback,isLocalLoopbackClientwould betrue, causing silent auto-approval — contradicting the test's assertion thatres.ok === false. The PR notes the e2e tests could not be run in the author's environment.Confidence Score: 2/5
isLocalLoopbackClientcheck bypasses the proxy-header safety guard thatisLocalDirectRequestenforces, creating a potential privilege escalation path for remote clients routed through a local proxy, and (2) the modified scope-upgrade test likely fails because the test connects via loopback (remoteAddr= 127.0.0.1) while asserting non-local behavior. The e2e tests were not run by the author.src/gateway/server/ws-connection/message-handler.ts(lines 658-659) — theisSilentPairinglogic needs a proxy-header guard.src/gateway/server.auth.e2e.test.ts(lines 956-967) — test assertion likely conflicts with the new logic.Last reviewed commit: eae8774