bugsnag-js icon indicating copy to clipboard operation
bugsnag-js copied to clipboard

Security issue: Bugsnag koa logs request cookies & request body

Open villesau opened this issue 2 years ago • 4 comments

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

  • Bugsnag version: 7.14.1, plugin: 7.14.0
  • Server framework version (if any):
    • Koa: 2.13.1

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.

villesau avatar Dec 31 '21 10:12 villesau

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

xljones avatar Jan 03 '22 18:01 xljones

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

villesau avatar Jan 03 '22 21:01 villesau

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]"
}

xljones avatar Jan 03 '22 22:01 xljones

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.

villesau avatar Jan 04 '22 08:01 villesau