fix(apps): exclude env-var overrides from app secrets export (#36186)

[wezell]

Summary

Fixes #36186.

PR #35860 (#35859) made AppsAPIImpl.getSecrets() overlay env-var tiers onto resolved AppSecrets. The export path built the file from getSecrets(), so a defined env-var override either corrupted the export (a locked tier-1 secret has its value in envVarValue with a null stored value) or leaked the env value into the export file.

Fix

collectSecretsForExport now reads the persisted secrets blob directly via readStoredSecrets() instead of getSecrets():

• env values can never enter an export;
• a stored value masked by an env override still exports its real stored value;
• apps provisioned purely from env (no stored blob — appKeysByHost() lists env-backed apps) are skipped, so export-all keeps working in env-provisioned deployments; the existing empty-result guard still errors when nothing remains.

Admin + license are already enforced at the exportSecrets() entry point, so bypassing getSecrets() does not weaken access control.

Tests (integration — AppsAPIImplTest, in MainSuite3a)

• Test_Export_Excludes_Env_Overridden_Secret (success): stored p1 masked by a host env override → export succeeds; round-trip restores the stored value (not the env value); nothing restored is fromEnv.
• Test_Export_Env_Only_App_Has_Nothing_To_Export (failure): app provisioned only from env → exportSecrets throws IllegalArgumentException instead of leaking the env value.

Verification

Core installs; integration module test-compiles.

🤖 Generated with Claude Code

This PR fixes: #36186

[github-actions]

🤖 Bedrock Review — deepseek.v3.2

[🟡 Medium] dotCMS/src/main/java/com/dotcms/security/apps/AppsAPIImpl.java:989 — The readStoredSecrets method is called, but its signature and behavior are not shown in the diff. If this method does not exist or has been changed incorrectly, it could cause a compilation error or runtime failure.

---

_{Run: #27635357262 · tokens: in: 2890 · out: 79 · total: 2969}

[claude]

Claude finished @wezell's task in 1m 14s —— View job

---

Rollback Safety Analysis

• Read rollback-unsafe categories reference
• Get full PR diff
• Analyze diff against unsafe categories
• Apply appropriate label

---

Result: ✅ Safe To Rollback

Label applied: AI: Safe To Rollback

Analysis

The diff touches exactly two files:

File	Change type
dotCMS/src/main/java/com/dotcms/security/apps/AppsAPIImpl.java	Logic change in collectSecretsForExport (~20 lines changed)
dotcms-integration/src/test/java/com/dotcms/security/apps/AppsAPIImplTest.java	New integration tests only (+114 lines)

Checked against every category in the rollback-unsafe reference:

Category	Verdict
C-1 Structural data model change	✅ No DB schema touched
C-2 Elasticsearch mapping change	✅ No ES mapping changes
C-3 Content JSON model version bump	✅ ImmutableContentlet/CURRENT_MODEL_VERSION untouched
C-4 DROP TABLE / DROP COLUMN	✅ No DDL statements
H-1 One-way data migration	✅ No data migration or backfill
H-2 RENAME TABLE / COLUMN	✅ None
H-3 PK restructuring	✅ None
H-4 New ContentType field type	✅ None
H-5 Binary storage provider change	✅ None
H-6 DROP PROCEDURE / FUNCTION	✅ None
H-7 NOT NULL column without default	✅ None
H-8 VTL viewtool contract change	✅ No viewtool methods modified
M-1 Non-broadening column type change	✅ None
M-2 Push publishing bundle format	✅ None
M-3 REST/GraphQL API contract change	✅ No REST endpoint signatures changed
M-4 OSGi public interface change	✅ No exported OSGi interfaces modified

The change is purely a runtime read-path fix inside collectSecretsForExport: it replaces getSecrets() (which applies env-var overlays) with readStoredSecrets() (raw blob). There are no database migrations, no Elasticsearch changes, no API contract mutations, and no storage model alterations. Rolling back to N-1 is safe — the previous getSecrets()-based export logic is restored and no persistent data was altered by N.

[wezell]

Thanks for the review. Evaluated both findings:

[🟠 High] readStoredSecrets NPE — false positive. readStoredSecrets cannot return null: it returns Map.of() when the blob is absent (AppsAPIImpl.java:320), otherwise AppSecrets.getSecrets(), whose backing map is constructor-guaranteed non-null (AppSecrets.java:25 coerces a null secrets to new HashMap<>()). The existing callers in the same class (resolveSecrets at lines 254/272) already use the result without a null check for this reason. No change needed.

[🟡 Medium] Test mutates global Config. Acknowledged. This is the established pattern for env-override tests in this codebase (e.g. AppsAPIImplHostEnvProvisioningTest uses the same Config.setProperty(envVarName(), …) + null-reset approach), and the override is reset in a finally so it's cleaned up even on exception. Keeping it consistent with the existing convention.

[wezell]

Re the re-run's [🟡 Medium] on AppsAPIImpl.java:989: readStoredSecrets(String) is a pre-existing private method in this same class (AppsAPIImpl.java:317), unchanged by this PR — it's just not shown in the diff context. Verified: dotcms-core installs and dotcms-integration test-compiles cleanly, so there's no compilation/runtime concern here. (Same line as the earlier finding; non-null behavior already addressed above.)