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

Improve error handling. #172

Merged
merged 4 commits into from
Aug 27, 2024
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
65 changes: 65 additions & 0 deletions lib/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -104,10 +110,69 @@ export async function addRoutes({app, service} = {}) {
res.status(201).json(body);
} catch(error) {
logger.error(error.message, {error});
// wrap if not already a BedrockError
if(!(error instanceof BedrockError)) {
_throwWrappedError({cause: error});
}
throw error;
}

// meter operation usage
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 link
Member

Choose a reason for hiding this comment

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

This implementation needs to be updated (it was taken from a source where some bugs were discovered):

https://github.com/digitalbazaar/bedrock-vc-delivery/blob/main/lib/helpers.js#L151-L170

Once that's updated, I think this is ready to go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aligned more with that code. jsonld.js doc loader is using .details.cause for not found errors, which also need stripping. Not sure if that's an issue in the other library as well.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably fine.

// 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];
}
}
// strip other potential stack data
if(_error.errors) {
_error.errors = _error.errors.map(_stripStackTrace);
}
if(Array.isArray(_error.details?.errors)) {
_error.details.errors = _error.details.errors.map(_stripStackTrace);
}
if(_error.cause) {
_error.cause = _stripStackTrace(_error.cause);
}
if(_error.details?.cause) {
_error.details.cause = _stripStackTrace(_error.details.cause);
}
return _error;
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
214 changes: 214 additions & 0 deletions test/mocha/70-misc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
/*!
* Copyright (c) 2020-2024 Digital Bazaar, Inc. All rights reserved.
*/
import * as bedrock from '@bedrock/core';
import * as helpers from './helpers.js';
import {klona} from 'klona';
import {mockData} from './mock.data.js';
import {v4 as uuid} from 'uuid';

const badCredentials = [
{
title: 'empty credential',
credential: {},
expect: {
statusCode: 400,
name: 'ValidationError',
cause: {
// FIXME non-BedrockError causes don't pass through
// could add details.errors[] checks in this case
//name: 'jsonld.ValidationError'
}
}
},
{
title: 'unkonwn context',
credential: {
'@context': [
'https://www.w3.org/ns/credentials/v2',
'bogus.example'
],
type: ['VerifiableCredential'],
credentialSubject: {
'ex:thing': true
}
},
expect: {
statusCode: 400,
name: 'DataError',
detailsError: {
name: 'jsonld.InvalidUrl'
}
}
},
{
title: 'empty subject',
credential: {
'@context': ['https://www.w3.org/ns/credentials/v2'],
type: ['VerifiableCredential'],
credentialSubject: {}
},
expect: {
// FIXME Improve @digitalbazaar/vc error handling.
// This test is a plain 'Error' with a message of
// '"credentialSubject" must make a claim.'.
statusCode: 400
}
},
{
title: 'undefined terms',
credential: {
'@context': ['https://www.w3.org/ns/credentials/v2'],
type: ['VerifiableCredential', 'UndefinedType'],
credentialSubject: {
undefinedTerm: 'notDefinedInContext'
}
},
expect: {
statusCode: 400,
name: 'DataError',
message: 'Invalid credential.',
detailsError: {
name: 'jsonld.ValidationError'
}
}
}
];

describe('fail for bad 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)}`;
});
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;
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) {
error.data.name.should.equal(testCred.expect.name);
}
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);
}
});
}
});