Skip to content

Conversation

@lakshya-sky
Copy link
Contributor

@lakshya-sky lakshya-sky commented Nov 8, 2025

Motivation

jsonwebtoken crate doesn't validate iat claims even if we explicitly ask it, hence we needed tests.

Description

Added unit tests to correctly reject invalid iat claims.

Closes #5074

@lakshya-sky lakshya-sky requested a review from a team as a code owner November 8, 2025 19:46
@lakshya-sky lakshya-sky changed the title tests to verify that out jwt authentication works correctly. tests to verify we reject invalid iat claims. Nov 8, 2025
@lakshya-sky lakshya-sky changed the title tests to verify we reject invalid iat claims. test: verify we reject invalid iat claims. Nov 9, 2025
@lakshya-sky lakshya-sky changed the title test: verify we reject invalid iat claims. test(l1): verify we reject invalid iat claims. Nov 9, 2025
@MegaRedHand MegaRedHand added the L1 Ethereum client label Nov 10, 2025
@MegaRedHand MegaRedHand moved this to In Review in ethrex_l1 Nov 10, 2025
Copy link
Collaborator

@mpaulucci mpaulucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Oppen
Copy link
Contributor

Oppen commented Nov 11, 2025

Are the tests running? They should fail, given the documented behavior of the library.

@lakshya-sky
Copy link
Contributor Author

lakshya-sky commented Nov 11, 2025

Hey @Oppen, yes tests are running here at https://github.com/lambdaclass/ethrex/actions/runs/19242301387/job/55007724747?pr=5245#step:5:843

As you can see when deserialize the claims part of the token we expect iat claim, if its not there it would throw error. When iat is available and its value is not within 60s range then we throw error.
So its safe to assume that we are checking it correctly.

If the library were to check the iat claims it would do the same as we are doing it here.

@Oppen
Copy link
Contributor

Oppen commented Nov 13, 2025

Hey @Oppen, yes tests are running here at https://github.com/lambdaclass/ethrex/actions/runs/19242301387/job/55007724747?pr=5245#step:5:843

As you can see when deserialize the claims part of the token we expect iat claim, if its not there it would throw error. When iat is available and its value is not within 60s range then we throw error. So its safe to assume that we are checking it correctly.

If the library were to check the iat claims it would do the same as we are doing it here.

Yes, a teammate showed me it's being checked externally. Not obvious, but it works.

@mpaulucci mpaulucci enabled auto-merge November 13, 2025 18:01
@MegaRedHand
Copy link
Collaborator

@lakshya-sky thanks for the contribution, but commits need to be signed. See https://github.com/lambdaclass/ethrex/blob/main/CONTRIBUTING.md#commit-signature-verification for how to do it.

auto-merge was automatically disabled November 17, 2025 16:50

Head branch was pushed to by a user without write access

@lakshya-sky
Copy link
Contributor Author

Hey @MegaRedHand, I've fixed the commits but it added two new reviewers while fixing it. Could you guys take a look again. Thanks!

@MegaRedHand MegaRedHand added this pull request to the merge queue Nov 18, 2025
Merged via the queue into lambdaclass:main with commit d69bedc Nov 18, 2025
36 checks passed
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Test jwt claims are correctly validated

4 participants