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

Improve some variable naming (internal enhancement) #26

Closed
natevw opened this issue Feb 8, 2018 · 2 comments
Closed

Improve some variable naming (internal enhancement) #26

natevw opened this issue Feb 8, 2018 · 2 comments

Comments

@natevw
Copy link
Collaborator

natevw commented Feb 8, 2018

This bit of code is a potential maintenance issue:

var str = val.slice(0, val.lastIndexOf('.'))
    , mac = exports.sign(str, secret);

We have three variables val, str, mac involved in some important logic and their names are confusing.

  • val is the incoming untrusted (but probably signed) input, e.g. "iamroot.f5da773d717015ffd4ffa4d0f9a87ed57b30811a", or "iamroot.b629a45f1f6175ddcddfbaac08dd2562" or "will.i.am" or "moe" or "\\"><script>alert('gotcha')</script>" or "" [or in a buggy app 42 or undefined or [Object object]]
  • str is the value we would like to return, iff it was signed by us, e.g. "iamroot"
  • mac is what val should be, if str were signed by us, e.g. "iamroot.f5da773d717015ffd4ffa4d0f9a87ed57b30811a"

Maybe rename these something like:

  • val → input
  • str → tentativeResult
  • mac → originalOutput [???]

Not urgent.

@dschnare
Copy link

dschnare commented Feb 1, 2020

@natevw I agree. Only I would perhaps suggest the following changes to improve understandability and maintainability:

  var decodedVal = decode(val)
    , expectedToken = exports.sign(decodedVal, secret)
    , expectedTokenBuffer = Buffer.from(expectedToken)
    , valBuffer = Buffer.from(val);

  return crypto.timingSafeEqual(expectedTokenBuffer, valBuffer) ? decodedVal : false;

Like you said though this is not urgent, otherwise I would have created a PR for this myself. I'll let the maintainers decide whether this is beneficial.

@natevw
Copy link
Collaborator Author

natevw commented Feb 4, 2020

Thanks, yeah I like your "expected" terminology since its meaning is pretty clear. No worries about the PR since I don't plan on changing this just for its own sake. Rather, my thinking is to wait until (if…) the code were changing anyway…and so far there's been no reason for other maintenance.

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

No branches or pull requests

2 participants