Hides token contents in logStartupOptions if they arrive as a buffer

[drdaz]

@flovilmart So this change hides the crypto data for my specific case. But as I've created this PR, it occurs to me that this probably needs to be done in a lot of places:

https://github.com/node-apn/node-apn/blob/master/doc/provider.markdown

There are numerous parameters in there that can end up dumping sensitive info in the log. Maybe it's best to just redact the whole thing?

[codecov]

Codecov Report

Merging #5322 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5322      +/-   ##
==========================================
- Coverage    93.9%   93.88%   -0.03%     
==========================================
  Files         123      123              
  Lines        8972     8972              
==========================================
- Hits         8425     8423       -2     
- Misses        547      549       +2

Impacted Files	Coverage Δ
src/Adapters/Auth/httpsRequest.js	95.23% <0%> (-4.77%)	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	97.07% <0%> (-0.09%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cbe8bd...a223bfc. Read the comment docs.

[flovilmart]

Thst’s Neat but can you check the key path completely exists? This could crash easily

[drdaz]

Ahh right. I made some untrue assumptions about how JS handles nesting :)

As I alluded to in my first comment though, I have some doubts about the best way to cover this. To cover all the sensitive info that can be dumped to log in this way means a lot of checks. Maybe it would be best to just include the data we want to present (if indeed any of the details actually need to be logged), rather than redact everything we don't?

[flovilmart]

Or we could print this only when verbose logs are on. Technically besides those, nothing is really critical no?

[drdaz]

That would also work. There is some value to seeing some output here; it's definitely nice to see that the config options you've provided have taken hold and are what you expect them to be. Restricting that output to verbose = true makes sense since it's a debugging situation.

[drdaz]

Here's a patch that hides the details of the push field if the VERBOSE environment variable isn't truthy.

In retrospect I'm not sure this is the right way check this, since verbose can be set via command line argument. Is there some other place I can fish this value out of?

[drdaz]

This won’t work, as it will fall into the next case as the value type will be ‘object’ :)

It does work; the value is already changed when it gets there :)

[flovilmart]

This won’t work, as it will fall into the next case as the value type will be ‘object’ :) ...

@drdaz 🤦‍♂️ right... sorry

_{Sent with GitHawk}

[drdaz]

This is fine then? If we start the server using --verbose rather than the VERBOSE env variable, it'll still work? I didn't check this...

[flovilmart]

This is fine then? If we start the server using --verbose rather than the VERBOSE env variable, it'll still work? I didn't check this...

@drdaz this won’t work if you pass —verbose instead of process.env. In this case, you could check he options.verbose status

_{Sent with GitHawk}