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

fix(wildcards): remove old secret references #67

Merged

Conversation

roggervalf
Copy link
Contributor

@roggervalf roggervalf commented Mar 7, 2024

fixes #63
we can remove old wildcard references from secret object by restoring original object to prevent memory leaks, I used same example as in the issue to see that there is not showing an array incrementing its size in each call

@roggervalf
Copy link
Contributor Author

pls @mcollina when you get some time

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Thanks for opening a PR! Can you please add a unit test?

${resultTmpl(serialize)}
`).bind(state)

redact.secret = secret
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is only for test case to compare the size of it

@@ -11,12 +11,21 @@ function redactor ({ secret, serialize, wcLen, strict, isCensorFct, censorFctTak
${strictImpl(strict, serialize)}
}
const { censor, secret } = this
const originalSecret = {}
const secretKeys = Object.keys(secret)
for (var i = 0; i < secretKeys.length; i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for node 6 we can not use spread operator directly so I have to use a for

@roggervalf
Copy link
Contributor Author

@mcollina test was added, pls when you get some time

@roggervalf roggervalf requested a review from mcollina March 7, 2024 14:17
Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 59b84b2 into davidmarkclements:main Mar 7, 2024
7 checks passed
@roggervalf roggervalf deleted the fix-remove-old-secret-references branch March 8, 2024 00:48
renovate bot referenced this pull request in valora-inc/logging Mar 10, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [fast-redact](https://togithub.com/davidmarkclements/fast-redact) |
[`^3.3.0` ->
`^3.4.0`](https://renovatebot.com/diffs/npm/fast-redact/3.3.0/3.4.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/fast-redact/3.4.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fast-redact/3.4.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fast-redact/3.3.0/3.4.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fast-redact/3.3.0/3.4.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>davidmarkclements/fast-redact (fast-redact)</summary>

###
[`v3.4.0`](https://togithub.com/davidmarkclements/fast-redact/releases/tag/v3.4.0)

[Compare
Source](https://togithub.com/davidmarkclements/fast-redact/compare/v3.3.0...v3.4.0)

#### What's Changed

- fix(wildcards): remove old secret references by
[@&#8203;roggervalf](https://togithub.com/roggervalf) in
[https://github.com/davidmarkclements/fast-redact/pull/67](https://togithub.com/davidmarkclements/fast-redact/pull/67)

#### New Contributors

- [@&#8203;roggervalf](https://togithub.com/roggervalf) made their first
contribution in
[https://github.com/davidmarkclements/fast-redact/pull/67](https://togithub.com/davidmarkclements/fast-redact/pull/67)

**Full Changelog**:
davidmarkclements/fast-redact@v3.3.0...v3.4.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - "after 8:00 before 23:00 every weekday except on Friday" in
timezone UTC.

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/valora-inc/logging).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4yMzAuMCIsInVwZGF0ZWRJblZlciI6IjM3LjIzMC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@msala-fp
Copy link

@mcollina These changes are breaking. If you will use more then once the same redact instance, it fails to restore the second object. It is breaking "pino" logger at our side. I can't be sure the source of the problem is the same, but if yes, so it could have bigger impact for lot of users.
Just try this test:

test('restores multi nested wildcard values, twice', ({ end, is }) => {
  const o = {
    a: {
      b1: {
        c1: {
          d1: { e: '123' },
          d2: { e: '456' }
        },
        c2: {
          d1: { e: '789' },
          d2: { e: '012' }
        }
      },
      b2: {
        c1: {
          d1: { e: '345' },
          d2: { e: '678' }
        },
        c2: {
          d1: { e: '901' },
          d2: { e: '234' }
        }
      }
    }
  }
  const o2 = {
    a: {
      b1: {
        c1: {
          d1: { e: '123' },
          d2: { e: '456' }
        },
        c2: {
          d1: { e: '789' },
          d2: { e: '012' }
        }
      },
      b2: {
        c1: {
          d1: { e: '345' },
          d2: { e: '678' }
        },
        c2: {
          d1: { e: '901' },
          d2: { e: '234' }
        }
      }
    }
  }

  const censor = 'censor'
  const paths = ['a.*.*.*.e']
  const redact = fastRedact({ paths, censor, serialize: false })

  redact(o)
  is(o.a.b1.c1.d1.e, censor)
  is(o.a.b1.c1.d2.e, censor)
  is(o.a.b1.c2.d1.e, censor)
  is(o.a.b1.c2.d2.e, censor)
  is(o.a.b2.c1.d1.e, censor)
  is(o.a.b2.c1.d2.e, censor)
  is(o.a.b2.c2.d1.e, censor)
  is(o.a.b2.c2.d2.e, censor)
  redact.restore(o)
  is(o.a.b1.c1.d1.e, '123')
  is(o.a.b1.c1.d2.e, '456')
  is(o.a.b1.c2.d1.e, '789')
  is(o.a.b1.c2.d2.e, '012')
  is(o.a.b2.c1.d1.e, '345')
  is(o.a.b2.c1.d2.e, '678')
  is(o.a.b2.c2.d1.e, '901')
  is(o.a.b2.c2.d2.e, '234')

  redact(o2)
  is(o2.a.b1.c1.d1.e, censor)
  is(o2.a.b1.c1.d2.e, censor)
  is(o2.a.b1.c2.d1.e, censor)
  is(o2.a.b1.c2.d2.e, censor)
  is(o2.a.b2.c1.d1.e, censor)
  is(o2.a.b2.c1.d2.e, censor)
  is(o2.a.b2.c2.d1.e, censor)
  is(o2.a.b2.c2.d2.e, censor)
  redact.restore(o2)
  is(o2.a.b1.c1.d1.e, '123')
  is(o2.a.b1.c1.d2.e, '456')
  is(o2.a.b1.c2.d1.e, '789')
  is(o2.a.b1.c2.d2.e, '012')
  is(o2.a.b2.c1.d1.e, '345')
  is(o2.a.b2.c1.d2.e, '678')
  is(o2.a.b2.c2.d1.e, '901')
  is(o2.a.b2.c2.d2.e, '234')

  end()
})

@tadhglewis
Copy link

tadhglewis commented Mar 13, 2024

We also ran into this issue which can cause pretty major application and data issues depending on your usage of pino/fast-redact 😬

bump @mcollina

@72636c
Copy link
Contributor

72636c commented Mar 13, 2024

Here's a minimal-ish repro for the pino logging use case:

import { pino } from 'pino';

const logger = pino({ redact: { censor: '???', paths: ['props.*'] } });

const props = { name: 'PII' };

logger.child({ props });
logger.child({ props });

console.log({ props });
// { props: { name: '???' } }

@MSala
Copy link
Contributor

MSala commented Mar 13, 2024

I have prepared a PR with potential fixes, retaining the benefits of changes from this PR.
See: #69
CC: @mcollina @roggervalf

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential memory leak when using wildcard patterns
6 participants