From 3ede5ec41841a1b33b9f568a6cf3a27c35146603 Mon Sep 17 00:00:00 2001 From: "David I. Lehn" Date: Mon, 12 Aug 2024 19:34:36 -0400 Subject: [PATCH 1/4] Improve error handling. - Return 400 instead of 500 for some common errors. --- CHANGELOG.md | 6 ++ lib/http.js | 15 ++++ test/mocha/70-misc.js | 203 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 224 insertions(+) create mode 100644 test/mocha/70-misc.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 6375c026..18c3fb41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # bedrock-vc-issuer ChangeLog +## 27.0.3 - 2024-xx-xx + +### Changed +- Improve error handling. + - Return 400 instead of 500 for some common errors. + ## 27.0.2 - 2024-08-26 ### Fixed diff --git a/lib/http.js b/lib/http.js index bbd39305..ede9078f 100644 --- a/lib/http.js +++ b/lib/http.js @@ -104,6 +104,21 @@ export async function addRoutes({app, service} = {}) { res.status(201).json(body); } catch(error) { logger.error(error.message, {error}); + // will be thrown for safe mode violations + if(error.name === 'jsonld.ValidationError') { + throw new BedrockError('Invalid credential.', { + name: 'SyntaxError', + details: { + httpStatusCode: 400, + public: true + } + }); + } + // TODO: handle other possible errors + // in particular @digitalbazaar/vc throws many simple 'TypeError' and + // 'Error' errors. Difficult to distinguish them from other errors. + + // default to other layers, likely will be an ISE/500 throw error; } diff --git a/test/mocha/70-misc.js b/test/mocha/70-misc.js new file mode 100644 index 00000000..60598627 --- /dev/null +++ b/test/mocha/70-misc.js @@ -0,0 +1,203 @@ +/*! + * Copyright (c) 2020-2024 Digital Bazaar, Inc. All rights reserved. + */ +import * as bedrock from '@bedrock/core'; +import * as helpers from './helpers.js'; +//import {createRequire} from 'node:module'; +import {klona} from 'klona'; +import {mockData} from './mock.data.js'; +import {v4 as uuid} from 'uuid'; + +//const require = createRequire(import.meta.url); + +// NOTE: using embedded context in mockCredential: +// https://www.w3.org/2018/credentials/examples/v1 +//const mockCredential = require('./mock-credential.json'); +//const mockCredentialV2 = require('./mock-credential-v2.json'); + +const badCredentials = [ + { + title: 'empty credential', + credential: {}, + expect: { + statusCode: 400, + name: 'ValidationError', + message: null + } + }, + { + title: 'unkonwn context', + credential: { + '@context': [ + 'https://www.w3.org/ns/credentials/v2', + 'bogus.example' + ], + type: ['VerifiableCredential'], + credentialSubject: { + 'ex:thing': true + } + }, + expect: { + // FIXME + statusCode: 400, + name: 'jsonld.InvalidUrl', + message: null + } + }, + { + title: 'empty subject', + credential: { + '@context': ['https://www.w3.org/ns/credentials/v2'], + type: ['VerifiableCredential'], + credentialSubject: {} + }, + expect: { + // FIXME + statusCode: 500, + name: null, + message: null + } + }, + { + title: 'undefined terms', + credential: { + '@context': ['https://www.w3.org/ns/credentials/v2'], + type: ['VerifiableCredential', 'UndefinedType'], + credentialSubject: { + undefinedTerm: 'notDefinedInContext' + } + }, + expect: { + statusCode: 400, + name: 'SyntaxError', + message: 'Invalid credential.' + } + } +]; + +describe.only('fail for bed credentials', () => { + let suites; + let capabilityAgent; + let zcaps; + let noStatusListIssuerId; + let noStatusListIssuerRootZcap; + beforeEach(async () => { + // generate a proof set using all of these suites in each test + suites = [{ + name: 'Ed25519Signature2020', + algorithm: 'Ed25519' + }, { + name: 'eddsa-rdfc-2022', + algorithm: 'Ed25519' + }, { + name: 'ecdsa-rdfc-2019', + algorithm: 'P-256' + }, { + name: 'ecdsa-sd-2023', + algorithm: 'P-256', + // require these options (do not allow client to override) + options: { + mandatoryPointers: ['/issuer'] + } + }, { + name: 'ecdsa-xi-2023', + algorithm: 'P-256' + }, { + name: 'bbs-2023', + algorithm: 'Bls12381G2', + // require these options (do not allow client to override) + options: { + mandatoryPointers: ['/issuer'] + } + }]; + + // generate a `did:web` DID for the issuer + const {host} = bedrock.config.server; + const localId = uuid(); + const did = `did:web:${encodeURIComponent(host)}:did-web:${localId}`; + + // provision dependencies + ({capabilityAgent, zcaps} = await helpers.provisionDependencies({ + did, cryptosuites: suites, status: false, zcaps: true + })); + + // create issue options + const issueOptions = { + issuer: did, + cryptosuites: suites.map(suite => { + const {name, options, zcapReferenceIds} = suite; + const cryptosuite = {name, zcapReferenceIds}; + if(options) { + cryptosuite.options = options; + } + return cryptosuite; + }) + }; + + // create `did:web` DID document for issuer + const didDocument = { + '@context': [ + 'https://www.w3.org/ns/did/v1', + 'https://w3id.org/security/suites/ed25519-2020/v1', + 'https://w3id.org/security/multikey/v1' + ], + id: did, + verificationMethod: [], + assertionMethod: [] + }; + for(const {assertionMethodKey} of suites) { + const description = await assertionMethodKey.getKeyDescription(); + delete description['@context']; + didDocument.verificationMethod.push(description); + didDocument.assertionMethod.push(description.id); + } + // add DID doc to map with DID docs to be served + mockData.didWebDocuments.set(localId, didDocument); + + // create issuer instance w/ no status list options + const noStatusListIssuerConfig = await helpers.createIssuerConfig({ + capabilityAgent, zcaps, issueOptions + }); + noStatusListIssuerId = noStatusListIssuerConfig.id; + noStatusListIssuerRootZcap = + `urn:zcap:root:${encodeURIComponent(noStatusListIssuerId)}`; + }); + // filter using 'only' and 'skip' + const _hasOnly = badCredentials.some(c => c.skip !== true && c.only === true); + const _badCredentials = badCredentials + .filter(c => c.skip !== true) + .filter(c => !_hasOnly || c.only === true); + for(const testCred of _badCredentials) { + it(`fails for ${testCred.title}`, async () => { + const credential = klona(testCred.credential); + let error; + let result; + try { + const zcapClient = helpers.createZcapClient({capabilityAgent}); + result = await zcapClient.write({ + url: `${noStatusListIssuerId}/credentials/issue`, + capability: noStatusListIssuerRootZcap, + json: { + credential, + options: { + extraInformation: 'abc' + } + } + }); + } catch(e) { + error = e; + } + should.exist(error); + should.not.exist(result); + if(testCred.expect.statusCode) { + error.status.should.equal(testCred.expect.statusCode); + } + if(testCred.expect.name !== null) { + error.data.name.should.equal(testCred.expect.name); + } + if(testCred.expect.message !== null) { + error.data.message.should.equal(testCred.expect.message); + } + }); + } +}); From 62f15a80cf6e617e66c39f3c6f57a4c22fea3b3c Mon Sep 17 00:00:00 2001 From: "David I. Lehn" Date: Thu, 15 Aug 2024 14:52:09 -0400 Subject: [PATCH 2/4] Add error cause. Co-authored-by: Dave Longley --- lib/http.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/http.js b/lib/http.js index ede9078f..55db2228 100644 --- a/lib/http.js +++ b/lib/http.js @@ -111,7 +111,8 @@ export async function addRoutes({app, service} = {}) { details: { httpStatusCode: 400, public: true - } + }, + cause: error }); } // TODO: handle other possible errors From a667db89daf307c256576b0166641e95e80dc35f Mon Sep 17 00:00:00 2001 From: "David I. Lehn" Date: Mon, 26 Aug 2024 19:03:36 -0400 Subject: [PATCH 3/4] Update error handling. - Don't wrap BedrockErrors. - Special case error names that shouold be 'DataError'. - Add more complex stack trace stripping. - Update tests. --- lib/http.js | 72 ++++++++++++++++++++++++++++++++++--------- test/mocha/70-misc.js | 67 +++++++++++++++++++++++----------------- 2 files changed, 96 insertions(+), 43 deletions(-) diff --git a/lib/http.js b/lib/http.js index 55db2228..7143224e 100644 --- a/lib/http.js +++ b/lib/http.js @@ -104,22 +104,10 @@ export async function addRoutes({app, service} = {}) { res.status(201).json(body); } catch(error) { logger.error(error.message, {error}); - // will be thrown for safe mode violations - if(error.name === 'jsonld.ValidationError') { - throw new BedrockError('Invalid credential.', { - name: 'SyntaxError', - details: { - httpStatusCode: 400, - public: true - }, - cause: error - }); + // wrap if not already a BedrockError + if(!(error instanceof BedrockError)) { + _throwWrappedError({cause: error}); } - // TODO: handle other possible errors - // in particular @digitalbazaar/vc throws many simple 'TypeError' and - // 'Error' errors. Difficult to distinguish them from other errors. - - // default to other layers, likely will be an ISE/500 throw error; } @@ -127,3 +115,57 @@ export async function addRoutes({app, service} = {}) { metering.reportOperationUsage({req}); })); } + +// known error cause names to map to 'DataError' +const _dataErrors = new Set([ + 'DataError', + 'jsonld.InvalidUrl', + 'jsonld.ValidationError' +]); +// TODO: handle other possible errors +// in particular @digitalbazaar/vc throws many simple 'TypeError' and +// 'Error' errors. Difficult to distinguish them from other errors. + +function _throwWrappedError({cause}) { + const {name/*, message*/} = cause; + + // TODO: should this be verbose with the top level error? it's also in the + // 'error' field. + //const error = new BedrockError(message ?? 'Invalid credential.', { + const error = new BedrockError('Invalid credential.', { + name: _dataErrors.has(name) ? 'DataError' : 'OperationError', + details: { + // using sanitized 'error' field instead of 'cause' due to bedrock + // currently filtering out non-BedrockError causes. + error: _stripStackTrace(cause), + httpStatusCode: cause.httpStatusCode ?? 400, + public: true + } + }); + + throw error; +} + +function _stripStackTrace(error) { + // copy error data + const stripped = {...error}; + if(error.name) { + stripped.name = error.name; + } + if(error.message) { + stripped.message = error.message; + } + // remove stack + delete stripped.stack; + // strip other potential stack data + if(stripped.errors) { + stripped.errors = stripped.errors.map(_stripStackTrace); + } + if(stripped.cause) { + stripped.cause = _stripStackTrace(stripped.cause); + } + if(stripped.details?.cause) { + stripped.details.cause = _stripStackTrace(stripped.details.cause); + } + return stripped; +} diff --git a/test/mocha/70-misc.js b/test/mocha/70-misc.js index 60598627..ce1be23a 100644 --- a/test/mocha/70-misc.js +++ b/test/mocha/70-misc.js @@ -3,18 +3,10 @@ */ import * as bedrock from '@bedrock/core'; import * as helpers from './helpers.js'; -//import {createRequire} from 'node:module'; import {klona} from 'klona'; import {mockData} from './mock.data.js'; import {v4 as uuid} from 'uuid'; -//const require = createRequire(import.meta.url); - -// NOTE: using embedded context in mockCredential: -// https://www.w3.org/2018/credentials/examples/v1 -//const mockCredential = require('./mock-credential.json'); -//const mockCredentialV2 = require('./mock-credential-v2.json'); - const badCredentials = [ { title: 'empty credential', @@ -22,7 +14,11 @@ const badCredentials = [ expect: { statusCode: 400, name: 'ValidationError', - message: null + cause: { + // FIXME non-BedrockError causes don't pass through + // could add details.errors[] checks in this case + //name: 'jsonld.ValidationError' + } } }, { @@ -38,10 +34,11 @@ const badCredentials = [ } }, expect: { - // FIXME statusCode: 400, - name: 'jsonld.InvalidUrl', - message: null + name: 'DataError', + detailsError: { + name: 'jsonld.InvalidUrl' + } } }, { @@ -52,10 +49,10 @@ const badCredentials = [ credentialSubject: {} }, expect: { - // FIXME - statusCode: 500, - name: null, - message: null + // FIXME Improve @digitalbazaar/vc error handling. + // This test is a plain 'Error' with a message of + // '"credentialSubject" must make a claim.'. + statusCode: 400 } }, { @@ -69,13 +66,16 @@ const badCredentials = [ }, expect: { statusCode: 400, - name: 'SyntaxError', - message: 'Invalid credential.' + name: 'DataError', + message: 'Invalid credential.', + detailsError: { + name: 'jsonld.ValidationError' + } } } ]; -describe.only('fail for bed credentials', () => { +describe('fail for bad credentials', () => { let suites; let capabilityAgent; let zcaps; @@ -162,13 +162,17 @@ describe.only('fail for bed credentials', () => { noStatusListIssuerRootZcap = `urn:zcap:root:${encodeURIComponent(noStatusListIssuerId)}`; }); - // filter using 'only' and 'skip' - const _hasOnly = badCredentials.some(c => c.skip !== true && c.only === true); - const _badCredentials = badCredentials - .filter(c => c.skip !== true) - .filter(c => !_hasOnly || c.only === true); - for(const testCred of _badCredentials) { - it(`fails for ${testCred.title}`, async () => { + for(const testCred of badCredentials) { + // handle 'skip' and 'only' flags. + let _it; + if(testCred.skip) { + _it = it.skip; + } else if(testCred.only) { + _it = it.only; + } else { + _it = it; + } + _it(`fails for ${testCred.title}`, async () => { const credential = klona(testCred.credential); let error; let result; @@ -192,12 +196,19 @@ describe.only('fail for bed credentials', () => { if(testCred.expect.statusCode) { error.status.should.equal(testCred.expect.statusCode); } - if(testCred.expect.name !== null) { + if(testCred.expect.name) { error.data.name.should.equal(testCred.expect.name); } - if(testCred.expect.message !== null) { + if(testCred.expect.message) { error.data.message.should.equal(testCred.expect.message); } + if(testCred.expect?.cause?.name) { + error.data.cause.name.should.equal(testCred.expect.cause.name); + } + if(testCred.expect?.detailsError?.name) { + error.data.details.error.name.should.equal( + testCred.expect.detailsError.name); + } }); } }); From 356d5fb10b00a01d3e5df4cc88576d30f6c5174c Mon Sep 17 00:00:00 2001 From: "David I. Lehn" Date: Tue, 27 Aug 2024 14:11:05 -0400 Subject: [PATCH 4/4] Update error handling. - Use `serialize-error` and allow list of error keys when create error. - More alignment with bedrock-vc-delivery error code. --- lib/http.js | 39 +++++++++++++++++++++++---------------- package.json | 1 + 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/lib/http.js b/lib/http.js index 7143224e..dfa0273c 100644 --- a/lib/http.js +++ b/lib/http.js @@ -10,10 +10,16 @@ import {getDocumentStore} from './helpers.js'; import {issue} from './issuer.js'; import {issueCredentialBody} from '../schemas/bedrock-vc-issuer.js'; import {logger} from './logger.js'; +import {serializeError} from 'serialize-error'; import {createValidateMiddleware as validate} from '@bedrock/validation'; const {util: {BedrockError}} = bedrock; +const ALLOWED_ERROR_KEYS = [ + 'message', 'name', 'type', 'data', 'errors', 'error', 'details', 'cause', + 'status' +]; + // FIXME: remove and apply at top-level application bedrock.events.on('bedrock-express.configure.bodyParser', app => { app.use(bodyParser.json({ @@ -147,25 +153,26 @@ function _throwWrappedError({cause}) { } function _stripStackTrace(error) { - // copy error data - const stripped = {...error}; - if(error.name) { - stripped.name = error.name; - } - if(error.message) { - stripped.message = error.message; + // serialize error and allow-list specific properties + const serialized = serializeError(error); + const _error = {}; + for(const key of ALLOWED_ERROR_KEYS) { + if(serialized[key] !== undefined) { + _error[key] = serialized[key]; + } } - // remove stack - delete stripped.stack; // strip other potential stack data - if(stripped.errors) { - stripped.errors = stripped.errors.map(_stripStackTrace); + if(_error.errors) { + _error.errors = _error.errors.map(_stripStackTrace); + } + if(Array.isArray(_error.details?.errors)) { + _error.details.errors = _error.details.errors.map(_stripStackTrace); } - if(stripped.cause) { - stripped.cause = _stripStackTrace(stripped.cause); + if(_error.cause) { + _error.cause = _stripStackTrace(_error.cause); } - if(stripped.details?.cause) { - stripped.details.cause = _stripStackTrace(stripped.details.cause); + if(_error.details?.cause) { + _error.details.cause = _stripStackTrace(_error.details.cause); } - return stripped; + return _error; } diff --git a/package.json b/package.json index c5adef41..317d2e6c 100644 --- a/package.json +++ b/package.json @@ -52,6 +52,7 @@ "cors": "^2.8.5", "klona": "^2.0.6", "lru-cache": "^6.0.0", + "serialize-error": "^11.0.3", "uuid": "^10.0.0" }, "peerDependencies": {