Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
26 changes: 26 additions & 0 deletions src/plugin.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1704,6 +1704,32 @@ describe('OIDC plugin (mock OIDC provider)', function () {
expect(entry.error).to.include(selfSignedReason);
});

it('logs helpful error messages for OIDC token parse failures', async function () {
getTokenPayload = () => ({
expires_in: tokenPayload.expires_in,
payload: { ...tokenPayload.payload, nonce: undefined },
});
const plugin = createMongoDBOIDCPlugin({
openBrowserTimeout: 60_000,
openBrowser: fetchBrowser,
allowedFlows: ['auth-code'],
redirectURI: 'http://localhost:0/callback',
});

try {
await requestToken(plugin, {
issuer: provider.issuer,
clientId: 'mockclientid',
requestScopes: [],
});
expect.fail('missed exception');
} catch (err: any) {
expect(err.message).to.equal(
'invalid response encountered (caused by: JWT "nonce" (nonce) claim missing)'
);
Comment on lines +1727 to +1729
Copy link

Choose a reason for hiding this comment

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

Should this be checking for "cause" to be defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nbbeeken Yeah, that's a good idea 👍 Added it in 337a4b2

}
});

it('handles JSON failure responses from the IDP', async function () {
overrideRequestHandler = (url, req, res) => {
if (new URL(url).pathname.endsWith('/token')) {
Expand Down
18 changes: 14 additions & 4 deletions src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,20 +254,30 @@ function getCause(err: unknown): Record<string, unknown> | undefined {
}
}

function wrapGenericError(err: unknown): MongoDBOIDCError {
if (MongoDBOIDCError.isMongoDBOIDCError(err)) return err;
return new MongoDBOIDCError(errorString(err), {
codeName: 'GenericError',
cause: err,
});
}

// [email protected] has reduced error messages for HTTP errors significantly, reducing e.g.
// an HTTP error to just a simple 'unexpect HTTP response status code' message, without
// further diagnostic information. So if the `cause` of an `err` object is a fetch `Response`
// object, we try to throw a more helpful error.
export async function improveHTTPResponseBasedError<T>(
err: T
): Promise<T | MongoDBOIDCError> {
): Promise<MongoDBOIDCError> {
// Note: `err.cause` can either be an `Error` object itself, or a `Response`, or a JSON HTTP response body
const cause = getCause(err);
if (cause) {
try {
const statusObject =
'status' in cause ? cause : (err as Record<string, unknown>);
if (!statusObject.status) return err;
if (!statusObject.status) {
return wrapGenericError(err);
}

let body = '';
try {
Expand Down Expand Up @@ -306,10 +316,10 @@ export async function improveHTTPResponseBasedError<T>(
{ codeName: 'HTTPResponseError', cause: err }
);
} catch {
return err;
return wrapGenericError(err);
}
}
return err;
return wrapGenericError(err);
}

// Check whether converting a Node.js `Readable` stream to a web `ReadableStream`
Expand Down
Loading