-
Notifications
You must be signed in to change notification settings - Fork 66
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
documentation incorrect #18
Comments
@gflarity - yes, you are correct, it is currently checking for C in each direction. |
THere's another issue, the docs say totp.verify window default is 6. Looks like it's actually 50. Opening new ticket. |
My understanding of these hotp's is not very deep, but wouldn't the correct behaviour be the one described in the docs? Surely, otp's generated with a counter less than what the application currently has stored should not be considered valid? The comments state the same as the documentation, only the implementation differs: // Now loop through from C to C + W to determine if there is
// a correct code
for(var i = counter - window; i <= counter + window; ++i) {
opt.counter = i;
if(this.gen(key, opt) === token) {
// We have found a matching code, trigger callback
// and pass offset
return { delta: i - counter };
}
} |
And now when I actually went to submit a pull request based on the above, I notice that you have a test that asserts that it verifies a token where the counter is less than the one stored in the application. |
"Surely, otp's generated with a counter less than what the application currently has stored should not be considered valid?" Nope. There's a window. Any values generated with a counter within the window specified are valid, that includes values that are a less than the one currently 'stored' . |
Then it's the documentation and comments which is invalid I suppose. Better go read that RFC :-) |
This appears to be the same underlying as #21. If you want true HOTP instead of TOTP you should simply set the window to 0, right? Does that need to be more clear in the docs? Or should there simply be an option that HOTP only allows forward counting? |
There's no API to find out the current counter value for TOTP - the only way to do it is to write/copy the calculation which somewhat defeats the point of having a library. The nearest comparable Ruby library has added an option for forward only counting: Right now there's no way to prevent reuse of TOTP tokens apart from keeping a list around of what was used when - forward only would be much cleaner and simpler to implement. |
I might be misunderstanding something but I think there's at least one error in the HTOP.verify doc:
window - The allowable margin for the counter. The function will check
Code:
Note that the for loop goes from counter - window, to counter + window. So if W was 100, it would be checking 200 values, not 100. Assuming that C means opt.counter in the docs, it would really be counting from -95 to 105.
The text was updated successfully, but these errors were encountered: