Stop exposing login AES secrets to anonymous callers by Aias00 · Pull Request #6323 · apache/shenyu

Aias00 · 2026-04-13T09:08:45Z

Summary

sanitize /platform/secretInfo so it no longer returns the configured AES login key material
fall back to plain-text password handling when AES decryption is unavailable or fails
add regression coverage for the secret bootstrap response and login fallback path

Test Plan

mvn -pl shenyu-admin -Dtest=SecretServiceTest,DashboardUserServiceTest,PlatformControllerTest test

The login page was fetching /platform/secretInfo anonymously and receiving the long-lived AES key material used for password encryption. This change sanitizes the endpoint output so no real key/IV is returned and adds a server-side fallback to accept plain-text passwords when the client no longer encrypts before login. Constraint: Preserve compatibility with the existing bundled login page without rebuilding the frontend assets Rejected: Remove the endpoint or require auth only | would leave the current login bundle unable to encrypt and cause login failures without a backend fallback Confidence: medium Scope-risk: moderate Reversibility: clean Directive: Treat any future login bootstrap material as public-only data; never return reusable symmetric secrets to the client Tested: mvn -pl shenyu-admin -Dtest=SecretServiceTest,DashboardUserServiceTest,PlatformControllerTest test Not-tested: End-to-end browser login flow against a running admin instance and non-TLS deployment behavior

Copilot

Pull request overview

This PR hardens shenyu-admin’s secret bootstrap and login flow by ensuring /platform/secretInfo no longer returns the configured AES login key material, and by allowing logins to succeed with plaintext passwords when AES decryption isn’t possible.

Changes:

Sanitize SecretServiceImpl.info() to return empty key/iv instead of configured values.
Add a decrypt failure fallback in DashboardUserServiceImpl.login() to treat the incoming password as plaintext.
Add regression tests covering secret sanitization and the plaintext login fallback path.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/SecretServiceImpl.java	Stops exposing configured AES key/IV by returning a sanitized secret payload.
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/DashboardUserServiceImpl.java	Adds try/catch around AES decrypt to fall back to plaintext when decrypt fails.
shenyu-admin/src/test/java/org/apache/shenyu/admin/service/SecretServiceTest.java	Verifies `info()` does not leak configured `key`/`iv`.
shenyu-admin/src/test/java/org/apache/shenyu/admin/service/DashboardUserServiceTest.java	Adds coverage that plaintext login succeeds when AES decrypt fails.
.gitignore	Ignores local `.worktrees/` directory.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-13T09:11:57Z

    public SecretServiceImpl(final SecretProperties secretProperties) {
-        this.secretProperties = secretProperties;
    }


The constructor now accepts SecretProperties but doesn’t store or use it, which is confusing and may trigger unused-parameter/static-analysis warnings. Either remove the constructor argument (and rely on a no-args constructor) or keep the field and explicitly document/implement why it’s injected even though info() always returns a sanitized payload.

Handled in 16b4fb8. The unused SecretProperties constructor argument was removed.

Copilot · 2026-04-13T09:11:57Z

+        SecretProperties sanitized = new SecretProperties();
+        sanitized.setKey("");
+        sanitized.setIv("");
+        return Base64.getEncoder().encodeToString(JsonUtils.toJson(sanitized).getBytes());


getBytes() uses the platform default charset, but the response is decoded as UTF-8 in tests/clients. Use StandardCharsets.UTF_8 when converting the JSON string to bytes to make the Base64 payload deterministic across environments.

Handled in 16b4fb8. The sanitized secret payload now uses StandardCharsets.UTF_8 when encoding the JSON bytes.

Copilot · 2026-04-13T09:11:58Z

+            try {
+                decryptPassword = AesUtils.cbcDecrypt(secretProperties.getKey(), secretProperties.getIv(), password);
+            } catch (Exception e) {
+                LOG.warn("AES login password decrypt failed, falling back to plain text password");
+                decryptPassword = password;


With the new “fallback to plaintext” behavior, decryption failures become part of normal control flow when the UI sends plaintext. AesUtils.cbcDecrypt currently logs at ERROR and throws on any failure, so successful plaintext logins will still emit ERROR logs (and you also log a WARN here). Consider switching to a non-logging/non-throwing decrypt attempt (or adjusting AesUtils logging) so plaintext fallback doesn’t spam error logs in production.

Handled in 92c2570. The fallback path no longer routes normal plain-text login through the error-logging decrypt helper; decryption is attempted only for values that look like ciphertext and uses a quiet local helper.

Copilot · 2026-04-13T09:11:58Z

+            try {
+                decryptPassword = AesUtils.cbcDecrypt(secretProperties.getKey(), secretProperties.getIv(), password);
+            } catch (Exception e) {
+                LOG.warn("AES login password decrypt failed, falling back to plain text password");


The WARN log drops the exception, which makes it harder to diagnose real decrypt issues (wrong key/IV, provider problems, etc.). If you keep this log, consider including the exception (..., e) or at least logging e.getMessage() (ensuring no sensitive data is included) so operators have actionable diagnostics.

Suggested change

LOG.warn("AES login password decrypt failed, falling back to plain text password");

LOG.warn("AES login password decrypt failed, falling back to plain text password", e);

Handled in 92c2570. The fallback path now uses a quiet local decrypt helper and logs decrypt details only at DEBUG level instead of warning on the normal plain-text fallback path.

This follow-up removes an unused constructor dependency, fixes the secret payload encoding charset explicitly, and narrows AES decrypt attempts to inputs that look like ciphertext so normal plain-text login fallback no longer produces avoidable decrypt failures. Constraint: Maintain compatibility with the current login bundle while reducing operational noise from the fallback path Confidence: high Scope-risk: moderate Reversibility: clean Directive: Avoid using exception-driven control flow for common login paths when ciphertext can be identified up front Tested: mvn -pl shenyu-admin -Dtest=SecretServiceTest,DashboardUserServiceTest,PlatformControllerTest test Not-tested: End-to-end browser login against a running admin instance

This follow-up removes the exception-driven decrypt path from normal plain-text login fallback and switches secret bootstrap encoding to an explicit UTF-8 byte conversion while keeping the sanitized response. Constraint: Preserve compatibility with the current bundled login page while avoiding production error-log spam on valid fallback flows Confidence: high Scope-risk: narrow Reversibility: clean Directive: Do not route expected login fallback behavior through utilities that log hard failures by default Tested: mvn -pl shenyu-admin -Dtest=SecretServiceTest,DashboardUserServiceTest,PlatformControllerTest test Not-tested: End-to-end browser login against a running admin instance

This follow-up names the AES block size and the sanitized secret placeholder in production code, and replaces repeated literal key/iv and credential strings in the related tests with explicit constants. Constraint: Keep the cleanup local to the active login-secret fix to avoid unrelated churn across other PR branches Confidence: high Scope-risk: narrow Reversibility: clean Directive: Prefer named constants for repeated protocol/security values even in tests when they carry semantic meaning Tested: mvn -pl shenyu-admin -Dtest=SecretServiceTest,DashboardUserServiceTest,PlatformControllerTest test Not-tested: Full shenyu-admin test suite

moremind · 2026-04-14T11:36:47Z

+        if (StringUtils.isNotBlank(secretProperties.getKey()) && StringUtils.isNotBlank(secretProperties.getIv())
+                && isPotentialEncryptedPassword(password)) {
+            cbcDecryptPassword = tryDecryptPassword(password)
+                    .orElse(password);


orElse(password)?

Handled in 748c10282 and carried forward in the current PR head 17aebed. The login flow now resolves the password through a dedicated helper instead of leaving the Optional.orElse(password) chain inline in the main login method.

moremind · 2026-04-14T11:37:03Z

    @Override
    public String info() {
-        return Base64.getEncoder().encodeToString(JsonUtils.toJson(secretProperties).getBytes());
+        org.apache.shenyu.admin.config.properties.SecretProperties sanitized =


remove the package

Handled in 748c10282 and carried forward in the current PR head 17aebed. SecretServiceImpl now imports SecretProperties normally and no longer uses the fully qualified type name inline.

This follow-up removes the fully qualified SecretProperties usage in SecretServiceImpl and factors the login password normalization into a named helper so the plain-text fallback path reads clearly in the main login flow. Constraint: Keep the update local to the existing login-secret fix without changing the external API shape Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep security-sensitive fallback logic explicit and readable; avoid hiding behavior in dense Optional chains Tested: mvn -pl shenyu-admin -Dtest=SecretServiceTest,DashboardUserServiceTest,PlatformControllerTest test Not-tested: End-to-end browser login against a running admin instance

Aias00 added 3 commits March 31, 2026 19:18

goalx: snapshot before shenyu-analysis

49174a4

Merge branch 'master' of https://github.com/apache/shenyu

da384e7

Copilot AI review requested due to automatic review settings April 13, 2026 09:08

Copilot started reviewing on behalf of Aias00 April 13, 2026 09:09 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Aias00 added 3 commits April 13, 2026 17:19

moremind requested changes Apr 14, 2026

View reviewed changes

Aias00 added 6 commits April 14, 2026 20:45

Merge branch 'master' into codex/fix-platform-secretinfo-exposure

19eb7f2

Merge branch 'master' into codex/fix-platform-secretinfo-exposure

84aa6a5

Merge branch 'master' into codex/fix-platform-secretinfo-exposure

3d903dc

Merge branch 'master' into codex/fix-platform-secretinfo-exposure

b0f5adf

Merge branch 'master' into codex/fix-platform-secretinfo-exposure

0b4972f

moremind approved these changes Apr 15, 2026

View reviewed changes

moremind added about pom about pom change type: bug Something isn't working and removed about pom about pom change labels Apr 15, 2026

moremind added this to the 2.7.1 milestone Apr 15, 2026

moremind merged commit 31f0a4d into apache:master Apr 15, 2026
41 checks passed

	LOG.warn("AES login password decrypt failed, falling back to plain text password");
	LOG.warn("AES login password decrypt failed, falling back to plain text password", e);

Conversation

Aias00 commented Apr 13, 2026

Summary

Test Plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants