Authenticator-Extension / Authenticator

Authenticator generates 2-Step Verification codes in your browser.
https://authenticator.cc
MIT License
3.42k stars 798 forks source link

[FIX] Seconds not properly prevented from being negative #1310

Open olfek opened 2 months ago

olfek commented 2 months ago

The current code ...

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/store/Accounts.ts#L90

... fails in these two examples:

0 - 61 + 60 < 0
59 - 120 + 60 < 0

The offset (middle value) can be as low as -300.

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/syncTime.ts#L27

So the least you can have is -241 (59 - 300), the + 60 is not enough to make a positive number.


Without this fix, in the examples given above, the effect will be:

Countdown circle animation may be out of sync with the current time step because animationDelay will be stuck at 0s.

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/components/Popup/EntryComponent.vue#L22-L29

timeout class will never activate unless entry.period < 5 because the left side of the < condition will be stuck at entry.period.

https://github.com/Authenticator-Extension/Authenticator/blob/6511920bac98576e3c97fe2d213b23e9dc899422/src/components/Popup/EntryComponent.vue#L59

olfek commented 2 months ago

Requesting review @Sneezry @mymindstorm

mymindstorm commented 2 months ago

It's on my to-do list, but might take me a while to get to this

olfek commented 1 month ago

🛎️🛎️🛎️ @Sneezry @mymindstorm

olfek commented 1 month ago

@mymindstorm 📟📟📟

mymindstorm commented 1 month ago

It looks like I don't have permission to push to your branch to fix the lint issues.

image

olfek commented 1 month ago

@mymindstorm I've added you as a collaborator.

olfek commented 1 month ago

FYI, I've never been a fan of prettier, it's too opinionated. The actual code is important, how it's formatted can always be corrected later.

olfek commented 4 weeks ago

@mymindstorm I think we're good to go here

olfek commented 4 weeks ago

@mymindstorm ...

After running prettier with the --write option, I get this from git diff --stat :

 src/store/Accounts.ts | 79 +++++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

Would you like me to commit this? - It will pollute the PR diff a little, I await your decision.

mymindstorm commented 4 weeks ago

That doesn't seem right, have you tried running npm run chrome? It will automatically format it using our configs.

olfek commented 4 weeks ago

@mymindstorm You have collaborator access to my fork, can you take over from here?

olfek commented 4 weeks ago

@mymindstorm ...

That doesn't seem right, have you tried running npm run chrome? It will automatically format it using our configs.

I don't think you have any prettier configs (https://prettier.io/docs/en/configuration)

mymindstorm commented 4 weeks ago

When you use prettier outright instead of through npm run, depending on how your system is configured it might use a different version which may have different rules.

mymindstorm commented 4 weeks ago

I'll get to this when I can just am extremely busy.

olfek commented 4 weeks ago

@mymindstorm ...

When you use prettier outright instead of through npm run, depending on how your system is configured it might use a different version which may have different rules.

https://prettier.io/docs/en/configuration says

Prettier intentionally doesn’t support any kind of global configuration. This is to make sure that when a project is copied to another computer, Prettier’s behavior stays the same. Otherwise, Prettier wouldn’t be able to guarantee that everybody in a team gets the same consistent results.

olfek commented 4 weeks ago

@mymindstorm ...

I propose temporarily (or permanently?) suspending the Prettier CI step until you set up a config file.

olfek commented 1 week ago

@mymindstorm ...

After running prettier with the --write option, I get this from git diff --stat :

 src/store/Accounts.ts | 79 +++++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

Would you like me to commit this? - It will pollute the PR diff a little, I await your decision.

@mymindstorm ...

So ... I was getting a big diff because I was using npx to run prettier like this:

npx prettier --check ./src/* ./src/**/* ./src/**/**/* ./sass/*.scss

... without running npm install, meaning the latest version of prettier (3.3.3) was being used instead of the package.json declared 2.2.1.

The default rules have changed since 2.2.1. See:

https://github.com/prettier/prettier/blob/2.2.1/src/language-js/options.js https://github.com/prettier/prettier/blob/3.3.3/src/language-js/options.js

https://github.com/prettier/prettier/blob/2.2.1/src/main/core-options.js https://github.com/prettier/prettier/blob/3.3.3/src/main/core-options.evaluate.js

olfek commented 1 week ago

@mymindstorm Can you run CI again?

olfek commented 3 days ago

@mymindstorm 📣📣📣