Skip to content
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

Tenant token generated by meilisearch js expires too long. #1495

Closed
tnamao opened this issue May 25, 2023 · 5 comments · Fixed by #1502
Closed

Tenant token generated by meilisearch js expires too long. #1495

tnamao opened this issue May 25, 2023 · 5 comments · Fixed by #1502
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@tnamao
Copy link

tnamao commented May 25, 2023

Description

Currently, the tenant token generated by the Go program has been changed to be generated by meilisearch js.
When the expiration date is fixed and the expiration date set for the claim in Go and JS is compared
I found that Go is set to unixtime and JS is set to millisec.

Is exp: expiresAt?.getTime() in this function missing conversion to unixtime?
https://github.com/meilisearch/meilisearch-js/blob/main/src/token.ts#L97-L119

The meilisearch's documentation also seems to convert Date.now() values ​​to unixtime.
https://www.meilisearch.com/docs/learn/security/tenant_tokens#generating-a-tenant-token-with-a-third-party-library

Expected behavior

Tenant token's exp is set unixtime.

Current behavior

Tenant token's exp is set millisec.

Regards,

@bidoubiwa
Copy link
Contributor

bidoubiwa commented May 25, 2023

Hey @tnamao
Thanks for the feedback :) It should definitely be fixed

@bidoubiwa bidoubiwa added bug Something isn't working good first issue Good for newcomers labels May 25, 2023
@qmakani
Copy link

qmakani commented May 29, 2023

@tnamao based on the meili docs, it needs unix epoch time.

The code on line 115 of token.ts uses getTime which return unix epoch time as expected: Date..prototype.getTime MDN Docs

@bidoubiwa
Copy link
Contributor

bidoubiwa commented May 29, 2023

Hey @qmakani, I looked around but indeed, the official definition of an unix timestamp is in seconds. Though it may sometimes be stored in different formats, see Wikipedia:

Unix time[a] is a date and time representation widely used in computing. It measures time by the number of seconds that have elapsed since 00:00:00 UTC on 1 January 1970, the Unix epoch, without adjustments made due to leap seconds. In modern computing, values are sometimes stored with higher granularity, such as microseconds or nanoseconds.

@tnamao What are the consequences of providing the timestamp in milliseconds? Is meilisearch considering the expiring date to be in the year 200023 ?

I don't mind fixing the timestamp to be in seconds, but I'm just wondering what issue the milliseconds are raising.

@bidoubiwa
Copy link
Contributor

bidoubiwa commented May 29, 2023

Hey @tnamao
After discussing with the @meilisearch/engine-team , indeed, the expiration date is expected to be in seconds. It'll be fixed in this pr #1502

@tnamao
Copy link
Author

tnamao commented May 30, 2023

@bidoubiwa
Thank you for the fix !!
I will comment on the PR.

What are the consequences of providing the timestamp in milliseconds? Is meilisearch considering the expiring date to be in the year 200023 ?

Multiplying the difference between the new Date().getTime() and the epoch by 1000 gives us 55378 years...

@meili-bors meili-bors bot closed this as completed in ed73bdf May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
3 participants