documentation incorrect

[gflarity]

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

•     'W' codes in the future against the provided passcode.  Note,
  

•     it is the calling applications responsibility to keep track of
  

•     'W' and increment it for each password check, and also to adjust
  

•     it accordingly in the case where the client and server become
  

•     out of sync (second argument returns non zero).
  

•     E.g. if W = 100, and C = 5, this function will check the passcode
  

•     against all One Time Passcodes between 5 and 105.
  

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 };
        }
    }

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.

[guyht]

@gflarity - yes, you are correct, it is currently checking for C in each direction.

[gflarity]

THere's another issue, the docs say totp.verify window default is 6. Looks like it's actually 50. Opening new ticket.

[gustavnikolaj]

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 };
        }
    }

[gustavnikolaj]

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.

[gflarity]

@gustavnikolaj

"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' .

[gustavnikolaj]

Then it's the documentation and comments which is invalid I suppose. Better go read that RFC :-)

[coolaj86]

This appears to be the same underlying as https://github.com/guyht/notp/issues/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?

[gazoakley]

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:

https://github.com/mdp/rotp/pull/58

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.