[fix] Add missing permission annotations to /appAuth/updateSk and /sandbox/proxyGateway#6388
Open
Aias00 wants to merge 15 commits into
Open
[fix] Add missing permission annotations to /appAuth/updateSk and /sandbox/proxyGateway#6388Aias00 wants to merge 15 commits into
Aias00 wants to merge 15 commits into
Conversation
The merge brings in upstream workflow hardening while preserving this branch's existing mvnd-based build flow and k8s test changes. Conflicts were limited to Java setup action selection and the new integrated-test Docker image pre-pull step, so the resolution keeps the retrying local setup-java wrapper and retains the pre-pull retries. Constraint: Merge was already in progress against origin/master at 86e544b Rejected: Keep direct actions/setup-java@v4 | would drop upstream retry behavior for transient setup-java failures Confidence: high Scope-risk: moderate Directive: Keep workflow Java setup calls on ./actions/setup-java-with-retry unless upstream removes the retry action Tested: No conflict markers remain in conflicted workflows; Ruby YAML parse for the three edited workflows; git diff --check for the three edited workflows Not-tested: Full GitHub Actions execution; full repository build/test suite
…ndbox/proxyGateway
- Add @RequiresPermissions("system:authen:edit") to AppAuthController.updateSk()
This endpoint was the only one in the controller without a permission check,
allowing any authenticated user to rotate arbitrary appAuth secrets.
- Change /appAuth/updateSk from GET to POST to prevent appSecret from
appearing in URLs, browser history, and server access logs.
- Add @RequiresPermissions("system:authen:list") to SandboxController.proxyGateway()
This endpoint generates server-side signed requests using stored appSecrets.
Without permission checks, any authenticated user could abuse it to forge
signed requests after compromising an appKey via the updateSk vulnerability.
These fixes address an authorization bypass where a low-privileged dashboard
user could chain updateSk + proxyGateway to impersonate arbitrary application
identities and send authenticated requests to allowlisted internal services.
Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR fixes authorization bypass vulnerabilities by adding missing permission checks to two admin endpoints and switching a sensitive secret-rotation endpoint from GET to POST.
Changes:
- Added
@RequiresPermissionsto/appAuth/updateSkand/sandbox/proxyGateway. - Changed
/appAuth/updateSkmapping fromGETtoPOSTand updated the related controller test. - Updated HTTP debug request docs and refreshed distribution LICENSE dependency listings.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| shenyu-admin/src/main/java/org/apache/shenyu/admin/controller/AppAuthController.java | Secures updateSk with permissions and changes it to POST to reduce secret exposure risk. |
| shenyu-admin/src/main/java/org/apache/shenyu/admin/controller/SandboxController.java | Adds permission gating to proxyGateway to prevent unauthorized signed request forwarding. |
| shenyu-admin/src/test/java/org/apache/shenyu/admin/controller/AppAuthControllerTest.java | Updates testUpdateSk to POST to match controller change. |
| shenyu-admin/src/http/http-debug-app-auth-controller-api.http | Updates the debug request to POST (but still includes secret in URL). |
| shenyu-dist/shenyu-bootstrap-dist/src/main/release-docs/LICENSE | Updates third-party dependency license listings for the bootstrap distribution. |
| shenyu-dist/shenyu-admin-dist/src/main/release-docs/LICENSE | Updates third-party dependency license listings for the admin distribution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
64
to
67
| POST http://localhost:9095/appAuth/updateSk?appKey=123&appSecret=123 | ||
| Accept: application/json | ||
| Content-Type: application/json | ||
| X-Access-Token: eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJ1c2VyTmFtZSI6ImFkbWluIiwiZXhwIjoxNjQ4NjUwMDg2fQ.aDeChT_Ey6FwYDdzSkc9ZLBHd5v-LVUZ6BPcYqJCo-Y |
| jsonschema-generator 4.38.0: https://github.com/victools/jsonschema-generator, The Apache License, Version 2.0 | ||
| jsonschema-module-jackson 4.38.0: https://github.com/victools/jsonschema-generator, The Apache License, Version 2.0 | ||
| jsonschema-module-swagger-2 4.38.0: https://github.com/victools/jsonschema-generator, The Apache License, Version 2.0 | ||
| re2j 1.8: https://github.com/google/re2j, Go License |
Comment on lines
+52
to
+53
| @PostMapping(path = "/proxyGateway") | ||
| @RequiresPermissions("system:authen:list") |
…ermission, revert unrelated LICENSE changes - Change /appAuth/updateSk to accept UpdateSkDTO as @RequestBody instead of @RequestParam, preventing appSecret from appearing in URLs, browser history, and server access logs (Copilot review comment #1) - Change /sandbox/proxyGateway permission from system:authen:list to system:authen:modify — a write/signing endpoint should not be guarded by a read permission (Copilot review comment #3) - Revert unrelated LICENSE file changes that were accidentally included (Copilot review comment #2 — re2j license issue is pre-existing) Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
Author
Addressed Copilot Review CommentsComment #1 — appSecret still in URL query string ✅ FixedChanged Comment #2 — LICENSE file re2j license ✅ RevertedThe LICENSE file changes were unrelated to this security fix and accidentally included from a dirty worktree. They have been reverted from this PR. Comment #3 — system:authen:list is semantically mismatched ✅ FixedChanged the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix authorization bypass vulnerabilities in two admin dashboard endpoints that allow low-privileged users to rotate arbitrary appAuth secrets and forge signed requests.
Vulnerability Details
1.
/appAuth/updateSk— Missing permission annotation (High)AppAuthController.updateSk()was the only endpoint in the controller without a@RequiresPermissionsannotation. Every other endpoint (apply,findPageByQuery,detail,updateDetail,detailPath,updateDetailPath,batchDelete,batchEnabled,batchOpened,syncData) has proper permission checks.This allowed any authenticated dashboard user (including lowest-privilege
defaultrole) to modify theappSecretof any application.Additionally, the endpoint used
@GetMapping, causingappSecretto appear in URLs, browser history, and server access logs.2.
/sandbox/proxyGateway— Missing permission annotation (Medium-High)SandboxController.proxyGateway()had no@RequiresPermissionsannotation. It accepts caller-controlled request data, looks up the server-sideappSecretfor a givenappKey, generates a valid signature, and forwards the request to an allowlisted target.3. Attack Chain
When combined:
/appAuth/updateSk/sandbox/proxyGatewaywith the controlledappKeyand attacker-chosen request dataChanges
@RequiresPermissions("system:authen:edit")+ change@GetMappingto@PostMapping@RequiresPermissions("system:authen:list")testUpdateSkfrom GET to POSTTesting
Security Impact