Skip to content

Commit 50bc4a7

Browse files
authored
fix(storage): don't query the encryption key registry when storage is off (unblocks backend:dev:saas) (#7265)
# Description of Changes Fixes a startup failure introduced by #7155 and reported against `task backend:dev:saas`. **What goes wrong** `StorageProviderConfig.storageEncryptionState(...)` is created on every startup, in every profile. When `storage.encryption.enabled` is false — the default, and what SaaS ships — the `||` short-circuit evaluates `fileEncryptionKeyRepository.count()`, a live query against `file_encryption_keys`: ```java if (writeEnabled || fileEncryptionKeyRepository.count() > 0) { // <- always runs when the flag is off ``` That table only exists if `ddl-auto=update` managed to create it. When it cannot — permissions on a shared Supabase branch DB, concurrent DDL from several developers, schema ordering — **ddl-auto logs and continues**, so the situation used to be a warning nobody noticed. Now it is a query that throws during bean creation and takes the whole context down. Two things make this sting in SaaS specifically: `storage.enabled` is false there, so before this feature nothing ever touched the table; and `hibernate.default_schema=stirling_pdf` means the table has to exist in a schema the app may not be able to create in. There is a second exposure on the request path: `suppressDirectDownloads()` also counts (60s cached), so even a surviving boot could 500 on downloads. **Fix** - The boot probe runs only when `storage.enabled` is true, so a deployment that does not use storage never touches the table. - Registry reads are wrapped. The boot probe degrades to "no keys" rather than propagating; `suppressDirectDownloads()` **fails safe by suppressing** rather than issuing a presigned URL it cannot vouch for. Losing the direct-download fast path is recoverable; serving ciphertext is not. **Safety is unchanged, and that is the important part.** The decorator is still installed unconditionally, so any blob carrying the `SPDFEAR1` magic is still decrypted via lazy materialisation or fails loudly — the eager probe only ever bought *earlier* master-key verification. A node that can actually serve stored files has `storage.enabled` on by definition, which is exactly the node the drifted-node protection is for; that test now configures it that way, and a new test pins that the decorator remains installed even with storage off. **Tests** — storage-disabled never calls `count()`; an unreadable registry still boots *and* still suppresses direct downloads; the decorator stays installed with storage off; storage-enabled still probes. Full proprietary suite green apart from the pre-existing Windows-symlink `FolderIdentitiesTest` failure, which is environmental and unrelated. **Note on scope:** deliberately minimal so it can land quickly. The Aikido `findAll()` code-quality finding lives in #7173 only (`rotateMasterKey` does not exist on main), so it is fixed there rather than here. #7173 will be rebased once this merges. --- ## Checklist ### General - [x] I have read the [Contribution Guidelines](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/CONTRIBUTING.md) - [x] I have read the [Stirling-PDF Developer Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md) (if applicable) - [ ] I have read the [How to add new languages to Stirling-PDF](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md) (if applicable) - [x] I have performed a self-review of my own code - [x] My changes generate no new warnings ### Documentation - [ ] I have updated relevant docs on [Stirling-PDF's doc repo](https://github.com/Stirling-Tools/Stirling-Tools.github.io/blob/main/docs/) (if functionality has heavily changed) - [ ] I have read the section [Add New Translation Tags](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/devGuide/HowToAddNewLanguage.md#add-new-translation-tags) (for new translation tags only) ### Translations (if applicable) - [ ] I ran [`scripts/counter_translation.py`](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/docs/counter_translation.md) ### UI Changes (if applicable) - [ ] Screenshots or videos demonstrating the UI changes are attached (e.g., as comments or direct attachments in the PR) ### Testing (if applicable) - [x] I have run `task check` to verify linters, typechecks, and tests pass - [x] I have tested my changes locally. Refer to the [Testing Guide](https://github.com/Stirling-Tools/Stirling-PDF/blob/main/DeveloperGuide.md#7-testing) for more details.
1 parent c91d63f commit 50bc4a7

3 files changed

Lines changed: 77 additions & 6 deletions

File tree

