A plugin serving an RPC endpoint cannot see the request that called it #5790
Replies: 1 comment
|
First responder note — this thread is the consumer/read-side leg of a seam this repository has already engaged three times on the enforcement side, so the family history is directly relevant to your "is there appetite?" question:
On the shape: your One expectation to set: your deployment (multi-person behind an authenticating proxy) will keep surfacing dsh's single-user assumptions beyond the request context — per-user session listing/ownership is the next one you will hit (sessions are single-writer and process-local in ownership today). A request scope at this seam is the foundation those would all build on, which is an argument for getting the primitive right rather than solving each consumer one at a time. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Host-side RPC dispatch drops the HTTP request before user code runs, so a plugin serving an endpoint has no way to learn anything the deployment's reverse proxy put on that request.
Where it stops
Headers reach DSH and are consumed by the trust and authentication layers through
ConnectionTrustRequest(packages/client/connection/src/rpc.ts:80-81). They stop there. The handler contract is(
packages/client/connection/src/rpc.ts:100-104) — no request, no headers. At the single dispatch site,packages/client/connection/src/rpc-host.ts:240, theRequestis in scope as the enclosingfetch(request: Request)parameter (:208):The request is right there, used on that very line for
request.signal, and everything else about it is dropped.All references are to
dsh-v0.1.2-rc.1(a66e470).Why this matters outside a single-user install
DSH's authentication model authenticates a browser to a process: the launch token and the browser session say this browser may talk to this process, not who is talking. That is the right model for the install DSH is built for.
We run DSH behind an authenticating reverse proxy, where more than one person reaches the same process and the proxy stamps the authenticated end user onto every request it forwards (
X-Forwarded-User, oauth2-proxy's default). The proxy is the only network path to the process. Everything we want to build on top — showing a person their own sessions and not their colleagues', deciding who may act on a given session — needs that one string to reach a plugin. Today it structurally cannot.We are not asking DSH to authenticate anyone, and not asking it to trust a header. We are asking for the request to still be readable at the point where a plugin runs. Whether a given header on it means anything is the deployment's problem, and stays the deployment's problem.
What we could not make work without a change
ctxkey or a Cordis service.ctxis per-plugin, not per-request; two concurrent calls from two people share it.ConnectionFetchRoute) does get the request, but it is outside Remote dispatch, so it buys nothing for the endpoints that matter.connection.rpc.interceptreceives the same three arguments as a handler.The change we would propose
One
AsyncLocalStorageowned byclient-connection, entered around the existing handler call, and one exported reader:ConnectionTrustRequestalready exists and is already exported, so the reader adds no new type. It isundefinedoutside a dispatch, which makes the failure mode "no request in scope" rather than a wrong answer.HostConnectionHandlealso gainsrunWithRequest(request, dispatch), implemented byHostConnectionService, so a transport other than the/apiHTTP route can enter the same scope and an endpoint owner reads one request context whatever carried the call. It is declared on the interface rather than only on the class becausectx.connectionis typed as the interface, which is how any other package would reach it.api-gateway's WebSocket path is the one that needs it, and it is the subject of a companion post (#5791); that half is only worth discussing if this half has appetite.The diff is three files and about 40 lines, most of it documentation. It touches no schema, no envelope, no trust check, and no part of the token exchange. It is attached or linked below.
Two things worth flagging.
Every
/apidispatch now enters the scope, including the calls that never read it: the wrap is unconditional at the dispatch site, because a handler that wants the request cannot be distinguished from one that does not before it runs.AsyncLocalStorage.runis cheap relative to a JSON round trip, but we have not measured it against unary RPC latency in this repository and are happy to, if a number would help the decision.AsyncLocalStorageis already the mechanism used for same-process attribution here.dsh-agentkeeps two of them to carry the liveAgentthrough the driver work it starts (packages/core/agent/src/index.ts:252-253, described as the initiator scope inpackages/core/agent/README.md). This proposal is the same idea one layer out, at the transport edge instead of inside the agent loop. If you would rather not add a second such scope, an explicit fourth handler argument would also solve the problem, at the cost of a breaking change toConnectionRpcHandler.What we are asking
Is there appetite for per-request context at this seam? Specifically:
AsyncLocalStoragescope the shape you would want, or would you rather widen the handler signature?client-connection, as proposed, or somewhere else?We run the change as a local patch over the published
0.1.2-rc.1package today, so this is not speculative and we are not blocked on an answer. We would rather not carry a patch, and we are happy to do the work in whatever shape you would accept — or to hear that you would not, which is also a useful answer.All reactions