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

Thrown AggregateError object contains the JS callstack in its message #336

Closed
2 tasks done
mbektchiev opened this issue Feb 25, 2021 · 2 comments
Closed
2 tasks done
Labels

Comments

@mbektchiev
Copy link

Describe the bug
When Issuer.discover is called with an invalid or wrong URL, the thrown error contains the JS callstack in its message property. This is a security issue since many times the error message is being sent to the end-users of a product.

To Reproduce
Issuer and Client configuration: (inline or gist) - Don't forget to redact your secrets.

try {
    await Issuer.discover("https1://invalid_url/");
} catch (err) {
    console.log(err);
}

Steps to reproduce the behaviour:

  1. Simply execute the above snippet

Expected behaviour
Instead of logging both failed promises errors callstacks it should only show their respective messages.

Environment:

  • openid-client version: 4.4.0
  • node version: 14.15.4

Additional context
This seems to be related to the default and uncustomisable behaviour of aggregate-error which is used by p-any and p-some. I've currently devised a quite gross but functional workaround:

        try {
            const result = await Issuer.discover(issuer);

            return result;
        } catch (err) {
            // HACK: Hide stack from AggregateError.message for security reasons
            if (err.constructor?.name === "AggregateError" && err[Symbol.iterator]) {
                const errorMessages = [...err].map((e) => e.message);

                throw Object.assign(
                    new Error(`Issuer discovery failed.\n${errorMessages.join("\n")}`),
                    { stack: err.stack },
                );
            }
            throw err;
        }
  • the bug is happening on latest openid-client too.
  • I have searched the issues tracker on github for similar issues and couldn't find anything related.
@panva panva closed this as completed in 3011cca Feb 26, 2021
@panva panva added bug and removed triage labels Feb 26, 2021
@panva
Copy link
Owner

panva commented Feb 26, 2021

Not a security issue in the lib per se, it's an issue in your code if you return any code generated exceptions to users in the first place.

Nevertheless, i've filtered out the stack from AggregateError instances. Released in v v4.4.1

@mbektchiev
Copy link
Author

Thank you for the fix, @panva!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
2 participants