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

feat(nockBack): substitutions option #2556

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stephenprater
Copy link
Contributor

VCR has a feature called Filter Sensitive Data - which is super handy if you're recording HTTP interactions, since data like API keys, passwords etc can show up in the fixtures, particularly if that data is passed via something other than a Request Header.

This implements a similar feature in Nock Back - the only place where it's really a problem.

Requested in #2230

Copy link
Member

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

I like it and happy to merge as is.

But I think that the exact values that need to be masked are often times not avaialble at the time when configuring nock. Would it make sense to add REGEX patterns as well?

Alternatively I wonder if we should add a callback option that receives the final fixture and that can mutate it in anyway you want.

My usecase usually looks like this: https://github.com/octokit/fixtures/blob/main/lib/remove-credentials.js#L12

Applied like this:
https://github.com/octokit/fixtures/blob/c71bcb3a802a45849d81500554c69be7f8f35add/lib/record-scenario.js#L44-L52

@gr2m gr2m changed the title Enable substitions for simple secret hiding feat(nockBack): substitutions option Nov 27, 2023
@stephenprater
Copy link
Contributor Author

Trying to think this through: I think if you make the substitutions object have an interface like:

interface MyInterface {
  [key: string]: string | ((key: string) => string);
}

There's a source of potentially subtle bugs here in that I remove those things from the fixture by scanning for exact matches. So if your callback function returned a common string it could be substituted in places where it's not supposed to be. (Although, hopefully the secrets you're hiding aren't common strings)

Would this let you use a Regexp how you're thinking? You need to pass the key into the callback function so you'd have some idea what value to retrieve. Contrived but something like this?

back("my_fixtures.json", {
  substitutions: {
      A_SECRET_KEY: (k) => process.env.A_SECRET_KEY.replace(/production/, "test")
  }
 })

Or are you talking about using a Regex as a key?

back("my_fixtures.json", {
  substitutions: {
     /KEY_[0-9a-f]{16}/:  process.env.MY_KEY
  }
})

I'm having a little trouble understanding how the second things would work. Nockback is always going to record a "static" string right, or the request wouldn't have worked? The value of the substitutions object is the static value and then it gets subbed out for the key. I think that value always needs to be a string by the time we write the fixture, and the "key" always needs to be a string because that's what we record instead of whatever the value is.

@gr2m
Copy link
Member

gr2m commented Dec 2, 2023

Or are you talking about using a Regex as a key?

I think something like this could work

back("my_fixtures.json", {
  substitutions: {
     /KEY_[0-9a-f]{16}/:  "[REDACTED]"
  }
})

A more flexible approach could be to add a callback before the file is written

back("my_fixtures.json", {
  beforeWrite(content) {
    // do your string replacements here
    return newContent
  }
})

@stephenprater
Copy link
Contributor Author

stephenprater commented Dec 14, 2023

back("my_fixtures.json", {
  substitutions: {
     /KEY_[0-9a-f]{16}/:  "[REDACTED]"  # not the way this works
     "[REDACTED]": /KEY_[0-9a-f]{16}/   # the way it currently works
  }
})

Oh - this is working the other direction - the "redacted value" is the key in the substitutions object because the redacted value should be static.

I can put the beforeWrite hook in as well (I think you could couple that with afterRead) to get to the same functionality.

@gr2m
Copy link
Member

gr2m commented Jan 14, 2024

sounds good 👍🏼

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.

None yet

2 participants