Conversation
packages/pds/src/pipethrough.ts
Outdated
| ): Promise<{ url: URL; headers: { authorization?: string } }> => { | ||
| const proxyTo = await parseProxyHeader(ctx, req) | ||
| const defaultProxy = defaultService(ctx, req.path) | ||
| const defaultProxy = defaultService(ctx, req.baseUrl) |
There was a problem hiding this comment.
The mount path doesn't get included in req.path (https://expressjs.com/en/4x/api.html#req.path) so had to change to baseUrl
There was a problem hiding this comment.
I think this works due to an implementation detail of xrpc-server, that it happens to write its catchall route this way:
this.router.use(
'/xrpc/:methodId', // <- baseUrl comes from here
opts?.catchall ?? this.catchall.bind(this),
)If we're able to base this on req.originalUrl (need to account for it containing query params) I think we may be able to avoid a potential footgun. Alternately can probably carefully concat req.baseUrl and req.path together.
There was a problem hiding this comment.
yup yup totally 👍
ended up rewriting to use req.originalUrl
also fwiw, this logic is only used to find the default service so it's relatively low stakes 👌
bnewbold
left a comment
bnewbold
left a comment
There was a problem hiding this comment.
A couple thoughts (I didn't look carefully enough to see if these are implemented):
- only allow exactly
/xrpc/<nsid>: no additional path components, no/xrpc/../blah, etc - only authenticated users (looks like this is enforced?)
atproto-accept-labelers,accept-language,content-type, probably a few other HTTP headers passed through (some request, some response)- bodies: JSON probably the priority, but might want to support other mimetypes? presumably things like size limits are enforced by load-balancers already
502 Bad Gatewayand504 Gateway Timeoutwhen needed? any 5xx or 4xx status codes can probably be passed back as-is?- if the upstream redirects (3xx), PDS could follow the redirects a couple hops, or return an error to user, but probably shouldn't return the redirect to the user
- to start, just the usual rate-limits?
- probably shouldn't support streams (websocket) in this early impl (!)
- will probably need to figure out some way to hint the level of auth ("scope") at some point: app-password, OAuth scope, etc
| try { | ||
| const auth = await ctx.authVerifier.access({ req }) |
There was a problem hiding this comment.
It would be kind of nice to bail early and not bother with the work of auth'ing if the request is non-lexrpc.
There was a problem hiding this comment.
nice - i split up the createUrlAndHeaders function and now do url + audience before checking auth and then create the signed headers after checking auth 👌
| let res: Response | ||
| let buffer: ArrayBuffer | ||
| try { | ||
| res = await fetch(url, reqInit) |
There was a problem hiding this comment.
Something to look out for in general is ensuring we always read the request body. By not reading the body, I think we'd end-up leaking connections and other resources.
There was a problem hiding this comment.
I think we're all set on this, but just wanted to put it out there!
There was a problem hiding this comment.
yup good looks 👍
think we always do read it - but taking a look back at it, i'm gonna adjust the error handling just a smidge 👌
devinivy
left a comment
devinivy
left a comment
There was a problem hiding this comment.
Looks good. This is gonna rule.
3 participants
Allow the PDS to proxy generic
/xrpc/endpoints based onatproto-proxy.Auth with the PDS is validated and a service auth token for the did is signed and attached to the outgoing request.