prose / gatekeeper

Enables client-side applications to dance OAuth with GitHub.
MIT License
760 stars 182 forks source link

hide secrets from log output #35

Closed kriswep closed 6 years ago

kriswep commented 6 years ago

I added a log and sanitize function and used that over console.log.

Secret stuff (gh client id, gh client secret, codes and tokens) will be truncated to 3 characters. If the secret is less equal 10 characters or not a string it will be replaced by three dots.

So it logs sthg like this

Configuration
oauth_client_id: ac4...
oauth_client_secret: 2ea...
oauth_host: github.com
oauth_port: 443
oauth_path: /login/oauth/access_token
oauth_method: POST
Gatekeeper, at your service: http://localhost:9999
authenticating code: 439...
bad_code
authenticating code: ...
bad_code
authenticating code: 628...
token ee7...

closes #34

compumike08 commented 6 years ago

@kriswep: I have a few suggestions to improve your implementation. I can’t go into details right now, but I’ll try to post a more detailed comment later tonight before this gets merged.

kriswep commented 6 years ago

Alright, I documented the new log function and replaced the hardcoded strings in there.

I did not change the strings in the authenticate route function. I felt doing so would actually reduce readability by providing very minor benefits. "token", "error" and "bad_code" in there were part of the output users saw and used for years, right? I don't think they would need to be changed anytime soon.

BTW: I was very tempted to use const for the var declarations but decided against that. Main reason being, that the rest of the project is still ES5. What would be your opinion towards using ES6? You declared node 6.x as engine, so you could use ES6 without transpiling for the most part. Just wondering...

compumike08 commented 6 years ago

Regarding const vs var, even though transpiling wouldn’t be an issue here, my personal opinion on the issue is that this is a case where consistency may be more important than correctness. Having const in just a few places in the codebase and still using var everywhere else might be a little confusing. I do think that at some point this project could be refactored to replace all instances of var with let/const, but unless/until that happens, my personal opinion is that it is best to be consistent.

kriswep commented 6 years ago

Sounds reasonable. Would be happy to help moving this project forward in that regard.

dereklieu commented 6 years ago

@kriswep thanks for the pr. As far as the logic goes, it looks good. I agree we shouldn't change the format of the returned object. I'll do some testing of the code tomorrow, but in the meantime I left a comment about logging errors. Thanks again for your work on this.