Skip to content

Commit 2128e54

Browse files
glasstigerclaude
andcommitted
Tighten token-store hardening and hygiene
Sanitize the warnPersistence detail. An IO error from the store can carry the operator-supplied store path (questdb.client.oidc.token.store.dir or user.home), which could itself hold terminal-spoofing characters, so route cause.getMessage() through the null-safe sanitizeForDisplay before printing to System.err - matching how every other untrusted display string is sanitized. Reap orphan temp files. A crash between createTempFile and the atomic rename leaves a <hash>*.tmp holding a valid-at-the-time refresh token; nothing ever steals it, so they would accumulate across crashes. save() now best-effort sweeps <hash>*.tmp older than lockStaleMillis, leaving a concurrent writer's fresh temp (recent mtime) untouched - so the random per-writer suffix that keeps concurrent saves from colliding stays. Chmod the store directory only on drift. ensureDirectory re-tightens a pre-existing directory on every save and every inLock; read the permissions first and only setPosixFilePermissions when they actually differ, dropping a redundant write syscall in the common case while keeping the re-tighten defense and the non-POSIX fallback. Compare the schema version as a long. The parser narrowed it to an int, so a tampered "v" of 1 + 2^32 truncated to SCHEMA_VERSION and passed the gate; keep the full long and compare as a long. Add tests for the temp-file sweep and the version-overflow reject; each was proven to fail with its fix reverted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 9be7c28 commit 2128e54

3 files changed

Lines changed: 82 additions & 9 deletions

File tree

core/src/main/java/io/questdb/client/cutlass/auth/FileTokenStore.java

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import java.nio.channels.FileChannel;
4141
import java.nio.charset.StandardCharsets;
4242
import java.nio.file.AtomicMoveNotSupportedException;
43+
import java.nio.file.DirectoryStream;
4344
import java.nio.file.FileAlreadyExistsException;
4445
import java.nio.file.Files;
4546
import java.nio.file.NoSuchFileException;
@@ -245,6 +246,7 @@ public void save(TokenStoreKey key, PersistedToken token) {
245246
byte[] content = serialize(key, token);
246247
try {
247248
ensureDirectory();
249+
sweepStaleTempFiles(key.hash());
248250
Path target = tokenFile(key);
249251
Path tmp = createTempFile(key.hash());
250252
boolean moved = false;
@@ -457,16 +459,20 @@ private static void releaseLock(Path lock, String nonce) {
457459

458460
private static void restrictToOwner(Path directory) {
459461
// best-effort: the at-rest protection of the plaintext token files is exactly these owner-only
460-
// directory permissions, so tighten a pre-existing directory rather than trust whatever it had. On a
461-
// non-POSIX filesystem (Windows) this is unsupported and falls back to the directory's existing ACL
462+
// directory permissions, so re-tighten a pre-existing directory another tool/umask left loose rather
463+
// than trust whatever it had. ensureDirectory runs this on every save and every inLock, so only chmod
464+
// on detected drift - skip the write syscall in the common case where the permissions already match. On
465+
// a non-POSIX filesystem (Windows) this is unsupported and falls back to the directory's existing ACL
462466
// (owner-only hardening there, via AclFileAttributeView, is a separate follow-up)
463467
try {
464-
Files.setPosixFilePermissions(directory, DIR_PERMS);
468+
if (!DIR_PERMS.equals(Files.getPosixFilePermissions(directory))) {
469+
Files.setPosixFilePermissions(directory, DIR_PERMS);
470+
}
465471
} catch (UnsupportedOperationException e) {
466472
// non-POSIX FS (e.g. Windows): cannot enforce owner-only perms; keep the inherited ACL
467473
warnNoPosixPermsOnce();
468474
} catch (IOException ignore) {
469-
// the directory is not ours to chmod: keep the existing permissions
475+
// the directory is not ours to inspect/chmod: keep the existing permissions
470476
}
471477
}
472478

@@ -614,6 +620,28 @@ private Path lockFile(TokenStoreKey key) {
614620
return directory.resolve(key.hash() + ".lock");
615621
}
616622

623+
private void sweepStaleTempFiles(String hashPrefix) {
624+
// a crash between createTempFile and the atomic rename orphans a <hash>*.tmp holding a
625+
// valid-at-the-time refresh token; unlike the lock file nothing ever steals it, so it would accumulate
626+
// across crashes. Best-effort sweep on save: delete only temps older than the lock-staleness window, so
627+
// a temp a concurrent writer is actively using (its mtime is seconds old) is never removed. A separate
628+
// random suffix per writer keeps concurrent saves from colliding, which is why temps are not a fixed name
629+
try (DirectoryStream<Path> stream = Files.newDirectoryStream(directory, hashPrefix + "*.tmp")) {
630+
final long now = System.currentTimeMillis();
631+
for (Path tmp : stream) {
632+
try {
633+
if (now - Files.getLastModifiedTime(tmp).toMillis() > lockStaleMillis) {
634+
Files.deleteIfExists(tmp);
635+
}
636+
} catch (IOException ignore) {
637+
// best-effort; skip this entry and let a later sweep retry
638+
}
639+
}
640+
} catch (IOException ignore) {
641+
// best-effort; a sweep failure must never fail a save
642+
}
643+
}
644+
617645
private Path tokenFile(TokenStoreKey key) {
618646
return directory.resolve(key.hash() + ".json");
619647
}
@@ -643,7 +671,7 @@ private static final class TokenFileParser implements JsonParser {
643671
long expiresAtMillis;
644672
boolean groupsInToken;
645673
long tokenTtlMillis;
646-
int version;
674+
long version;
647675
private int depth;
648676
private int field = FIELD_NONE;
649677
private boolean malformed;
@@ -698,7 +726,9 @@ public void onEvent(int code, CharSequence tag, int position) {
698726
if (depth == 1) {
699727
switch (field) {
700728
case FIELD_VERSION:
701-
version = (int) parseLongOrZero(tag);
729+
// keep the full long: an over-32-bit value (e.g. 1 + 2^32) must not narrow to
730+
// SCHEMA_VERSION and slip through the schema gate, so compare it as a long
731+
version = parseLongOrZero(tag);
702732
break;
703733
case FIELD_CLIENT_ID:
704734
putValue(clientId, tag);

core/src/main/java/io/questdb/client/cutlass/auth/OidcDeviceAuth.java

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1499,9 +1499,11 @@ private boolean tryRefreshCoordinated() {
14991499
}
15001500

15011501
private void warnPersistence(String operation, Throwable cause) {
1502-
// best-effort persistence: report to System.err and carry on with the in-memory token. The store
1503-
// never puts token bytes in its messages, so this cannot leak the secret.
1504-
String detail = cause.getMessage();
1502+
// best-effort persistence: report to System.err and carry on with the in-memory token. The store never
1503+
// puts token bytes in its messages, but an IO error can carry the operator-supplied store path, which
1504+
// could itself hold terminal-spoofing characters - sanitize the detail before printing, as every other
1505+
// untrusted display string is sanitized (sanitizeForDisplay is null-safe).
1506+
String detail = sanitizeForDisplay(cause.getMessage());
15051507
System.err.println("questdb client: OIDC token store " + operation
15061508
+ " failed; continuing without persistence" + (detail != null ? " [" + detail + ']' : ""));
15071509
}

core/src/test/java/io/questdb/client/test/cutlass/auth/FileTokenStoreTest.java

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -709,6 +709,30 @@ public void testSpecialCharactersAndNullsRoundTrip() throws Exception {
709709
});
710710
}
711711

712+
@Test
713+
public void testStaleTempFilesAreSweptOnSave() throws Exception {
714+
assertMemoryLeak(() -> {
715+
Path dir = storeDir();
716+
Files.createDirectories(dir);
717+
// 1s staleness window so the test does not have to wait
718+
FileTokenStore store = new FileTokenStore(dir, 3_000, 1_000);
719+
TokenStoreKey key = sampleKey();
720+
// an orphan temp left by a crashed save (backdated past the staleness window) must be reaped on the
721+
// next save; a fresh temp (recent mtime - a concurrent writer's) must be left untouched
722+
Path staleTmp = dir.resolve(key.hash() + "stale.tmp");
723+
Files.createFile(staleTmp);
724+
Files.setLastModifiedTime(staleTmp, FileTime.fromMillis(System.currentTimeMillis() - 10_000));
725+
Path freshTmp = dir.resolve(key.hash() + "fresh.tmp");
726+
Files.createFile(freshTmp);
727+
728+
store.save(key, sampleToken("ACCESS-1", "REFRESH-1"));
729+
730+
Assert.assertFalse("a stale orphan temp must be swept on save", Files.exists(staleTmp));
731+
Assert.assertTrue("a fresh temp (a concurrent writer's) must not be swept", Files.exists(freshTmp));
732+
Assert.assertNotNull("the save must still succeed", store.load(key));
733+
});
734+
}
735+
712736
@Test
713737
public void testTokenStoreKeyRejectsNullRequiredFields() throws Exception {
714738
assertMemoryLeak(() -> {
@@ -757,6 +781,23 @@ public void testTruncatedJsonReturnsNull() throws Exception {
757781
});
758782
}
759783

784+
@Test
785+
public void testVersionOverflowReturnsNull() throws Exception {
786+
assertMemoryLeak(() -> {
787+
Path dir = storeDir();
788+
FileTokenStore store = new FileTokenStore(dir);
789+
TokenStoreKey key = sampleKey();
790+
// a tampered version that narrows to SCHEMA_VERSION when cast to int (1 + 2^32) must not pass the
791+
// schema gate: the parser keeps the version as a long and compares it as a long
792+
store.save(key, sampleToken("ACCESS-1", "REFRESH-1"));
793+
Assert.assertNotNull("the valid entry must load", store.load(key));
794+
byte[] valid = Files.readAllBytes(tokenFile(dir, key));
795+
String tampered = new String(valid, StandardCharsets.UTF_8).replace("\"v\":1", "\"v\":4294967297");
796+
Files.write(tokenFile(dir, key), tampered.getBytes(StandardCharsets.UTF_8));
797+
Assert.assertNull("a version that truncates to 1 as an int must be rejected", store.load(key));
798+
});
799+
}
800+
760801
private static TokenStoreKey sampleKey() {
761802
return new TokenStoreKey("questdb", "https://idp.example.com:443/token",
762803
"https://idp.example.com:443/device", "openid", null, false);

0 commit comments

Comments
 (0)