change expiresAt to be nullable and do default check with at for backward compatibility with mobile apps#127
change expiresAt to be nullable and do default check with at for backward compatibility with mobile apps#127kf7mxe wants to merge 5 commits into
Conversation
UnknownJoe796
left a comment
There was a problem hiding this comment.
Automated Review (commit c2e4045)## PR #127 Review: Make expiresAt nullable for backwards compatibility
Overall this is a reasonable compatibility shim, but there's one significant concern and a couple of minor ones.
1. Fragile fallback — 1.hours hardcoded in two places must match proofExpiration
Files: Signer.ext.kt:35, Proof.ext.kt:10
Issue: The signingInfo function falls back to at + 1.hours when expiresAt is null, and the expired check does the same. This only produces correct signature verification if the original proof was signed with a proofExpiration of exactly 1 hour.
All current proof method implementations default proofExpiration to 1.hours, so this works today. However, PinBasedProofEndpoints takes proofExpiration as a required parameter with no default — any consumer of this library can set it to something other than 1 hour. If they do, and a mobile client strips expiresAt during the round-trip, the signingInfo fallback will compute a different payload than what was originally signed, and signature verification will silently fail.
Why it matters: This is an auth-critical code path. The backwards compatibility guarantee is actually conditional on a specific configuration, which isn't documented anywhere. Future developers (or current library consumers) won't know this.
Suggested fix: Either:
- Extract the fallback duration into a shared constant with a clear name like
DEFAULT_PROOF_EXPIRATION_FALLBACKand document that it must match the signing-time expiration for null-expiresAtproofs to verify, or - Remove the fallback from
signingInfoentirely and instead haveSigner.verify/verifyBlockingtry verification with the fallback only if the first attempt fails (try original payload, then try withat + 1.hours).
Confidence: High
2. Stale doc comment on Proof.expiresAt
File: proofModels.kt:113
The doc comment still says @property expiresAt The timestamp when this proof is no longer valid. without mentioning it's nullable or what null means. The inline comment // Nullable for now for backwards compatibility with mobile apps is only visible in source, not in generated docs.
Suggested fix: Update the KDoc:
* @property expiresAt The timestamp when this proof is no longer valid.
* Null for backwards compatibility with older clients; when null, defaults to [at] + 1 hour.Confidence: High
3. No test coverage for the null expiresAt path
Files: No test changes in the diff.
The backwards compatibility scenario (proof deserialized without expiresAt → signature still verifies → expiry check still works) has no test. Given this is security-critical code with a subtle correctness requirement (the fallback must match the signing duration), a test would catch regressions.
Confidence: Medium
# Conflicts: # sessions/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/extensions/Proof.ext.kt # sessions/src/main/kotlin/com/lightningkite/lightningserver/sessions/proofs/extensions/Signer.ext.kt
made expiresAt nullable and do default check with at for backward compatibility with mobile apps