From 57d0fa47a5e7f51c86688ca6bdc42b93f1562ea4 Mon Sep 17 00:00:00 2001 From: Benjamin Goering <171782+gobengo@users.noreply.github.com> Date: Thu, 19 Jan 2023 11:08:52 -0800 Subject: [PATCH] feat: access-api proxy.js has configurable options.catchInvocationError, by default catches HTTPError -> error result w/ status=502 (#366) , which defaults to something that catches HTTPErrors and rewrites to a status=502 error result Motivation: * relates to: https://github.com/web3-storage/w3protocol/issues/363 * instead of that behavior of getting a `HandlerExecutionError` (due to internally having an uncaught `HTTPError`), after this PR we should * not have the uncaught `HTTPError`, so no uncaught `HandlerExecutionError` * the result will have `status=502` and `x-proxy-error` with information about the proxy invocation request that errored (which will help debug #363) Limitations * This inlcudes in `result['x-proxy-error']` any info from the underlying `@ucanto/transport/http` `HTTPError`, but unless/until we put more info on that error, we still won't have the raw response object and e.g. won't have response headers. * but if those properties are ever added to `HTTPError`, they should show up in `x-proxy-error` --- packages/access-api/src/ucanto/proxy.js | 83 +++++++++++--- .../access-api/test/helpers/upload-api.js | 33 ++++-- packages/access-api/test/ucanto-proxy.test.js | 107 ++++++++++++++++++ 3 files changed, 201 insertions(+), 22 deletions(-) diff --git a/packages/access-api/src/ucanto/proxy.js b/packages/access-api/src/ucanto/proxy.js index c62c39ac8..a95a55732 100644 --- a/packages/access-api/src/ucanto/proxy.js +++ b/packages/access-api/src/ucanto/proxy.js @@ -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) { + return badGatewayResult + } + throw error +} + /** * @template {Ucanto.ConnectionView} [Connection=Ucanto.ConnectionView] * @param {object} options + * @param {(error: unknown) => Promise} [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] */ @@ -16,8 +57,12 @@ export function createProxyHandler(options) { * @returns {Promise>} */ 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 @@ -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} */ (connection) - ) - return result + const proxyInvocation = Client.invoke({ + issuer: proxyInvocationIssuer, + capability: capabilities[0], + audience, + proofs: [invocationIn], + expiration, + notBefore, + }) + try { + const [result] = await Client.execute( + [proxyInvocation], + /** @type {Client.ConnectionView} */ (connection) + ) + return result + } catch (error) { + if (catchInvocationError) { + const caughtResult = await catchInvocationError(error) + return caughtResult + } + throw error + } } } diff --git a/packages/access-api/test/helpers/upload-api.js b/packages/access-api/test/helpers/upload-api.js index 5079d5150..d80c32ad6 100644 --- a/packages/access-api/test/helpers/upload-api.js +++ b/packages/access-api/test/helpers/upload-api.js @@ -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} */ ({ ...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} */ ({ ...request.headers }), + body: Buffer.concat(chunks), + }) + response.writeHead(200, headers) + response.write(body) + response.end() + } catch (error) { + response.writeHead(500) + response.end(error) + } } } @@ -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 + */ +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()) +} diff --git a/packages/access-api/test/ucanto-proxy.test.js b/packages/access-api/test/ucanto-proxy.test.js index 3786304d3..e6f873564 100644 --- a/packages/access-api/test/ucanto-proxy.test.js +++ b/packages/access-api/test/ucanto-proxy.test.js @@ -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 () => { @@ -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}` + ) + }) })