search

jaredhanson / passport-totp

TOTP authentication strategy for Passport and Node.js.
MIT License
152 stars 47 forks source link

Codes valid from 10 minutes ago still return success #13

Open lunchbag opened 5 years ago

lunchbag commented 5 years ago

It seems like any delta result from totp.verify is accepted. So even if a one-time code was valid 10 minutes ago, it will still return success..

Any chance we can add an option to specify the limit? I'm happy to open a PR for this, but wanted to open up for a discussion first in case I missed anything or am mistaken on how this should work.

honzahommer commented 5 years ago

@lunchbag Did you try to adjust window option? The default value is 6.

passport.authenticate('totp', { window: 1 })
sgywzy commented 4 years ago

~~Same issue. Setting key period with return done(null, key.key, key.period); gets ignored too.~~

This part works fine, my bad for reporting this as not working.

Gennttii commented 4 years ago

any fix on this?

sgywzy commented 4 years ago

@lunchbag Did you try to adjust window option? The default value is 6.

passport.authenticate('totp', { window: 1 })

@Gennttii any fix on this?

window defines the amount of time(code) which gets accepted after the first one is generated. By default this value is set to 6. Which means that the first code gets generated which is valid for 30sec(key.period in the example) and 6more code(6x 30 sec) needs to be generated before the very first key becomes invalid. Total time needed for the first key to become invalid with default settings is 7 * key.period.

Author doesnt make use of the option values in his code during passport.authenticate, therefore everything will be ignored there.

You need to define window size in where u initialize the strategy.

passport.use(new TotpStrategy({
  window: 1,
},
(async (user, done) => {
...

Changing window: 0, will accept every single code ever generated. This is something we dont want obviously. Changing key.period to less than 30seconds(default) seems to break and no code will be accepted.

If your use case is to accept ONLY the the current code generated for the defined time period. Then the fastes fix i found was replace this line : https://github.com/jaredhanson/passport-totp/blob/9abb4862d60862028ab5af112a331c81d220d77b/lib/strategy.js#L78 with if (!rv || rv.delta !== 0) { return self.fail(); }

rv.delta keeps track of the generated codes. 0 is always the current code while the previous code will be -1, before that -2 etc.. This addition will enforce that only the most recent code is accepted, nothing else.