davidmarkclements / fast-redact

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

Original object is modified and not restored in any way #50

Closed meirkeller closed 1 year ago

meirkeller commented 2 years ago

I want to be able the redact a key in multiple levels. So i wrote this code:

const fastRedact = require('fast-redact');
const paths = ['*.d', '*.*.d', '*.*.*.d'];

const redact = fastRedact({
  paths,
});

const obj = {
  x: { c: { d: 'hide me', e: 'leave me be' } },
  y: { c: { d: 'and me', f: 'I want to live' } },
  z: { c: { d: 'and also I', g: 'I want to run in a stream' } },
};

console.log(redact(obj));
console.log(obj);

The output is:

{"x":{"c":{"d":"[REDACTED]","e":"leave me be"}},"y":{"c":{"d":"[REDACTED]","f":"I want to live"}},"z":{"c":{"d":"[REDACTED]","g":"I want to run in a stream"}}}
{
  x: { c: { d: '[REDACTED]', e: 'leave me be' } },
  y: { c: { d: '[REDACTED]', f: 'I want to live' } },
  z: { c: { d: '[REDACTED]', g: 'I want to run in a stream' } }
}

You can see that the original object is modified.

Seems like something is wrong with the nestedRestore and the nestedRedact functions.

When i added multiple levels that do not exist in the object. Seems like that nestedRedact with the specialSet functions doesn't build the store properly.

jsumners commented 2 years ago

https://github.com/davidmarkclements/fast-redact/blob/41339e6fa3e8b3a47c7266d3b5540ca0a8dd6764/readme.md?plain=1#L180-L188

mcollina commented 2 years ago

ouch, it seem we have a bug in the restore function.

mcollina commented 2 years ago

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

meirkeller commented 2 years ago

Will try to find a fix. Need to dig in the specialSet function.

meirkl commented 2 years ago

51 this could be a fix. Needs some more investigation.

meirkeller commented 2 years ago
function specialSet (o, k, path, afterPath, censor, isCensorFct, censorFctTakesPath) {
  const afterPathLen = afterPath.length
  const lastPathIndex = afterPathLen - 1
  const originalKey = k
  var i = -1
  var n
  var nv
  var ov
  var oov = null
  var exists = true
  var wc = null
  ov = n = o[k]
  if (typeof n !== 'object') return { value: null, parent: null, exists }
  while (n != null && ++i < afterPathLen) {
    k = afterPath[i]
    oov = ov
    if (k !== '*' && !wc && !(typeof n === 'object' && k in n)) {
      exists = false
      break
    }
    if (k === '*') {
      wc = k
      if (i !== lastPathIndex) {
        continue
      }
    }
    if (wc) {
      const wcKeys = Object.keys(n)
      for (var j = 0; j < wcKeys.length; j++) {
        const wck = wcKeys[j]
        const wcov = n[wck]
        const kIsWc = k === '*'
        if (kIsWc || (typeof wcov === 'object' && wcov !== null && k in wcov)) {
          if (kIsWc) {
            ov = wcov
          } else {
            ov = wcov[k]
          }
          nv = (i !== lastPathIndex)
            ? ov
            : (isCensorFct
              ? (censorFctTakesPath ? censor(ov, [...path, originalKey, ...afterPath]) : censor(ov))
              : censor)
          if (kIsWc) {
            n[wck] = nv
          } else {
            if (wcov[k] === nv) {
              exists = false
            } else {
              wcov[k] = (nv === undefined && censor !== undefined) || (has(wcov, k) && nv === ov) ? wcov[k] : nv
            }
          }
        }
      }
      wc = null
    } else {
      ov = n[k]
      nv = (i !== lastPathIndex)
        ? ov
        : (isCensorFct
          ? (censorFctTakesPath ? censor(ov, [...path, originalKey, ...afterPath]) : censor(ov))
          : censor)
      n[k] = (has(n, k) && nv === ov) || (nv === undefined && censor !== undefined) ? n[k] : nv
      n = n[k]
    }
    if (typeof n !== 'object') break
    if (ov === oov) {
      exists = false
    }
  }
  return { value: ov, parent: oov, exists }
}

This can fix pino issue. but it's a Band Aid

if (ov === oov) {
  exists = false
}
sgurnani99 commented 1 year ago

Hello @mrjwt , @mcollina I was wondering if this issue is resolved as I face a similar problem where the original object is modified adding 'REDACTED' to the properties of the original object

Additionally I tried the restore function in an attempt to fix it and that appears to undo the redactions on object generated by redact function. Is that the intended behavior?

Thanks

nagy135 commented 1 year ago

any progress on this?

carlos-xpln commented 1 year ago

any progress on this?

x2

nagy135 commented 1 year ago

We should at least put warning "do not use multiple levels of wildcards!" to readme because this issue is actually breaking services, mutating data potentially causing damage

This mutating issue happens on:

└─┬ pino@8.8.0
  └── fast-redact@3.1.2
jsumners commented 1 year ago

Pull requests to resolve issues are welcome.

nagy135 commented 1 year ago
/*
 /test/index.js
*/
test('redact.restore function places original values back in place (deeply nested wildcards)', ({ end, is }) => {
  const censor = 'test'
  const value = 'hidden'
  const paths = ['*.c2.*.d']
  const redact = fastRedact({ paths, censor, serialize: false })
  const o = {
    x: { c1: [ { d: value, e: 'leave me be' } ] },
    y: { c2: [ { d: value, f: 'I want to live' } ] },
    z: { c3: [ { d: value, g: 'I want to run in a stream' } ] }
  }

  redact(o)
  is(o.x.c1[0].d, value)
  is(o.y.c2[0].d, censor)
  is(o.z.c3[0].d, value)
  redact.restore(o)
  is(o.x.c1[0].d, value)
  is(o.y.c2[0].d, value)
  is(o.z.c3[0].d, value)
  end()
})

It seems to be happening when combined with arrays ...testcase that started this issue works now, this one does not

matt-clarson commented 1 year ago

i've opened PR that should fix this issue, if someone can take a look https://github.com/davidmarkclements/fast-redact/pull/59