fix(cost-management): move data fetching server-side to eliminate token exposure and RBAC bypass#2616
Open
hardengl wants to merge 8 commits intoredhat-developer:mainfrom
Open
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
hardengl
requested review from
PreetiW,
asmasarw,
batzionb,
mareklibra and
ydayagi
as code owners
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
Review Summary by QodoMove Cost Management data fetching server-side to eliminate token exposure and RBAC bypass
WalkthroughsDescription• Moved data fetching server-side via new secure backend proxy (/api/cost-management/proxy/*) to eliminate token exposure • Removed /token endpoint that exposed SSO credentials directly to browser clients • Enforced RBAC server-side before forwarding requests to Cost Management API • Removed dangerously-allow-unauthenticated proxy configuration from app-config • Updated frontend clients to route through secure backend proxy instead of direct API calls Diagramflowchart LR
Browser["Browser Client"]
BackendProxy["Secure Backend Proxy<br/>/api/cost-management/proxy/*"]
RBAC["RBAC Permission<br/>Check"]
TokenMgmt["Internal SSO Token<br/>Management"]
CostAPI["Cost Management API"]
Browser -->|Backstage Cookie| BackendProxy
BackendProxy -->|Check Permissions| RBAC
BackendProxy -->|Fetch Token| TokenMgmt
BackendProxy -->|Inject Filters<br/>+ Bearer Token| CostAPI
CostAPI -->|Response| BackendProxy
BackendProxy -->|Filtered Data| Browser
File Changes1. workspaces/cost-management/plugins/cost-management-backend/src/routes/secureProxy.ts
|
Code Review by Qodo
1.
|
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Comment on lines
+299
to
+320
| const rbacControlledPatterns = | ||
| access.filterStyle === 'ros' | ||
| ? [/^cluster=/m, /^project=/m] | ||
| : [/^filter\[exact:cluster\]=/m, /^filter\[exact:project\]=/m]; | ||
|
|
||
| const rawQuery = req.originalUrl.split('?')[1] || ''; | ||
| const rawParams = rawQuery.split('&').filter(p => p.length > 0); | ||
|
|
||
| for (const param of rawParams) { | ||
| const shouldStrip = rbacControlledPatterns.some(pattern => | ||
| pattern.test(param), | ||
| ); | ||
| if (!shouldStrip) { | ||
| const eqIdx = param.indexOf('='); | ||
| const key = eqIdx >= 0 ? param.substring(0, eqIdx) : param; | ||
| const val = eqIdx >= 0 ? param.substring(eqIdx + 1) : ''; | ||
| targetUrl.searchParams.append( | ||
| decodeURIComponent(key), | ||
| decodeURIComponent(val), | ||
| ); | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Author
There was a problem hiding this comment.
Good catch. Fixed in c092f79.
The approach was changed from regex-based matching on the raw (still percent-encoded) query string to Set-based matching on decoded keys:
- Each raw query parameter is now decoded via
decodeURIComponent(rawKey)before checking against the RBAC-controlled set - The controlled keys are stored in a
Set<string>(cluster,project,filter[exact:cluster],filter[exact:project]) instead of regex patterns - This means
filter%5Bexact%3Acluster%5D,filter%5bexact%3acluster%5d, or any other encoded variant will be decoded tofilter[exact:cluster]and correctly stripped
The stripping logic now looks like:
const rbacControlledKeys = new Set(
access.filterStyle === 'ros'
? ['cluster', 'project']
: ['filter[exact:cluster]', 'filter[exact:project]'],
);
const decodedKey = decodeURIComponent(rawKey);
if (!rbacControlledKeys.has(decodedKey)) {
targetUrl.searchParams.append(decodedKey, decodeURIComponent(rawVal));
}
workspaces/cost-management/plugins/cost-management-backend/src/routes/secureProxy.ts
Show resolved
Hide resolved
…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
Author
|
Addressed in 75d3be7 — extracted the common authorize-cache-filter-decide pattern into Net change: -144 lines, +111 lines (33 fewer lines total). |
This was referenced Mar 26, 2026
fix(cost-management): add authorization, validation, and confirmation for Apply Recommendation
#2618
Open
…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>
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
…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
1 participant
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
Addresses three high-severity security findings from the RHDH Cost Plugin threat model.
Jira Tickets
Additional tickets under the same epic (addressed in stacked PRs):
Security Findings Fixed
1. Token Exposure (FLPATH-3487, Finding #1)
The
GET /tokenendpoint returned a full SSO access token (withapi.consolescope) directly to the browser, allowing any authenticated Backstage user to call the Cost Management API externally, bypassing all RBAC controls.Fix: Deleted the
/tokenendpoint entirely. All data fetching now goes through a new server-side secure proxy (/api/cost-management/proxy/*) that:httpAuth2. RBAC Bypass via Query Parameter Manipulation (FLPATH-3487, Finding #2)
The old frontend-side RBAC enforcement allowed users to modify query parameters in browser DevTools to access unauthorized cluster/project data.
Fix: The secure proxy strips all RBAC-controlled filter parameters from the client request and injects the server-determined authorized filters. Additionally, percent-encoded variants of RBAC keys (e.g.,
filter%5Bexact%3Acluster%5D) are decoded before comparison, preventing encoded bypass.3. Path Traversal (FLPATH-3487, Finding #3)
The proxy path was not validated, allowing
../segments or absolute paths.Fix: Pre-construction validation rejects paths containing
../segments or leading/. Post-construction validation confirms the final URL stays within the expected base path.4. 401 Retry Dropping Filters (FLPATH-3504)
The
OptimizationsClient.fetchWithTokenAndRetrymethod's 401-retry code path dropped RBAC filters when re-fetching data after token refresh.Fix: This is now a non-issue because all RBAC filtering happens server-side in the secure proxy. The frontend client no longer handles tokens or RBAC filters at all — it simply calls the backend proxy which handles everything. The
fetchWithTokenAndRetrymethod has been replaced withfetchViaBackendProxy.Architecture change
How it works
When a frontend request arrives at
/api/cost-management/proxy/*, the backend:httpAuthros.*orcost.*policyThis means granting
ros.demolabonly allows seeing data for thedemolabcluster — the user cannot modify query parameters to access other clusters.Test plan
Unit tests
yarn tsc -b— clean compilation, zero errorsCI
Deployment verification
Qodo bot findings (addressed)
decodeURIComponenttry/catch for malformed percent-encoding (returns 400)costManagementProxyBaseUrl