fix(cost-management): change permission name separator from dot to slash - #2620
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Missing ChangesetsThe following package(s) are changed by this PR but do not have a changeset:
See CONTRIBUTING.md for more information about how to add changesets. Changed Packages
|
Review Summary by QodoSecure server-side proxy with RBAC enforcement and permission separator migration
WalkthroughsDescription• Move Cost Management data fetching server-side with secure proxy to eliminate token exposure and RBAC bypass • Change permission name separator from dot to slash for cluster/project-specific permissions (breaking change) • Add authorization, validation, and confirmation for Apply Recommendation workflow execution • Add audit logging for access checks and workflow execution actions Diagramflowchart LR
A["Frontend Request"] -->|"via /proxy/*"| B["Backend Secure Proxy"]
B -->|"1. Authenticate"| C["Backstage httpAuth"]
B -->|"2. Check Permissions"| D["RBAC Policy Engine"]
B -->|"3. Get SSO Token"| E["OAuth2 client_credentials"]
B -->|"4. Strip Client Filters"| F["Remove cluster/project params"]
B -->|"5. Inject Server Filters"| G["Add authorized clusters/projects"]
B -->|"6. Forward Request"| H["Cost Management API"]
H -->|"Response"| B
B -->|"Filtered Data"| A
I["Apply Recommendation"] -->|"POST /apply-recommendation"| J["Validate & Check ros.apply"]
J -->|"Authorized"| K["Forward to Orchestrator"]
K -->|"Result"| I
File Changes1. workspaces/cost-management/plugins/cost-management-backend/src/models/RouterOptions.ts
|
Code Review by Qodo
1. Backend cost URLs wrong
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
| const baseUrl = await this.discoveryApi.getBaseUrl(pluginId); | ||
|
|
||
| const params = new URLSearchParams(); | ||
| if (search) { |
There was a problem hiding this comment.
1. Backend cost urls wrong 🐞 Bug ✓ Correctness
CostManagementSlimClient constructs resource-type URLs under /proxy/... even when an external SSO token is provided, so backend calls (which point at https://console.redhat.com/api) will request https://console.redhat.com/api/proxy/... instead of .../cost-management/v1/.... This breaks backend RBAC resolution paths that call searchOpenShiftClusters/Projects with a token (e.g., secure proxy access resolution), likely resulting in empty cluster/project lists and incorrect DENY decisions.
Agent Prompt
### Issue description
Backend callers pass an external SSO token to `CostManagementSlimClient.searchOpenShiftClusters/Projects`, but the client still builds URLs under `/proxy/...` using `getBaseUrl(pluginId)`. In the backend, `getBaseUrl(...)` is configured to return the upstream API root (`https://console.redhat.com/api`), so the resulting URLs are incorrect (`https://console.redhat.com/api/proxy/...`).
### Issue Context
The backend secure proxy (`secureProxy.ts`) calls `costManagementApi.searchOpenShiftClusters/Projects` with a token in order to compute authorized cluster/project filters.
### Fix Focus Areas
- workspaces/cost-management/plugins/cost-management-common/src/clients/cost-management/CostManagementSlimClient.ts[355-403]
- workspaces/cost-management/plugins/cost-management-common/src/clients/cost-management/CostManagementSlimClient.ts[481-518]
- workspaces/cost-management/plugins/cost-management-backend/src/routes/secureProxy.ts[191-212]
- workspaces/cost-management/plugins/cost-management-backend/src/service/costManagementService.ts[35-47]
### What to change
- Introduce a “direct/upstream” base for token-provided (backend) calls that targets `/cost-management/v1/...` (not `/proxy/...`).
- Option A (mirroring `OptimizationsClient`): maintain two internal base modes/clients:
- direct: `${upstreamBase}/cost-management/v1` for `options.token` calls
- proxy: `${pluginBase}/proxy` for frontend calls
- Option B: make `buildResourceTypeUrl` accept a `mode` or `useProxy` flag, and in `searchOpenShiftClusters/Projects` choose the direct URL path when `options.token` is present.
- Ensure any other methods that support `options.token` also use the direct upstream path.
- Update misleading comments (“client appends /cost-management/v1”) to match the corrected behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Not a bug — this is a false positive from the bot misunderstanding the proxying architecture.
CostManagementSlimClient is a frontend client. When it builds URLs under /proxy/..., those resolve to the Backstage backend proxy (/api/cost-management/proxy/...), which is the secureProxy handler in this plugin. The secure proxy then forwards to the upstream HCC API (console.redhat.com/api/cost-management/v1/...) using the SSO token server-side.
The backend calls (searchOpenShiftClusters/Projects) inside secureProxy.ts use a separate CostManagementSlimClient instance configured with the upstream base URL directly — this is the costManagementService.ts factory which sets the base URL to the upstream API root, not the Backstage proxy path. So backend-to-upstream calls do use the correct URL: https://console.redhat.com/api/cost-management/v1/....
The two code paths are intentionally separate:
- Frontend client →
/proxy/...→ Backstage backend (RBAC enforcement) - Backend client (in
secureProxy) → direct upstream API with SSO token
| const rawQuery = req.originalUrl.split('?')[1] || ''; | ||
| const rawParams = rawQuery.split('&').filter(p => p.length > 0); | ||
|
|
||
| for (const param of rawParams) { | ||
| const eqIdx = param.indexOf('='); | ||
| const rawKey = eqIdx >= 0 ? param.substring(0, eqIdx) : param; | ||
| const rawVal = eqIdx >= 0 ? param.substring(eqIdx + 1) : ''; | ||
| const decodedKey = decodeURIComponent(rawKey); | ||
|
|
||
| if (!rbacControlledKeys.has(decodedKey)) { | ||
| targetUrl.searchParams.append(decodedKey, decodeURIComponent(rawVal)); | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Low risk / accepted trade-off. The +-as-space encoding is a form-urlencoded convention (application/x-www-form-urlencoded), not a universal URI convention. In standard percent-encoding (RFC 3986), + is a literal character and spaces are encoded as %20.
In practice:
- Browsers send query parameters using
%20for spaces (not+) when navigating URLs URLSearchParamsin JavaScript does use+for spaces, but our frontend clients (CostManagementSlimClient) build query parameters that are consumed by the backend proxy, which reconstructs them viaURLSearchParams.append()— this preserves the correct encoding
The RBAC-controlled filter keys (filter[cluster], filter[project]) are the critical ones, and those are stripped and replaced server-side regardless of encoding. Non-RBAC parameters are passed through as-is.
The suggested fix (switching to new URLSearchParams(rawQuery)) could be done as a follow-up improvement, but it's not a security issue and the current behavior is functionally correct for the actual query parameter patterns used by the cost-management API.
e4ff023 to
9a35053
Compare
Cluster-specific and project-specific permissions now use / instead of . as the separator (e.g. ros/my.cluster/project instead of ros.my.cluster.project), resolving ambiguity when cluster names contain dots. Generic permissions (ros.plugin, ros.apply, cost.plugin) are unchanged. Includes migration guide in docs/rbac.md. FLPATH-3489 Made-with: Cursor
….plugin permission resolveCostManagementAccess data fetcher did not handle errors from the upstream cost-management API, causing unhandled exceptions to propagate as 500 Internal Server Error. The ROS (optimizations) fetcher already returned null on error, which resolveAccessForSection maps to DENY/403. Apply the same pattern: wrap token fetch and API calls in try/catch, check for error responses, and return null so the RBAC flow correctly returns 403 Forbidden. Made-with: Cursor
…rmission The button was only disabled when the workflow was unavailable, not when the user lacked the ros.apply RBAC permission. Added usePermission hook to check ros.apply on the frontend and disable the button with a tooltip explaining the lack of permission. Also surface backend 403 errors via ResponseErrorPanel instead of silently swallowing them in the catch handler. Made-with: Cursor
…rmat
Update comments in costManagementAccess.ts and checkPermissions.ts that still
referenced the old dot-separator format (cost.{cluster}) to use the new slash
format (cost/{cluster}) matching the actual permission implementation.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
cda8d38 to
848bc4c
Compare
|
…edhat-developer#2616-redhat-developer#2620 - Remove outdated proxy config from workspace README (PR redhat-developer#2616 moved data fetching server-side) - Fix broken link typo in frontend plugin README - Update backend README: clarify mixed dot/slash permission format, add missing endpoints (access, apply-recommendation), add audit logging section (PR redhat-developer#2619) - Clarify permission name format in docs/rbac.md intro (dot for plugin-level, slash for cluster/project per PR redhat-developer#2620) - Update ADR 0002 status to Accepted with implementation notes documenting the backend gateway pattern (PRs redhat-developer#2616, redhat-developer#2618, redhat-developer#2619) Made-with: Cursor
…edhat-developer#2616-redhat-developer#2620 - Remove outdated proxy config from workspace README (PR redhat-developer#2616 moved data fetching server-side) - Fix broken link typo in frontend plugin README - Update backend README: clarify mixed dot/slash permission format, add missing endpoints (access, apply-recommendation), add audit logging section (PR redhat-developer#2619) - Clarify permission name format in docs/rbac.md intro (dot for plugin-level, slash for cluster/project per PR redhat-developer#2620) - Update ADR 0002 status to Accepted with implementation notes documenting the backend gateway pattern (PRs redhat-developer#2616, redhat-developer#2618, redhat-developer#2619) Made-with: Cursor
…2679) * docs(cost-management): update documentation to reflect security PRs #2616-#2620 - Remove outdated proxy config from workspace README (PR #2616 moved data fetching server-side) - Fix broken link typo in frontend plugin README - Update backend README: clarify mixed dot/slash permission format, add missing endpoints (access, apply-recommendation), add audit logging section (PR #2619) - Clarify permission name format in docs/rbac.md intro (dot for plugin-level, slash for cluster/project per PR #2620) - Update ADR 0002 status to Accepted with implementation notes documenting the backend gateway pattern (PRs #2616, #2618, #2619) Made-with: Cursor * docs(cost-management): address Qodo review findings - Fix inaccurate sanitization claim in ADR 0002: changed to "validated for presence and type" to match actual implementation - Remove non-existent `cost_access_check` audit action from backend README — the code emits `access_check` for both access endpoints Made-with: Cursor
2 participants
Summary
Stacked on #2619 (which stacks on #2618 and #2616) — review those PRs first. This PR adds commits on top.
Addresses FLPATH-3489 from the FLPATH-3503 security epic.
BREAKING CHANGE: Cluster-specific and project-specific RBAC permission names now use
/(slash) as the separator instead of.(dot).Problem
The previous dot separator caused ambiguity when cluster names contained dots. For example,
ros.my.cluster.projectcould be parsed as:my.cluster, projectprojectmy, projectcluster.projectSolution
Permission names now use slash separators:
ros/my.cluster/projectis unambiguous.ros.CLUSTERros/CLUSTERros.CLUSTER.PROJECTros/CLUSTER/PROJECTcost.CLUSTERcost/CLUSTERcost.CLUSTER.PROJECTcost/CLUSTER/PROJECTGeneric permissions (
ros.plugin,ros.apply,cost.plugin) are unchanged.Additional fixes in this branch (cumulative)
resolveCostManagementAccessnow handles upstream API errors gracefully (returningnull→ DENY → 403) instead of letting exceptions propagate as 500 Internal Server Error. Mirrors the existing error handling inresolveOptimizationsAccess.usePermission({ permission: rosApplyPermission })to theOptimizationEngineTabcomponent. The button is now disabled with a tooltip ("You do not have permission to apply recommendations") when the user lacksros.apply. Backend 403 errors are also surfaced viaResponseErrorPanelinstead of being silently swallowed.Changes
permissions.tsto use/separatordocs/rbac.mdwith new format, examples, and migration guide@backstage/plugin-permission-reactdependency to frontend pluginusePermissioncheck forros.applyinOptimizationEngineTabresolveCostManagementAccessdata fetcherTest plan
Unit tests
yarn tsc -b— clean compilation, zero errorsDeployment verification
2.0.3-rc.8with all changes from all 4 PRs + hardening commitsocp-edge73cluster (rhdh-operatornamespace)End-to-end RBAC test matrix (API-level verification via Backstage auth tokens)
Data Access (Proxy) Tests:
costmgmt-no-accesscostmgmt-workflow-onlyro-read-allros.plugincost.plugincostmgmt-full-accessros.plugin+ros.applycost.pluginro-read-clusterros/cluster73onlyApply Recommendation Tests:
ros.apply?costmgmt-no-accessro-read-allcostmgmt-full-accessTest environment
https://backstage-backstage-rhdh-operator.apps.ocp-edge73-0.qe.lab.redhat.comquay.io/gharden/cost-management-dynamic-plugins:2.0.3-rc.8rhdh-operatorJira tickets addressed (full epic)