fix(cost-management): add authorization, validation, and confirmation for Apply Recommendation#2618
Open
hardengl wants to merge 12 commits intoredhat-developer:mainfrom
Open
fix(cost-management): add authorization, validation, and confirmation for Apply Recommendation#2618hardengl wants to merge 12 commits intoredhat-developer:mainfrom
hardengl wants to merge 12 commits intoredhat-developer:mainfrom
Conversation
…en exposure and RBAC bypass Addresses security findings from the RHDH Cost Plugin threat model: - Removed /token endpoint that exposed SSO credentials to the browser - Added secure backend proxy (/api/cost-management/proxy/*) with server-side RBAC enforcement and internal SSO token management - Removed dangerously-allow-unauthenticated proxy configuration - Updated frontend clients to route through the secure backend proxy FLPATH-3487 Made-with: Cursor
…hitecture - Remove all references to dangerously-allow-unauthenticated proxy configuration - Document new secure backend proxy architecture and endpoints - Update static and dynamic plugin setup instructions - Add server-side RBAC enforcement explanation to rbac.md Made-with: Cursor
…C bypass - Reject proxyPath containing dot-segments (../) or leading slashes to prevent path traversal beyond /cost-management/v1/ - Post-construction check ensures resolved pathname stays under the base path - Decode query parameter keys before RBAC matching so percent-encoded variants (e.g. filter%5Bexact%3Acluster%5D) are correctly stripped - Replace regex-based stripping with Set-based decoded key lookup Made-with: Cursor
…uplication Extract resolveAccessForSection() to eliminate structural duplication between resolveOptimizationsAccess and resolveCostManagementAccess. Each section now provides only a data-fetching callback while the common authorize-cache-filter logic lives in one place. Made-with: Cursor
hardengl
requested review from
PreetiW,
asmasarw,
batzionb,
mareklibra and
ydayagi
as code owners
8 tasks
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
8 tasks
Review Summary by QodoAdd authorization, validation, and secure proxy for Apply Recommendation and Cost Management API access
WalkthroughsDescription• Add server-side authorization and input validation for Apply Recommendation workflow via new `POST /apply-recommendation` backend endpoint • Implement secure backend proxy (/api/cost-management/proxy/*) that enforces RBAC server-side and eliminates token exposure to browser • Register ros.apply permission and add confirmation dialog to prevent accidental workflow execution • Remove /token endpoint and dangerously-allow-unauthenticated proxy configuration; route all data fetching through secure backend Diagramflowchart LR
A["Frontend Request"] --> B["Backend Secure Proxy"]
B --> C["Authenticate via httpAuth"]
C --> D["Check RBAC Permissions"]
D --> E["Obtain SSO Token Server-Side"]
E --> F["Strip Client Filters"]
F --> G["Inject Server-Authorized Filters"]
G --> H["Forward to Cost Management API"]
H --> I["Return Filtered Response"]
J["Apply Recommendation Request"] --> K["POST /apply-recommendation"]
K --> L["Validate resourceType Allowlist"]
L --> M["Check ros.apply Permission"]
M --> N["Forward to Orchestrator"]
N --> O["Return Workflow Instance ID"]
File Changes1. workspaces/cost-management/plugins/cost-management-backend/src/routes/applyRecommendation.ts
|
Code Review by Qodo
1.
|
…eclare config schema Defense-in-depth: hardcode fetch method to 'GET' instead of passing req.method, preventing SSRF amplification if the route registration ever changes from router.get to router.all. Add costManagementProxyBaseUrl to config.d.ts with @visibility backend so the config key is schema-validated and documented. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… for Apply Recommendation - New ros.apply permission required to execute Apply Recommendation workflow - New POST /apply-recommendation backend endpoint validates resourceType against server-side allowlist and checks ros.apply permission before forwarding to Orchestrator - Workflow execution now routes through the cost-management backend instead of directly to the Orchestrator plugin, enabling server-side authorization and audit logging - Confirmation dialog prevents accidental workflow execution - Register costPluginPermissions in permission integration router (was previously missing) Fixes: FLPATH-3488, FLPATH-3492, FLPATH-3491 Made-with: Cursor
Run yarn build:api-reports:only to update report-clients.api.md and report.api.md with the correct auto-generated format. Made-with: Cursor
- Wrap decodeURIComponent in try/catch, return 400 on malformed encoding
- Safe JSON parse in applyRecommendation (check content-type first)
- Change router.all('/proxy/*') to router.get (proxy is read-only)
- Delete unused routes/token.ts
Made-with: Cursor
Construct a clean inputData object with only known fields before forwarding to the Orchestrator, preventing extra injected fields from passing through to the workflow execution. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
hardengl
force-pushed
the
fix/apply-recommendation-auth
branch
from
afd87b1 to
90ad7a3
Compare
13 tasks
Extract three helper functions from the secureProxy handler to bring cognitive complexity from 24 down below the SonarQube threshold of 15: - isPathTraversal: path validation guard - parseClientQueryParams: query string parsing with RBAC key stripping - injectRbacFilters: server-side RBAC filter injection Made-with: Cursor
hardengl
force-pushed
the
fix/apply-recommendation-auth
branch
from
058dcfe to
0321e9f
Compare
…indings Add missing OpenShift route, menu hierarchy, and explain why the proxy.endpoints config was removed (replaced by server-side secure proxy). Made-with: Cursor
…nt config Update the ConfigMap example to exactly match the working configuration deployed on ocp-edge73: icon reference name costManagementIcon and dot-separated menuItems keys (cost-management.optimizations). Made-with: Cursor
|
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
2 participants
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
Stacked on #2616 — review that PR first. This PR adds commits on top.
Addresses FLPATH-3488, FLPATH-3492, FLPATH-3491 from the FLPATH-3503 security epic.
Problem
The "Apply Recommendation" workflow was executed by the frontend calling the Orchestrator API directly. This had three security issues:
ros.applypermission gate.resourceTypefield passed to the workflow was not validated against an allowlist. A malicious user could inject arbitrary resource types.Solution
Created a new backend endpoint
POST /api/cost-management/apply-recommendationthat:httpAuthros.applypermission (returns 403 if denied)resourceTypeagainst a hardcoded allowlist (openshift,containers)inputDatafields (strips control characters, enforces field length limits)ros.apply— no Orchestrator permissions required)Frontend changes
ros.applypermission, usingusePermissionfrom@backstage/plugin-permission-reactResponseErrorPanelinstead of being silently swallowedNew permission:
ros.applyChanges
POST /apply-recommendationroute (applyRecommendation.ts)rosApplyPermissioninpermissions.tspermissionIntegrationRouterusePermissionhook inOptimizationEngineTab.tsxOptimizationsBreakdownPage.tsxOptimizationEngineTab.tsxTest plan
Unit tests
yarn tsc -b— clean compilation, zero errorsapplyRecommendation.test.ts— 6 tests covering:CI
Deployment verification
costmgmt-full-access(hasros.apply): HTTP 200 — workflow instance createdro-read-all(noros.apply): HTTP 403 — correctly deniedcostmgmt-no-access(no permissions): HTTP 403 — correctly deniedQodo bot findings (addressed)