mirror of
https://github.com/pocket-id/pocket-id.git
synced 2025-12-11 07:32:57 +03:00
🐛 Bug Report: Login Code too long #100
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @James18232 on GitHub.
Reproduction steps
occasionally (1% of the time) when you click create login code, the app generates a login code longer than 6 digits (16 digits).
the app will continue to generate long login codes until the container is restarted, after which it will return to regular length.
I cannot reliably reproduce the bug.
Expected behavior
it should create a 6 digit login code.
Actual Behavior
creates a login code longer than 6 digits.
Version and Environment
pocketid: v1.6.4
reverse proxy: caddy
docker: (rootless)
Log Output
none available.
@James18232 commented on GitHub:
I have caught the behaviour again and confirmed no drift - all is ok on that end. Interestingly the behaviour remained on fresh container recreation - pointing to a different issue.
The issue i think is the front end sends the time based on the browser local timezone, and the backend also checks based on local timezone. This is an unsafe assumption as the user may be in a different timezone to the backend server.
This explains the ‘randomness’ of the issue - even a broken clock is right twice a day.
The main issue is the backend container (alpine base) does not respect the local timezone variables set for the container - it falls back to UTC.
The fix would be to send the time from the frontend in UTC and compare the time at the backend also in UTC.
@stonith404 commented on GitHub:
Is there a possibility that the clock in your container is drifting? This seems to be the most likely cause, especially since you mentioned that it starts working again after you restart the container.
When you create a one-time access token, the frontend sends the current time plus 15 minutes as the expiration date to the backend. The backend only generates a 6-digit code if the expiration date is less than or equal to 15 minutes in the future. So if the container's clock is out of sync it might create a 16-digit code.
@kmendell commented on GitHub:
@James18232 The login codes if teh time period is short will be 15 characters, are you saying, once one is generated at long length no matter what time period you choose they always are over 15 characters?
@James18232 commented on GitHub:
Didn’t know you could even change the valid time period to something other than 15min..
Occasionally on code generation it will “randomly” decide to stop generating 6 digits and start with 16 digit ones, after which it gets stuck at the longer codes.. only a container restart seems to fix it..
@James18232 commented on GitHub:
Next time i catch the behaviour i will check for drift.
@James18232 commented on GitHub:
@stonith404 thanks. I’ve checked explicitly and can also see its functioning as expected (regardless of TZ). I have set up some proper logging to get a better idea of the issue. If it sees a smidge over 15min that will confirm things.
@stonith404 commented on GitHub:
I'm 99% sure that the backend handles timezones correctly. Could you tell me which database you are using and which timezones you are testing? Also, if the issue is really caused by timezones, it shouldn't work sometimes and not at other times. Did you change timezones when you tried it?
By the way, setting the timezone should work with the
TZvariable. I tested it withTZ=Europe/Zurich, and it works for me.@stonith404 commented on GitHub:
Yeah, setting the
TZenv variable doesn’t change the container’s system timezone, but it does affect Pocket ID, which you can verify by checking the timestamps in the logs.The reason the expiration is configurable is because both users and admins use the same endpoint for creating login codes. An admin can define how long a code should be valid when generating one for another user. Beyond that, I think it makes sense to keep expiration configurable so you can adjust it to your needs when using the API.
That said, different timezones between frontend and backend shouldn’t matter. The frontend always sends the time along with its timezone to the backend, and the backend converts accordingly. In my tests, I can successfully create a 6-digit login code even when frontend and backend are running in different timezones.
Could you share which timezones you’re using? Additionally, can you try to send a few requests to the login code creation endpoint with different expiration times? What happens if you set the expiration to only 5 minutes in the future?
@ItalyPaleAle commented on GitHub:
I think I have a fix in #855
SInce the expiration time comes from the client, it's possible that there's a (small) drift and that may cause the issues we have here.
@James18232 commented on GitHub:
thats right - it works randomly sometimes and not others - due to the reliance on timezones i believe. setting a TZ does not work for me - you can test with the ‘date’ command inside the container which spits out utc. Then install the tzdata package, after which the container ‘recognizes’ the passed in tz variable.
This may be a quirk of rootless docker compared to rootfull docker however I wouldn’t think so.
Regardless the check requires a common timezone on frontend and backend and that should just be set to UTC.
i also dont get the whole concept of a longer token. the access token is fixed for 15minutes - its like the backend anticipated longer time periods could be set (maybe by the user in settings?) and because it its a longer time period justifies a longer code..
you could cut out all of the complexity by ignoring any frontend time and just setting 15min codes.
Regardless the check requires a common timezone on frontend and backend and that should just be set to UTC, then the current code should work fine.
@ItalyPaleAle commented on GitHub:
Thanks for confirming!
@stonith404 commented on GitHub:
Interesting issue, thanks for confirming 😁
@James18232 commented on GitHub:
Just closing this out. Confirming the issue was triggered by very small drift per @ItalyPaleAle. the time until expiry when bugging was in the realm of 15m0.00006893s (as an example).
i will mark as fixed.