app/proprietary/src/main/java/stirling/software/proprietary/storage/config/StorageProviderConfig.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,10 @@ public StorageEncryptionState storageEncryptionState(
7070
createKeyService(
7171
configuredFileEncryptionKey, clusterEnabled, requiresNew),
7272
fileEncryptionKeyRepository);
73-
if (writeEnabled || fileEncryptionKeyRepository.count() > 0) {
73+
// The registry table may not exist when storage is unused, so only probe if it is on.
74+
boolean probeForExistingKeys =
75+
!writeEnabled && applicationProperties.getStorage().isEnabled();
76+
if (writeEnabled || (probeForExistingKeys && state.encryptedContentMayExist())) {
7477
state.initialiseEagerly();
7578
log.info(
7679
"Storage encryption at rest active (writes {})",

app/proprietary/src/main/java/stirling/software/proprietary/storage/crypto/StorageEncryptionState.java

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package stirling.software.proprietary.storage.crypto;
22

33
import java.time.Duration;
4+
import java.util.Optional;
45
import java.util.function.Supplier;
56

67
import lombok.extern.slf4j.Slf4j;
@@ -104,9 +105,28 @@ public boolean suppressDirectDownloads() {
104105
&& now - keysExistCheckedAtNanos < KEYS_EXIST_CACHE_TTL.toNanos()) {
105106
return keysExistCached;
106107
}
107-
keysExistCached = keyRepository.count() > 0;
108+
keysExistCached = countKeys().orElse(true /* unreadable registry: fail safe */);
108109
keysExistCheckedAtNanos = now;
109110
keysExistEverChecked = true;
110111
return keysExistCached;
111112
}
113+
114+
/** True when key rows exist; empty when the registry could not be read. */
115+
private Optional<Boolean> countKeys() {
116+
try {
117+
return Optional.of(keyRepository.count() > 0);
118+
} catch (RuntimeException e) {
119+
log.warn(
120+
"Could not read the storage encryption key registry ({}). Treating direct"
121+
+ " downloads as unsafe; encrypted content is still decrypted on"
122+
+ " demand.",
123+
e.getMessage());
124+
return Optional.empty();
125+
}
126+
}
127+
128+
/** Whether key rows exist, for the decrypt-only startup path; false if unreadable. */
129+
public boolean encryptedContentMayExist() {
130+
return countKeys().orElse(false);
131+
}
112132
}

app/proprietary/src/test/java/stirling/software/proprietary/storage/config/StorageProviderConfigTest.java

Lines changed: 52 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,15 +4,19 @@
44
import static org.assertj.core.api.Assertions.assertThatCode;
55
import static org.assertj.core.api.Assertions.assertThatThrownBy;
66
import static org.mockito.ArgumentMatchers.anyString;
7+
import static org.mockito.Mockito.atLeastOnce;
78
import static org.mockito.Mockito.doAnswer;
89
import static org.mockito.Mockito.doNothing;
910
import static org.mockito.Mockito.mock;
11+
import static org.mockito.Mockito.never;
12+
import static org.mockito.Mockito.verify;
1013
import static org.mockito.Mockito.when;
1114

1215
import java.util.Base64;
1316
import java.util.Optional;
1417

1518
import org.junit.jupiter.api.Test;
19+
import org.springframework.dao.InvalidDataAccessResourceUsageException;
1620
import org.springframework.transaction.PlatformTransactionManager;
1721

1822
import stirling.software.common.model.ApplicationProperties;
@@ -24,6 +28,7 @@
2428
import stirling.software.proprietary.storage.crypto.InMemoryKeyRepo;
2529
import stirling.software.proprietary.storage.crypto.StorageEncryptionState;
2630
import stirling.software.proprietary.storage.provider.StorageProvider;
31+
import stirling.software.proprietary.storage.repository.FileEncryptionKeyRepository;
2732
import stirling.software.proprietary.storage.repository.StoredFileBlobRepository;
2833

2934
/**
@@ -66,16 +71,16 @@ void decorator_writeEnabled_requiresLicenceAndSuppressesDirectDownloads() {
6671

6772
@Test
6873
void decorator_flagOffButKeysExist_decryptOnlyModeStillMaterialises() throws Exception {
69-
// Simulate the drifted-node case: another node already created keys.
70-
StorageProviderConfig seedCfg = newConfig("local", License.SERVER, true);
74+
// Drifted node: keys created elsewhere; storage on, as it must be to serve files.
75+
StorageProviderConfig seedCfg = newConfig("local", License.SERVER, true, true);
7176
StorageEncryptionState seedState = seedCfg.storageEncryptionState(MASTER, false, txManager);
7277
Team team = new Team();
7378
team.setId(1L);
7479
User owner = new User();
7580
owner.setTeam(team);
7681
seedState.keyService().activeKekForOwner(owner);
7782

78-
StorageProviderConfig cfg = newConfig("local", License.NORMAL, false);
83+
StorageProviderConfig cfg = newConfig("local", License.NORMAL, false, true);
7984
StorageEncryptionState state = cfg.storageEncryptionState(MASTER, false, txManager);
8085

8186
assertThat(cfg.storageProvider(state, Optional.empty()))
@@ -104,6 +109,44 @@ void encryption_enabled_wrongLengthKey_failsStartup() {
104109
.hasMessageContaining("32 bytes");
105110
}
106111

112+
// ---- deployments that do not use storage (the SaaS shape) ---------------------------
113+
114+
@Test
115+
void storageDisabled_neverQueriesTheKeyRegistry() {
116+
StorageProviderConfig cfg = newConfig("local", License.NORMAL, false);
117+
StorageEncryptionState state = cfg.storageEncryptionState(MASTER, false, txManager);
118+
119+
assertThat(state.isWriteEnabled()).isFalse();
120+
verify(keyRepo.mock, never()).count();
121+
}
122+
123+
@Test
124+
void storageDisabled_decoratorStillInstalledSoCiphertextIsNeverServedRaw() {
125+
StorageProviderConfig cfg = newConfig("local", License.NORMAL, false);
126+
StorageEncryptionState state = cfg.storageEncryptionState(MASTER, false, txManager);
127+
128+
assertThat(cfg.storageProvider(state, Optional.empty()))
129+
.isInstanceOf(EncryptingStorageProvider.class);
130+
}
131+
132+
@Test
133+
void unreadableKeyRegistry_stillStartsAndFailsSafeOnDirectDownloads() {
134+
FileEncryptionKeyRepository broken = mock(FileEncryptionKeyRepository.class);
135+
when(broken.count())
136+
.thenThrow(new InvalidDataAccessResourceUsageException("no such table"));
137+
StorageEncryptionState state = new StorageEncryptionState(false, () -> null, broken);
138+
139+
assertThat(state.encryptedContentMayExist()).isFalse();
140+
assertThat(state.suppressDirectDownloads()).isTrue();
141+
}
142+
143+
@Test
144+
void storageEnabledWithoutEncryption_probesRegistry() {
145+
StorageProviderConfig cfg = newConfig("local", License.NORMAL, false, true);
146+
cfg.storageEncryptionState(MASTER, false, txManager);
147+
verify(keyRepo.mock, atLeastOnce()).count();
148+
}
149+
107150
// ---- backend licence gates (unchanged behaviour) ------------------------------------
108151

109152
@Test
@@ -161,9 +204,14 @@ void provider_unknown_throwsUnsupportedProvider_notLicense() {
161204

162205
private StorageProviderConfig newConfig(
163206
String provider, License license, boolean encryptionEnabled) {
207+
return newConfig(provider, license, encryptionEnabled, false);
208+
}
209+
210+
private StorageProviderConfig newConfig(
211+
String provider, License license, boolean encryptionEnabled, boolean storageEnabled) {
164212
ApplicationProperties props = new ApplicationProperties();
165213
props.getStorage().setProvider(provider);
166-
props.getStorage().setEnabled(false); // local-fallback path skips dir creation
214+
props.getStorage().setEnabled(storageEnabled);
167215
props.getStorage().getEncryption().setEnabled(encryptionEnabled);
168216
StoredFileBlobRepository repo = mock(StoredFileBlobRepository.class);
169217
LicenseKeyChecker checker = mock(LicenseKeyChecker.class);

0 commit comments

Comments
 (0)