hide secrets from log output #35

kriswep · 2017-10-04T18:55:07Z

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

add log and sanitze function and used that over console.log closes prose#34

compumike08 · 2017-10-04T19:54:09Z

@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.

compumike08

Thank you @kriswep for taking the time to address this security issue. I’m sure the entire community which uses gatekeeper is very greatful for your time and effort (I know I am). I did have a few minor suggestions to improve code quality and future maintainablity. Feel free to look at my line comments in this review to see my suggestions, and implement any which you agree with. Thanks again.

compumike08 · 2017-10-05T01:00:33Z

server.js

+function log(label, value, sanitized) {
+  value = value || '';
+  if (sanitized){
+    if (typeof(value) === 'string' && value.length > 10){


Replace the hard-coded 10 with a named constant to avoid introducing “magic numbers”.

compumike08 · 2017-10-05T01:02:25Z

server.js

+  value = value || '';
+  if (sanitized){
+    if (typeof(value) === 'string' && value.length > 10){
+      console.log(label, value.substring(3,0) + '...');


Replace hard-coded ’...’ string with a named constant. This will make it easy to change the displayed string everywhere in the future without having to hunt through the code for each hard-coded instance.

compumike08 · 2017-10-05T01:03:09Z

server.js

+    if (typeof(value) === 'string' && value.length > 10){
+      console.log(label, value.substring(3,0) + '...');
+    } else {
+      console.log(label, '...');


Replace hard-coded ’...’ string with a named constant. This will make it easy to change the displayed string everywhere in the future without having to hunt through the code for each hard-coded instance.

compumike08 · 2017-10-05T01:04:59Z

server.js

@@ -49,6 +53,19 @@ function authenticate(code, cb) {
  req.on('error', function(e) { cb(e.message); });
 }

+function log(label, value, sanitized) {


Add a JSDoc comment block to this function to document the function, the parameters, what each parameter is for, and what types of data are expected for each parameter.

compumike08 · 2017-10-05T01:06:47Z

server.js

-    var result = err || !token ? {"error": "bad_code"} : { "token": token };
-    console.log(result);
+    if ( err || !token ) {
+      result = {"error": "bad_code"};


Replace hard-coded ”error” and ”bad_code” strings with named constants to make it easier to change the values in the future, in case it is ever necessary to do so.

compumike08 · 2017-10-05T01:07:42Z

server.js

+      result = {"error": "bad_code"};
+      log(result.error);
+    } else {
+      result = {"token": token};


Replace hard-coded ”token” string with a named constant to make it easier to change the value in the future, in case it is ever necessary to do so.

compumike08 · 2017-10-05T01:08:17Z

server.js

+      log(result.error);
+    } else {
+      result = {"token": token};
+      log("token", result.token, true);


Replace hard-coded ”token” string with a named constant to make it easier to change the value in the future, in case it is ever necessary to do so.

and document log fn

kriswep · 2017-10-05T19:24:40Z

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

Looks good to me.

compumike08 · 2017-10-06T19:29:37Z

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 · 2017-10-06T20:09:52Z

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

dereklieu · 2017-10-09T21:08:17Z

server.js

-    console.log(result);
+    if ( err || !token ) {
+      result = {"error": "bad_code"};
+      log(result.error);


This is only indirectly related to what your PR fixes, but since we're touching this code, one thing I'd find useful is, in the case where we are returning err, logging that instead of just logging bad_code. This would make it easier to debug, since it would likely return error messages from whatever Oauth provider this is attached to.

I added a change for this.

However, while testing, I noticed your implementation in the authenticate function swallows quite a lot of errors. I opened issue #36 for that. I would prefer that to be fixed in another pr.

dereklieu · 2017-10-09T21:09:46Z

@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.

hide secrets from log output

7b54030

add log and sanitze function and used that over console.log closes prose#34

compumike08 reviewed Oct 5, 2017

View reviewed changes

replace introduced strings

52ba3f4

and document log fn

compumike08 reviewed Oct 6, 2017

View reviewed changes

dereklieu reviewed Oct 9, 2017

View reviewed changes

allow server err msg to be logged

fc61ca4

kriswep mentioned this pull request Oct 15, 2017

authenticate functions swallows errors from github #36

Open

dereklieu merged commit 5169fa9 into prose:master Oct 17, 2017

kriswep deleted the sanitize-logs branch October 20, 2017 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hide secrets from log output #35

hide secrets from log output #35

kriswep commented Oct 4, 2017

compumike08 commented Oct 4, 2017

compumike08 left a comment

compumike08 Oct 5, 2017

compumike08 Oct 5, 2017

compumike08 Oct 5, 2017

compumike08 Oct 5, 2017

compumike08 Oct 5, 2017

compumike08 Oct 5, 2017

compumike08 Oct 5, 2017

kriswep commented Oct 5, 2017

compumike08 left a comment

compumike08 commented Oct 6, 2017

kriswep commented Oct 6, 2017

dereklieu Oct 9, 2017

kriswep Oct 15, 2017 •

edited

Loading

dereklieu commented Oct 9, 2017

hide secrets from log output #35

hide secrets from log output #35

Conversation

kriswep commented Oct 4, 2017

compumike08 commented Oct 4, 2017

compumike08 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kriswep commented Oct 5, 2017

compumike08 left a comment

Choose a reason for hiding this comment

compumike08 commented Oct 6, 2017

kriswep commented Oct 6, 2017

Choose a reason for hiding this comment

kriswep Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

dereklieu commented Oct 9, 2017

kriswep Oct 15, 2017 •

edited

Loading