docs/docs/users/reference/env_variables.md (1)

53-53: Minor style suggestion: simplify wording.

The phrase "Whether or not to disable" could be shortened to "Whether to disable" for improved clarity.

-| `FOREST_JWT_DISABLE_EXP_VALIDATION`                       | 1 or true                        | empty                                          | 1                                                             | Whether or not to disable JWT expiration validation                                                                   |
+| `FOREST_JWT_DISABLE_EXP_VALIDATION`                       | 1 or true                        | empty                                          | 1                                                             | Whether to disable JWT expiration validation                                                                   |

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/auth/mod.rs (1)

45-52: Warn when globally bypassing exp and simplify claim handling.

Emit a warning to surface the security tradeoff and simplify by mutating required_spec_claims in place.

-    let mut claims = validation.required_spec_claims.clone();
-    claims.remove("exp");
-    let buff: Vec<_> = claims.iter().collect();
-    validation.set_required_spec_claims(&buff);
-    validation.validate_exp = false;
+    tracing::warn!(
+        "JWT expiration validation is disabled via FOREST_JWT_DISABLE_EXP_VALIDATION; \
+         use only in development/testing or explicit cross-system token sharing."
+    );
+    validation.required_spec_claims.remove("exp");
+    validation.validate_exp = false;

src/auth/mod.rs (2)

29-31: Avoid serializing "exp": null; omit the claim when None.

Add skip_serializing_if so tokens without exp don’t emit a null value and better mirror peers (e.g., Lotus).

-    #[serde(default)]
-    exp: Option<usize>,
+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    exp: Option<usize>,

---

68-75: This currently encodes "exp": null; ensure the field is omitted instead.

As written, exp: None will serialize to null. If the intent is to produce tokens without the exp claim, rely on skip_serializing_if on Claims.exp (see earlier comment). No further changes to this helper needed once that attribute is added.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

src/auth/mod.rs (1)

128-130: Consider RAII guard for env var cleanup on panic.

The test logic is correct and properly validates both tokens without expiration and expired tokens when validation is disabled. The unsafe blocks are necessary because std::env::set_var and std::env::remove_var are inherently unsafe in Rust (they can cause data races).

However, if the test panics between setting and removing the env var, the cleanup at line 147-149 won't execute, potentially affecting subsequent tests. Consider implementing the RAII guard pattern suggested in the previous review to ensure cleanup even on panic:

struct EnvVarGuard {
    key: &'static str,
}

impl EnvVarGuard {
    fn new(key: &'static str, value: &str) -> Self {
        unsafe {
            std::env::set_var(key, value);
        }
        Self { key }
    }
}

impl Drop for EnvVarGuard {
    fn drop(&mut self) {
        unsafe {
            std::env::remove_var(self.key);
        }
    }
}

Then replace lines 128-130 and 147-149 with:

let _guard = EnvVarGuard::new("FOREST_JWT_DISABLE_EXP_VALIDATION", "1");

Note: This addresses the previous review feedback which you marked as "Done" but wasn't fully implemented.

Also applies to: 147-149

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro