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

Send cookies explicitly into context.request's transaction #3322

Merged
merged 8 commits into from May 8, 2023

Conversation

david-luna
Copy link
Member

@david-luna david-luna commented May 5, 2023

Description

This PR changes the transaction serialisation to be more explicit about sanitization of sensitive data in request's cookie header.

Current status

cookies are sanitized in the header value based on sanitizeFieldNames configuration. As a result you see clear values and redacted ones. This may lead to think the agent is not properly doing sanitization.

After this PR

cookies are going to be placed in context.request.cookies map, which is accepted by the intake API, and sanitized based on the configuration. The cookie header will be completely redacted to avoid duplication of data.

Fixes: #3273

Checklist

  • Implement code
  • Add tests
  • Update TypeScript typings
  • Update documentation
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label May 5, 2023
@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) May 5, 2023
@david-luna david-luna changed the title Dluna/3273 cookie header not redacted Send cookies explicitly into context.request's transaction May 5, 2023
@@ -21594,6 +21612,29 @@
"integrity": "sha512-OPs5WnnT1xkCBiuQrZA4+YAV4HEJejmHneyraIaxsbev5yCEr6KMwINNFP9wQeFIw8FWcoTqF3vQsa5CDaI+8Q==",
"dev": true
},
"@types/node-fetch": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Doing a client checkout of main and npm i produces also this addition so I guess its dependency not locked in a previous PR.

Copy link
Member

Choose a reason for hiding this comment

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

@david-luna david-luna marked this pull request as ready for review May 5, 2023 13:11
@david-luna david-luna requested a review from trentm May 5, 2023 13:11
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

One type on the changelog, and a question about the deprecation below. Otherwise this looks good.

CHANGELOG.asciidoc Outdated Show resolved Hide resolved
lib/parsers.js Outdated
@@ -58,6 +60,15 @@ function getContextFromRequest (req, conf, type) {
Object.assign({}, req.headers),
conf.sanitizeFieldNamesRegExp
)

// TODO: deprecate filterHttpHeaders in next major release
Copy link
Member

Choose a reason for hiding this comment

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

We might be disagreeing on the definition of "deprecate". For me, "deprecating" means that we would document (and mark in types) that the config var is "deprecated" -- hence users should avoid it and the docs should show users how to migrate away from it, but the field is still supported.

The "TODO" for the next major release would be to remove the filterHttpHeaders config var.
Thoughts?

If we agree, then would you like to do the deprecation tweaks in this PR, or prefer that we do it in a separate change?

Copy link
Member Author

@david-luna david-luna May 8, 2023

Choose a reason for hiding this comment

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

I think your approach is much better. I shall do the changes in #3320 before next major

Thanks!

david-luna and others added 2 commits May 8, 2023 11:39
Co-authored-by: Trent Mick <trent.mick@elastic.co>
@david-luna david-luna merged commit b57086b into main May 8, 2023
33 checks passed
APM-Agents (OLD) automation moved this from In Progress to Done May 8, 2023
@david-luna david-luna deleted the dluna/3273-cookie-header-not-redacted branch May 8, 2023 10:34
trentm added a commit that referenced this pull request May 16, 2023
* feat: add cookies object into context.request
---------

Co-authored-by: Trent Mick <trent.mick@elastic.co>
trentm added a commit that referenced this pull request Jun 9, 2023
Since #3322 it is no longer used.

Refs: #3322
david-luna pushed a commit that referenced this pull request Jun 12, 2023
Since #3322 it is no longer used.

Refs: #3322
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
Development

Successfully merging this pull request may close these issues.

Set-Cookie header is redacted, but Cookie header is not
2 participants