Skip to content

Commit 931b66a

Browse files
wolpertclaude
andcommitted
fix(security): close 3 low-severity account/side-channel findings
Security review of the auth flows surfaced three low-severity issues, all fixed here: - pk-auth-persistence-dynamodb: refresh-token rotation gated the atomic ConditionExpression on `expiresAtIso > :now`, a lexicographic compare of variable-precision Instant.toString() values (the fractional-seconds field is dropped when zero), so "...:00Z" sorts after "...:00.000001Z" and a just-expired token could rotate within the rotate TOCTOU window. Compare the numeric epoch-second `ttl` attribute instead. Fails closed now. - pk-auth-jwt: PkAuthJwtValidator in stateful mode guarded the AccessTokenStore lookup with `jti != null`, so a validly-signed, in-issuer/-audience, unexpired token with no jti bypassed the revocation gate. Reject jti-less tokens with MissingClaim("jti") when a non-noop store is bound. The library's own tokens always carry a jti, so behavior is unchanged for them. - pk-auth-magic-link: corrected the MagicLinkService.startLogin javadoc, which falsely claimed timing-side-channel resistance; the not-found path returns before JWT issuance and email dispatch, so latency still enables enumeration. Documentation only; no behavior change. All affected modules pass spotlessCheck, unit tests, and the DynamoDB Local refresh-token integration suite (incl. concurrent-rotation and expired-token cases). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 6b9dec1 commit 931b66a

5 files changed

Lines changed: 73 additions & 15 deletions

File tree

CHANGELOG.md

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,39 @@ tags.
1111

1212
## [Unreleased]
1313

14+
## [1.2.0]
15+
16+
### Security
17+
18+
- **DynamoDB refresh-token rotation now compares expiry numerically, not
19+
lexically.** `DynamoDbRefreshTokenRepository.rotateAtomically` gated the
20+
atomic-rotate `ConditionExpression` on `expiresAtIso > :now`, comparing two
21+
`Instant.toString()` strings. That output is variable-precision (the
22+
fractional-seconds field is dropped when zero), so a bytewise `>` sorts
23+
`…:00Z` *after* `…:00.000001Z` and could treat a just-expired token as still
24+
fresh. The condition now compares the numeric epoch-second `ttl` attribute
25+
(`#ttl > :nowEpoch`). The authoritative expiry check in `RefreshTokenService`
26+
was already correct, so the exposure was limited to a sub-second
27+
fail-open in the database-level backstop for the rotate TOCTOU window.
28+
- **Stateful JWT validation rejects `jti`-less tokens instead of skipping
29+
revocation.** When a non-noop `AccessTokenStore` is bound,
30+
`PkAuthJwtValidator` previously guarded the store lookup with `jti != null`,
31+
so a validly-signed, in-issuer, in-audience, unexpired token carrying no
32+
`jti` bypassed the `exists` revocation gate entirely. The validator now
33+
returns `MissingClaim("jti")` for a `jti`-less token in stateful mode. Tokens
34+
minted by `PkAuthJwtIssuer` always carry a `jti`, so this never affected the
35+
library's own tokens; it closes the gap for any other token signed with the
36+
same keyset.
37+
- **Corrected the `MagicLinkService.startLogin` timing-side-channel
38+
contract (documentation).** The javadoc claimed the method defeats timing
39+
side-channels, but the not-found path returns before JWT issuance and email
40+
dispatch, so response latency still distinguishes known from unknown
41+
usernames. The contract now documents the result-shape guarantee only and
42+
points hosts at uniform-latency / rate-limiting mitigations. No behavioural
43+
change.
44+
45+
## [1.1.0] — 2026-06-02
46+
1447
### Added
1548

1649
- `TokenTtlPolicy` SPI in `pk-auth-jwt` for per-audience JWT access-token TTL
@@ -112,5 +145,7 @@ tags.
112145
First stable release. Captures the surface produced by the 0.x development
113146
series; see `git log` for the full history.
114147

115-
[Unreleased]: https://github.com/codeheadsystems/pk-auth/compare/v1.0.0...HEAD
148+
[Unreleased]: https://github.com/codeheadsystems/pk-auth/compare/v1.2.0...HEAD
149+
[1.2.0]: https://github.com/codeheadsystems/pk-auth/compare/v1.1.0...v1.2.0
150+
[1.1.0]: https://github.com/codeheadsystems/pk-auth/compare/v1.0.0...v1.1.0
116151
[1.0.0]: https://github.com/codeheadsystems/pk-auth/releases/tag/v1.0.0

gradle.properties

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,4 @@ org.gradle.jvmargs=-Xmx2g \
2121

2222
# Project identity
2323
group=com.codeheadsystems
24-
version=1.1.0-SNAPSHOT
24+
version=1.2.0-SNAPSHOT

pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/PkAuthJwtValidator.java

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ public final class PkAuthJwtValidator {
4545
private final ClockProvider clockProvider;
4646
private final RevocationCheck revocationCheck;
4747
private final AccessTokenStore accessTokenStore;
48+
private final boolean statefulStore;
4849

4950
/**
5051
* Constructs a validator with no-op revocation and no-op access-token store (tokens are valid
@@ -88,6 +89,7 @@ public PkAuthJwtValidator(
8889
this.clockProvider = Objects.requireNonNull(clockProvider, "clockProvider");
8990
this.revocationCheck = Objects.requireNonNull(revocationCheck, "revocationCheck");
9091
this.accessTokenStore = Objects.requireNonNull(accessTokenStore, "accessTokenStore");
92+
this.statefulStore = accessTokenStore != NoopAccessTokenStore.INSTANCE;
9193
}
9294

9395
/** Verifies signature and standard claims, then reconstructs a {@link JwtClaims}. */
@@ -167,10 +169,18 @@ public JwtVerificationResult validate(String token) {
167169
if (revocationCheck.isRevoked(jti, subject)) {
168170
return new JwtVerificationResult.Revoked(jti, subject);
169171
}
170-
if (jti != null && !accessTokenStore.exists(jti)) {
171-
// Stateful mode: the token's jti is not in the store (logout / admin-revoke / never
172-
// recorded). The noop store always returns true so stateless deployments skip this branch.
173-
return new JwtVerificationResult.Revoked(jti, subject);
172+
if (statefulStore) {
173+
// Stateful mode: every token we issue carries a jti recorded in the store. A token without a
174+
// jti can't be checked against the store, so it can't be proven un-revoked — reject it rather
175+
// than letting it bypass the revocation gate entirely. (The noop store leaves statefulStore
176+
// false, so stateless deployments skip this branch and remain valid-until-exp.)
177+
if (jti == null) {
178+
return new JwtVerificationResult.MissingClaim("jti");
179+
}
180+
if (!accessTokenStore.exists(jti)) {
181+
// jti not in the store: logout / admin-revoke / never recorded.
182+
return new JwtVerificationResult.Revoked(jti, subject);
183+
}
174184
}
175185

176186
String methodClaim = stringClaim(body, PkAuthJwtIssuer.CLAIM_METHOD);

pk-auth-magic-link/src/main/java/com/codeheadsystems/pkauth/magiclink/MagicLinkService.java

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -225,13 +225,20 @@ public SendResult startEmailVerification(UserHandle user, String email) {
225225
/**
226226
* Sends a login email to the user with the supplied username.
227227
*
228-
* <p><strong>Privacy invariant:</strong> this method ALWAYS returns {@link SendResult.Sent},
229-
* regardless of whether the supplied username exists in the system. When no user is found the
230-
* method returns early (skipping JWT issuance and email dispatch) but returns the same {@code
231-
* Sent} shape as a successful send. This prevents account-enumeration via both result-shape
232-
* side-channels and timing side-channels that would otherwise reveal whether an account exists.
233-
* Callers MUST NOT rely on a {@link SendResult.UserNotFound} outcome from this method — that
234-
* variant is produced only by signup flows where confirming account non-existence is intentional.
228+
* <p><strong>Privacy invariant (result-shape only):</strong> this method ALWAYS returns {@link
229+
* SendResult.Sent}, regardless of whether the supplied username exists in the system. When no
230+
* user is found the method returns early (skipping JWT issuance and email dispatch) but returns
231+
* the same {@code Sent} shape as a successful send, so the response body never reveals whether an
232+
* account exists. Callers MUST NOT rely on a {@link SendResult.UserNotFound} outcome from this
233+
* method — that variant is produced only by signup flows where confirming account non-existence
234+
* is intentional.
235+
*
236+
* <p><strong>This method is NOT constant-time.</strong> The not-found path returns before JWT
237+
* issuance and the (typically blocking) email dispatch, so a known username incurs measurably
238+
* more latency than an unknown one; an attacker who can time responses can still enumerate
239+
* accounts. Equalising this is impractical at the library layer because SMTP/transport latency
240+
* dominates and varies — hosts that need timing-side-channel resistance should front this with a
241+
* uniform-latency wrapper or rate-limit and monitor for enumeration probing.
235242
*
236243
* @since 0.9.1
237244
*/

pk-auth-persistence-dynamodb/src/main/java/com/codeheadsystems/pkauth/persistence/dynamodb/DynamoDbRefreshTokenRepository.java

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,12 +138,18 @@ public boolean rotateAtomically(
138138

139139
// Transact: conditional update on parent primary (still fresh) + put successor items.
140140
// ConditionExpression — fresh iff used_at, revoked_at unset and expires_at > now.
141+
// Expiry compares the numeric epoch-second `ttl` attribute (== expiresAt.epochSecond),
142+
// NOT the ISO string: Instant.toString() is variable-precision (it drops the
143+
// fractional-seconds field when zero), so a lexicographic ">" sorts "...:00Z" after
144+
// "...:00.000001Z" and would treat an expired token as still fresh.
141145
Expression freshness =
142146
Expression.builder()
143147
.expression(
144148
"attribute_not_exists(usedAtIso) AND attribute_not_exists(revokedAtIso)"
145-
+ " AND expiresAtIso > :now")
146-
.putExpressionValue(":now", AttributeValue.fromS(nowIso))
149+
+ " AND #ttl > :nowEpoch")
150+
.putExpressionName("#ttl", "ttl")
151+
.putExpressionValue(
152+
":nowEpoch", AttributeValue.fromN(Long.toString(now.getEpochSecond())))
147153
.build();
148154

149155
try {

0 commit comments

Comments
 (0)