davidmarkclements / fast-redact

very fast object redaction
MIT License
278 stars 30 forks source link

fix(wildcards): remove old secret references #67

Closed roggervalf closed 6 months ago

roggervalf commented 6 months ago

fixes https://github.com/davidmarkclements/fast-redact/issues/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 commented 6 months ago

pls @mcollina when you get some time

roggervalf commented 6 months ago

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

msala-fp commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

I have prepared a PR with potential fixes, retaining the benefits of changes from this PR. See: https://github.com/davidmarkclements/fast-redact/pull/69 CC: @mcollina @roggervalf