fix(via_verifier_coordinator): reject far-future timestamps #340
Conversation
Summary of ChangesHello @romanornr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a potential vulnerability in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a vulnerability where far-future timestamps were not being rejected. A constant MAX_CLOCK_SKEW_SECONDS is introduced, and a check is added to validate incoming timestamps against it. My review includes a suggestion to improve the clarity of the timestamp validation logic and a more significant recommendation to refactor the new test, which currently doesn't validate the implementation, into a proper unit test by extracting the core logic into a testable function.
| #[test] | ||
| fn test_future_timestamp_validation() { | ||
| let now = chrono::Utc::now().timestamp(); | ||
|
|
||
| // Boundary tests for MAX_CLOCK_SKEW_SECONDS (30 seconds) | ||
| assert!(now + 10 <= now + MAX_CLOCK_SKEW_SECONDS); | ||
| assert!(now + 30 <= now + MAX_CLOCK_SKEW_SECONDS); | ||
| assert!(now + 31 > now + MAX_CLOCK_SKEW_SECONDS); | ||
|
|
||
| // Far-future timestamps must be rejected (VIA-AUTH-003) | ||
| let far_future = now + 157_680_000; | ||
| assert!(far_future > now + MAX_CLOCK_SKEW_SECONDS); | ||
| } |
There was a problem hiding this comment.
This test doesn't validate the implementation logic. The assertions are tautological; for example, assert!(now + 10 <= now + MAX_CLOCK_SKEW_SECONDS) simplifies to assert!(10 <= 30), which is always true and doesn't depend on the code under test. This provides a false sense of security.
A more effective approach would be to extract the timestamp validation logic into a separate, easily testable function. This would allow for proper unit testing of the boundary conditions.
Here's an example of how you could refactor:
-
Extract the logic into a new function:
fn check_timestamp_skew( parsed_timestamp: i64, now: i64, max_age_secs: i64, max_skew_secs: i64, ) -> Result<(), &'static str> { let diff = now - parsed_timestamp; if diff > max_age_secs { return Err("Timestamp is too old"); } if diff < -max_skew_secs { return Err("Timestamp is too far in the future"); } Ok(()) }
-
Write a meaningful test for it:
#[test] fn test_timestamp_skew_validation() { let now = 1_000_000; let max_age = 60; let max_skew = 30; // Valid timestamps assert!(check_timestamp_skew(now, now, max_age, max_skew).is_ok()); assert!(check_timestamp_skew(now - max_age, now, max_age, max_skew).is_ok()); assert!(check_timestamp_skew(now + max_skew, now, max_age, max_skew).is_ok()); // Invalid timestamps assert_eq!(check_timestamp_skew(now - max_age - 1, now, max_age, max_skew), Err("Timestamp is too old")); assert_eq!(check_timestamp_skew(now + max_skew + 1, now, max_age, max_skew), Err("Timestamp is too far in the future")); }
This approach ensures your logic is correctly tested and makes the code more modular.
| if parsed_timestamp > timestamp_now + MAX_CLOCK_SKEW_SECONDS { | ||
| tracing::warn!("Reject timestamp from verifier {}: {} seconds ahead of current time", verifier_index, parsed_timestamp - timestamp_now); | ||
| return Err(ApiError::Unauthorized("Timestamp is too far in the future".into())); | ||
| } |
There was a problem hiding this comment.
While this check is correct, it can be made more consistent with the check for old timestamps by reusing the timestamp_diff variable that is already calculated. This improves code clarity by handling both past and future timestamp checks in a more symmetrical way.
| if parsed_timestamp > timestamp_now + MAX_CLOCK_SKEW_SECONDS { | |
| tracing::warn!("Reject timestamp from verifier {}: {} seconds ahead of current time", verifier_index, parsed_timestamp - timestamp_now); | |
| return Err(ApiError::Unauthorized("Timestamp is too far in the future".into())); | |
| } | |
| if timestamp_diff < -MAX_CLOCK_SKEW_SECONDS { | |
| tracing::warn!("Reject timestamp from verifier {}: {} seconds ahead of current time", verifier_index, -timestamp_diff); | |
| return Err(ApiError::Unauthorized("Timestamp is too far in the future".into())); | |
| } |
1 participant
What ❔
Why ❔
Line A
Line B
Let's say verifier_request_timeout = 60 seconds)
The check asks:
Scenario 30 seconds ago:
timestamp_diff = +30
is it > 60 ?
result = Allowed
Scenario 90 seconds ago:
timestamp_diff = +30
Is it > 60 ?
result = Rejected too old
5 years in future
timestamp_diff
is it > 60?
result = Allowed
The negative number is always less than the postive timeout, so it always passes
Better to don't allow future timestamps.
60 seconds for verifier_request_timeout is fine to account for network delays, processing
30 seconds max_clock_skew_seconds since it only needs to handle clock sync drift
Checklist
zkstack dev fmtandzkstack dev lint.