feat(filters): filter set-cookie headers #485
Conversation
|
Note: The Node 8+9 tav tests are currently failing because of a bug in npm triggered by the use of a github dependency link. |
| @@ -51,6 +58,17 @@ function httpHeaders (payload) { | |||
| .map(function (k) { return k + '=' + cookies[k] }) | |||
| .join('; ') | |||
| } | |||
|
|
|||
| if (typeof headers['set-cookie'] !== 'undefined') { | |||
There was a problem hiding this comment.
I don't think we need to protect our selfs from people overwriting the undefined property right? If so you can just do a simple comparison instead. Unless there's an edge case I'm not considering?
There was a problem hiding this comment.
You mean do if (headers['set-cookie'] !== undefined) instead?
| headers['set-cookie'] = stringify(setCookies) | ||
| } catch (err) { | ||
| // Ignore error | ||
| headers['set-cookie'] = '[malformed set-cookie header]' |
There was a problem hiding this comment.
Are we sure we want to overwrite it in this case? I guess it's to prevent leaking secrets, but it does make it hard to fix it if the user never sees how it looked 🤔
There was a problem hiding this comment.
I'm not really sure. I think it makes sense, from a security perspective, to redact the entire header if the parser doesn't understand it. But I don't feel strongly about it. This just seemed like the most obvious approach at the time.
Maybe others have some opinions on this?
There was a problem hiding this comment.
@elastic/apm-agent-devs @roncohen: Should we redact the entire Set-Cookie header if we can't parse it or just report it as is?
There was a problem hiding this comment.
In the Java agent, we always redact the whole value. I’m not sure if the overhead of parsing the value is worth it.
Qard
force-pushed
the
set-cookie
branch
3 times, most recently
from
5a3503e
to
1f5a1bd
Compare
package.json
Outdated
| @@ -84,6 +84,7 @@ | |||
| "require-ancestors": "^1.0.0", | |||
| "require-in-the-middle": "^3.1.0", | |||
| "semver": "^5.5.0", | |||
| "set-cookie-serde": "github:qard/set-cookie-serde#implementation", | |||
There was a problem hiding this comment.
This PR is marked as "In Review", but I guess it should still be "In Progress" until this dependency have been published right?
There was a problem hiding this comment.
I just marked as "In Review" to indicate it's as done as it will get until the PR for set-cookie-serde is merged and released. This is blocked until that gets released.
Fixes #468
Depends on elastic/set-cookie-serde/pull/1
Checklist