Closed natevw closed 2 years ago
@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.
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.
This bit of code is a potential maintenance issue:
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 app42
orundefined
or[Object object]
]str
is the value we would like to return, iff it was signed by us, e.g. "iamroot"mac
is whatval
should be, ifstr
were signed by us, e.g. "iamroot.f5da773d717015ffd4ffa4d0f9a87ed57b30811a"Maybe rename these something like:
Not urgent.