bugsnag / bugsnag-js

JavaScript error handling tool for BugSnag. Monitor and report JavaScript bugs & errors.
https://docs.bugsnag.com/platforms/javascript
MIT License
852 stars 251 forks source link

Security issue: Bugsnag koa logs request cookies & request body #1630

Open villesau opened 2 years ago

villesau commented 2 years ago

Describe the bug

When error is reported, bugsnag Koa plugin reports also request cookies: https://github.com/bugsnag/bugsnag-js/blob/18223eb59dba07a4961d1eb50d49cc7bc90fa3b5/packages/plugin-koa/src/koa.js#L99

And

https://github.com/bugsnag/bugsnag-js/blob/18223eb59dba07a4961d1eb50d49cc7bc90fa3b5/packages/plugin-koa/src/request-info.js#L12

As you see, there's no filter what so ever when cookies are passed forward.

Steps to reproduce

  1. Setup Koa project with standard bugsnag setup
  2. Fire an error
  3. In the logged error you also see cookies

Environment

The issue likely exists in other plugins too.

E: I also think that the request body is logged as well. With post requests that might contain users passwords and other credentials too. Very dangerous.

xljones commented 2 years ago

Hey @villesau, the request headers, and body are captured where available. This is by design.

You can see exactly what data is captured automatically here, and how to remove them from Bugsnag reports: https://docs.bugsnag.com/platforms/javascript/koa/automatically-captured-data/#request-information

password is a redacted key term by default so won't be included in reports, but you can add more terms to this array as described here: https://docs.bugsnag.com/platforms/javascript/koa/configuration-options/#redactedkeys

villesau commented 2 years ago

@xander-jones You have quite dangerous defaults then. How on earth you think that logging cookies is a sane default?

xljones commented 2 years ago

For many developers, logging cookies may be a helpful link in debugging. Sensitive data shouldn't be stored in cookies strictly speaking, but in cases where it is required, yes I agree it ought to be redacted. I'd suggest this is an exception to the norm.

I don't think this is a security issue per se as this data is redactable, and all automatically captured data is documented. But I hear your concerns with regards defaults – I'll raise this with the team 👍

For future travellers; you can remove cookies from ever being sent in Koa, (or equivalently for any JS framework with Bugsnag in use where a request is captured) with redactedKeys as follows:

Bugsnag.start({
  apiKey: YOUR_BUGSNAG_API_KEY,
  plugins: [BugsnagPluginKoa],
  redactedKeys: [
    'cookie',       // exact match: "cookie"
    'access_token', // exact match: "access_token"
    /^password$/i,  // case-insensitive: "password", "PASSWORD", "PaSsWoRd"
    /^cc_/          // prefix match: "cc_number" "cc_cvv" "cc_expiry"
  ]
})

const app = new Koa()
// app setup excluded for brevity ...

app.use(
  router()
    .get('/cookies', (ctx, next) => {
        ctx.cookies.set("foo", "bar");
        ctx.bugsnag.notify(new Error("Cookies be gone!"))
    })
  // ...
)

results in the following reported to Bugsnag under the request tab:

"headers": {
  "cookie": "[REDACTED]"
}
villesau commented 2 years ago

Thank you for taking it forward. Cookie is probably the most sensitive field of all of them since in regular web apps it has the session information that gives access to all the users data.

One way to make Bugsnag more secure (with typescript at least) would be to make redactedKeys mandatory. This way you could enforce the user to acknowledge the existence of the field, even when you explicitly pass an empty array.