Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 #366

Merged
merged 3 commits into from
Jan 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 68 additions & 15 deletions packages/access-api/src/ucanto/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,50 @@
import * as Ucanto from '@ucanto/interface'
import * as Client from '@ucanto/client'

const BadGatewayHTTPErrorResult = {
/**
* Given unknown error, detect whether it is an upstream HTTPError.
* If so, return a result object indicating generic 'Bad Gateway'.
* Otherwise, if error is not a bad gateway error, return undefined
*
* @param {unknown} error - error encountered when proxying a ucanto invocation to an upstream url
*/
catch(error) {
if (!error || typeof error !== 'object') {
return
}
const status = 'status' in error ? Number(error.status) : undefined
const isServerError = status !== undefined && status >= 500 && status < 600
if (!isServerError) {
return
}
return {
error: true,
status: 502,
statusText: 'Bad Gateway',
'x-proxy-error': error,
}
},
}

/**
* default catchInvocationError value for createProxyHandler.
* It catches `HTTPError` errors to an error result with status=502 and statusText='Bad Gateway'
*
* @param {unknown} error
*/
function defaultCatchInvocationError(error) {
const badGatewayResult = BadGatewayHTTPErrorResult.catch(error)
if (badGatewayResult) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably send error to sentry here

return badGatewayResult
}
throw error
}

