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: properly restore nested wildcard values #59

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

matt-clarson
Copy link
Contributor

fixes #50
also fixes pinojs/pino#1745

This fixes the issue of restoring redacting values across multiple nested wildcards - the problem was that the redact/restore modifiers assumed the same value could be used to restore all instances of a redacted path. To fix, I've changed the specialSet function to keep track of the paths it visits in a tree, so that the correct path/value can be stored for use in restoring.

This has required a couple of extra modifications to the specialSet and iterateNthLevel functions:

  • special set now doesn't return anything - it takes the store as an argument and pushes restore instructions to it directly
    • this also means that there's no need to return an exists value for paths that aren't being redacted - where that was being done we can just not update the store
  • the iterateNthLevel function also doesn't returning anything now, similarly taking and mutating the store itself
    • as far as i can tell, this change also fixed a separate bug where iterateNthLevel would exit early, leaving some parts of a wildcard path untouched - this is covered in a new test case

Copy link

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

Very hard to make sense of what's going on here without intimate knowledge of this package, so I'll just comment for now.

I'm fairly confident that this package has extensive code coverage and therefore, if all tests pass in addition to the new ones, you fixed the issue without introducing any regression, but the package maintainer is probably the best person to confirm that.

Apart from this, excellent job, the code is quite cryptic and solving the issue without introducing regressions is a remarkable achievement.

lib/modifiers.js Outdated Show resolved Hide resolved
lib/modifiers.js Outdated Show resolved Hide resolved
lib/modifiers.js Outdated Show resolved Hide resolved
lib/modifiers.js Outdated Show resolved Hide resolved
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
Copy link
Collaborator

this is amazing work, thanks!

@mcollina mcollina merged commit a325e20 into davidmarkclements:main Jul 27, 2023
renovate bot referenced this pull request in valora-inc/logging Jul 30, 2023
[![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.2.0` ->
`^3.3.0`](https://renovatebot.com/diffs/npm/fast-redact/3.2.0/3.3.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/fast-redact/3.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fast-redact/3.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fast-redact/3.2.0/3.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fast-redact/3.2.0/3.3.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

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

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

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

#### What's Changed

- fix: properly restore nested wildcard values by
[@&#8203;matt-clarson](https://togithub.com/matt-clarson) in
[https://github.com/davidmarkclements/fast-redact/pull/59](https://togithub.com/davidmarkclements/fast-redact/pull/59)

#### New Contributors

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

**Full Changelog**:
davidmarkclements/fast-redact@v3.2.0...v3.3.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:eyJjcmVhdGVkSW5WZXIiOiIzNi4yNC4yIiwidXBkYXRlZEluVmVyIjoiMzYuMjQuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

Log redaction mutates the original object Original object is modified and not restored in any way
4 participants