/**
* @template {Ucanto.ConnectionView<any>} [Connection=Ucanto.ConnectionView<any>]
* @param {object} options
* @param {(error: unknown) => Promise<unknown>} [options.catchInvocationError] - catches any error that comes from invoking the proxy invocation on the connection. If it returns a value, that value will be the proxied invocation result.
* @param {{ default: Connection, [K: Ucanto.UCAN.DID]: Connection }} options.connections
* @param {Ucanto.Signer} [options.signer]
*/
Expand All @@ -16,8 +57,12 @@ export function createProxyHandler(options) {
* @returns {Promise<Ucanto.Result<any, { error: true }>>}
*/
return async function handleInvocation(invocationIn, context) {
const { connections, signer } = options
const { audience, capabilities } = invocationIn
const {
connections,
signer,
catchInvocationError = defaultCatchInvocationError,
} = options
const { audience, capabilities, expiration, notBefore } = invocationIn
const connection = connections[audience.did()] ?? connections.default
// eslint-disable-next-line unicorn/prefer-logical-operator-over-ternary, no-unneeded-ternary
const proxyInvocationIssuer = signer
Expand All @@ -28,18 +73,26 @@ export function createProxyHandler(options) {
: // this works, but involves lying about the issuer type (it wants a Signer but context.id is only a Verifier)
// @todo obviate this type override via https://github.com/web3-storage/ucanto/issues/195
/** @type {Ucanto.Signer} */ (context.id)

const [result] = await Client.execute(
[
Client.invoke({
issuer: proxyInvocationIssuer,
capability: capabilities[0],
audience,
proofs: [invocationIn],
}),
],
/** @type {Client.ConnectionView<any>} */ (connection)
)
return result
const proxyInvocation = Client.invoke({
issuer: proxyInvocationIssuer,
capability: capabilities[0],
audience,
proofs: [invocationIn],
Comment on lines +77 to +80
Copy link
Contributor

@hugomrdias hugomrdias Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add the other props from invocationIn like expiration, etc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github is forcing me to add another comments because i approved instead of requesting changes lol

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why? I don't think that will add anything. All that info will already be in the resulting proxyInvocation proof chain because the whole invocationIn (including its expiration is in proofs). wdyt @Gozala
  2. That's not related to the point of this PR, so if you're proposing changing the proxy logic, let's do it as a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? I don't think that will add anything. All that info will already be in the resulting proxyInvocation proof chain because the whole invocationIn (including its expiration is in proofs). wdyt @Gozala

You are correct that expiration in the invocationIn will restrict time bounds, however if you do not specify expiration here it will automatically default to 30secs (if I'm not mistaken), which is to say it may be shorter than what's in the invocationIn.

In other words I think it is a good idea to include expiration and notBefore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also prompted me to look at take another look at the spec and it seems to state

All proofs MUST contain time bounds equal to or broader than the UCAN being delegated. If the proof expires before the outer UCAN — or starts after it — the reader MUST treat the UCAN as invalid.

Which is to say that UCANs that not copying those time bounds would render a UCAN invalid, that said I'm not sure if this is how ucanto does it, but I'll check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I didn't realize it would default.

44a054b

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is to say that UCANs that not copying those time bounds would render a UCAN invalid, that said I'm not sure if this is how ucanto does it, but I'll check

I read it differently... in this case of proxying, the incoming invocationIn becomes the proof. Let's say it expires in 24hrs. Let's say without copying over that expiry, the proxyInvocation gets created with a 30sec expiry.

All proofs MUST contain time bounds equal to or broader than the UCAN being delegated.

The time bounds of the proof invocationIn is [t0, 0 + 24hrs]. The time bounds of 'the UCAN being delegated' I think is the proxyInvocation whose time bounds is [t1, t1 + 30sec]. Assuming t1 - t0 ~= 0, then I think it's fair to say the time bounds of the proof are broader than the invocation/delegation.

If the proof expires before the outer UCAN — or starts after it — the reader MUST treat the UCAN as invalid.

The proof would not expire before the 'outer UCAN'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it would be bad if invocationIn expiry was like 15sec, and the proxyInvocation expiry defaulted to 30sec. So that makes it worth copying the expiration for that scenario

expiration,
notBefore,
})
try {
const [result] = await Client.execute(
[proxyInvocation],
/** @type {Client.ConnectionView<any>} */ (connection)
)
return result
} catch (error) {
if (catchInvocationError) {
const caughtResult = await catchInvocationError(error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this to be a promise ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, but resolving a returned promise is intentional so the process of building a result from the error can be async.

return caughtResult
}
throw error
}
}
}
33 changes: 26 additions & 7 deletions packages/access-api/test/helpers/upload-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,18 @@ export function ucantoServerNodeListener(ucantoServer) {
for await (const chunk of request) {
chunks.push(chunk)
}
const { headers, body } = await ucantoServer.request({
headers: /** @type {Record<string,string>} */ ({ ...request.headers }),
body: Buffer.concat(chunks),
})
response.writeHead(200, headers)
response.write(body)
response.end()
try {
const { headers, body } = await ucantoServer.request({
headers: /** @type {Record<string,string>} */ ({ ...request.headers }),
body: Buffer.concat(chunks),
})
response.writeHead(200, headers)
response.write(body)
response.end()
} catch (error) {
response.writeHead(500)
response.end(error)
}
}
}

Expand All @@ -67,3 +72,17 @@ export function serverLocalUrl(address) {
throw new Error(`cant determine local url from address`)
return new URL(`http://localhost:${address.port}`)
}

/**
* @param {import('node:http').Server} server
* @returns Promise<URL>
*/
export async function listen(server) {
await new Promise((resolve, reject) => {
server.listen(0, () => {
// eslint-disable-next-line unicorn/no-useless-undefined
resolve(undefined)
})
})
return serverLocalUrl(server.address())
}
107 changes: 107 additions & 0 deletions packages/access-api/test/ucanto-proxy.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import * as ed25519 from '@ucanto/principal/ed25519'
import { createProxyHandler } from '../src/ucanto/proxy.js'
// eslint-disable-next-line no-unused-vars
import * as Ucanto from '@ucanto/interface'
import * as nodeHttp from 'node:http'
import { listen, ucantoServerNodeListener } from './helpers/upload-api.js'
import * as HTTP from '@ucanto/transport/http'

describe('ucanto-proxy', () => {
it('proxies invocations to another ucanto server', async () => {
Expand Down Expand Up @@ -89,4 +92,108 @@ describe('ucanto-proxy', () => {
'proxy result is same returned from upstream'
)
})
it('when upstream responds with status=500, proxy responds with status=502 Bad Gateway', async () => {
const upstreamPrincipal = await ed25519.generate()
const stubbedUpstreamResponse = {
status: 532,
statusText: 'Bad Gateway Test',
}
// create upstream
const upstreamHttpServer = nodeHttp.createServer((request, response) => {
response.writeHead(
stubbedUpstreamResponse.status,
stubbedUpstreamResponse.statusText
)
response.end()
})
after(() => upstreamHttpServer.close())
const upstreamUrl = await listen(upstreamHttpServer)
// create the proxy that will proxy requests to the upstream
const proxy = Server.create({
id: upstreamPrincipal,
decoder: CAR,
encoder: CBOR,
service: {
test: {
succeed: createProxyHandler({
connections: {
default: Client.connect({
id: upstreamPrincipal,
encoder: CAR,
decoder: CBOR,
channel: HTTP.open({
url: upstreamUrl,
}),
}),
},
}),
},
},
})
const proxyHttpServer = nodeHttp.createServer(
ucantoServerNodeListener(proxy)
)
after(() => proxyHttpServer.close())
const proxyUrl = await listen(proxyHttpServer)
const proxyConnection = Client.connect({
id: upstreamPrincipal,
encoder: CAR,
decoder: CBOR,
channel: HTTP.open({ url: proxyUrl }),
})

// invoke the proxy
const invoker = await ed25519.Signer.generate()
const [result] = await proxyConnection.execute(
Client.invoke({
issuer: invoker,
audience: upstreamPrincipal,
capability: {
can: 'test/succeed',
with: /** @type {const} */ ('did:web:dag.house'),
},
})
)

assert.equal(result?.error, true, 'result has error=true')
assert.equal(
'status' in result && result?.status,
502,
'result has status=502'
)
assert.equal(
'statusText' in result && result?.statusText,
'Bad Gateway',
'result has statusText'
)
assert.ok('x-proxy-error' in result, 'result has x-proxy-error')
assert.ok(
result['x-proxy-error'] && typeof result['x-proxy-error'] === 'object',
'result has x-proxy-error object'
)
assert.ok(
'status' in result['x-proxy-error'],
'result has x-proxy-error.status'
)
assert.equal(
result['x-proxy-error'].status,
stubbedUpstreamResponse.status,
`result['x-proxy-error'] has status=${stubbedUpstreamResponse.status}`
)
assert.ok(
'statusText' in result['x-proxy-error'],
'result has x-proxy-error.statusText'
)
assert.equal(
result['x-proxy-error'].statusText,
stubbedUpstreamResponse.statusText,
`result['x-proxy-error'] has statusText=${stubbedUpstreamResponse.statusText}`
)
assert.ok('url' in result['x-proxy-error'], 'result has x-proxy-error.url')
assert.equal(
result['x-proxy-error'].url,
upstreamUrl,
`result['x-proxy-error'] has url=${upstreamUrl}`
)
})
})