Forward impersonation token to BFF modules in federated dev mode#7372
Forward impersonation token to BFF modules in federated dev mode#7372lucferbux wants to merge 1 commit intoopendatahub-io:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughSix frontend webpack configuration files (automl, autorag, eval-hub, gen-ai, maas, mlflow) replace static proxy header injection from precomputed Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Security IssuesCWE-522 (Insufficiently Protected Credentials): Token extraction logic performs naive string manipulation on bearer tokens without validation.
CWE-269 (Improper Access Control): Conditional header forwarding based on
Missing token presence validation: After stripping
Recommendation: Validate 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/eval-hub/frontend/config/webpack.dev.js (1)
96-130:⚠️ Potential issue | 🟠 MajorCache fallback headers and require Bearer auth before forwarding.
Lines 103 and 125 now run
getProxyHeaders()per fallback request; inuser_tokenmode that shells out tokubectltwice on the dev-server request path for both proxies. Lines 97-101 and 119-123 also forward any non-emptyAuthorizationvalue asx-forwarded-access-tokenwithout validating the Bearer scheme (CWE-20), which can disable the kubectl fallback for malformed auth.Proposed change
const getProxyHeaders = () => { if (AUTH_METHOD === 'internal') { return { 'kubeflow-userid': 'user@example.com', @@ } return {}; }; + +const defaultProxyHeaders = getProxyHeaders(); + +const getForwardedAccessToken = (req) => { + if (AUTH_METHOD !== 'user_token') { + return undefined; + } + const upstreamAuth = req.headers.authorization; + const match = typeof upstreamAuth === 'string' ? upstreamAuth.match(/^Bearer\s+(.+)$/i) : null; + return match ? match[1].trim() : undefined; +}; @@ - onProxyReq: (proxyReq, req) => { - const upstreamAuth = req.headers.authorization; - if (upstreamAuth && AUTH_METHOD === 'user_token') { + onProxyReq: (proxyReq, req) => { + const token = getForwardedAccessToken(req); + if (token) { // Federated mode: forward the upstream token (may be impersonated) as x-forwarded-access-token - const token = upstreamAuth.replace(/^Bearer\s+/i, ''); proxyReq.setHeader('x-forwarded-access-token', token); } else { - const headers = getProxyHeaders(); - Object.entries(headers).forEach(([key, value]) => { + Object.entries(defaultProxyHeaders).forEach(([key, value]) => { proxyReq.setHeader(key, value); }); } }, @@ - onProxyReq: (proxyReq, req) => { - const upstreamAuth = req.headers.authorization; - if (upstreamAuth && AUTH_METHOD === 'user_token') { + onProxyReq: (proxyReq, req) => { + const token = getForwardedAccessToken(req); + if (token) { // Federated mode: forward the upstream token (may be impersonated) as x-forwarded-access-token - const token = upstreamAuth.replace(/^Bearer\s+/i, ''); proxyReq.setHeader('x-forwarded-access-token', token); } else { - const headers = getProxyHeaders(); - Object.entries(headers).forEach(([key, value]) => { + Object.entries(defaultProxyHeaders).forEach(([key, value]) => { proxyReq.setHeader(key, value); }); } },As per coding guidelines, “Changes here affect build performance and output for all consumers.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eval-hub/frontend/config/webpack.dev.js` around lines 96 - 130, Validate the incoming Authorization header in onProxyReq: only treat it as a forwardable token if req.headers.authorization matches the Bearer scheme (/^Bearer\s+/i) before stripping and setting x-forwarded-access-token (use the same regex used elsewhere), and only call getProxyHeaders() when you actually need the fallback; cache the result on the request object (e.g., req.__cachedProxyHeaders) so subsequent proxy handlers reuse it instead of invoking getProxyHeaders() twice; update both onProxyReq blocks to check AUTH_METHOD === 'user_token' AND a Bearer-formatted upstreamAuth before forwarding, otherwise load and reuse req.__cachedProxyHeaders and set those headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/automl/frontend/config/webpack.dev.js`:
- Around line 91-103: The onProxyReq handler currently calls getProxyHeaders()
unnecessarily and forwards any non-empty Authorization value without validating
the Bearer scheme; update onProxyReq to first check that
req.headers.authorization exists and matches the Bearer scheme
(case-insensitive) before extracting the token and setting
x-forwarded-access-token, and otherwise call getProxyHeaders() once and reuse
its return value to set fallback headers; reference onProxyReq, AUTH_METHOD,
getProxyHeaders, and the x-forwarded-access-token header when making these
changes.
In `@packages/autorag/frontend/config/webpack.dev.js`:
- Around line 95-107: The onProxyReq handler currently calls getProxyHeaders()
on every fallback and forwards any non-empty Authorization as
x-forwarded-access-token without validating the Bearer scheme; update onProxyReq
to first check AUTH_METHOD and the Authorization header with a Bearer scheme
(e.g. test /^Bearer\s+/i) and only extract and forward the token as
x-forwarded-access-token when the header is a valid Bearer token (use
upstreamAuth.replace to strip the scheme). If the Authorization header is
missing or not Bearer, call getProxyHeaders() once and apply its returned
headers to proxyReq (avoid calling getProxyHeaders() when a valid Bearer token
is present to prevent unnecessary kubectl shellouts), keeping the existing
proxyReq.setHeader calls.
In `@packages/gen-ai/frontend/config/webpack.dev.js`:
- Around line 85-97: The onProxyReq handler is calling getProxyHeaders() on
every fallback and forwards any non-empty Authorization header as
x-forwarded-access-token without validating the Bearer scheme; fix by (1) moving
a cachedHeaders variable outside/on top of the dev-server proxy scope so
getProxyHeaders() is called once and reused (avoid shelling out repeatedly in
AUTH_METHOD === 'user_token' fallback), and (2) validate that
req.headers.authorization matches the Bearer pattern /^Bearer\s+/i before
extracting and forwarding the token in onProxyReq (only then
setHeader('x-forwarded-access-token', token)); if the Authorization header is
absent or malformed, fall back to using cachedHeaders and set those headers on
proxyReq.
In `@packages/maas/frontend/config/webpack.dev.js`:
- Around line 94-106: The onProxyReq handler currently calls getProxyHeaders()
multiple times and forwards any non-empty Authorization header as
x-forwarded-access-token without validating the Bearer scheme; update onProxyReq
to (1) require the Authorization header to match the Bearer scheme
(/^Bearer\s+/i) before extracting and forwarding the token when AUTH_METHOD ===
'user_token', (2) treat malformed or missing Bearer tokens as a signal to use
the fallback proxy headers instead of attempting to forward, and (3) call
getProxyHeaders() at most once per request (cache its result in a local
variable) so kubectl is not invoked multiple times; reference onProxyReq,
AUTH_METHOD and getProxyHeaders in your change.
In `@packages/mlflow/frontend/config/webpack.dev.js`:
- Around line 90-102: The onProxyReq handler currently forwards any non-empty
req.headers.authorization as x-forwarded-access-token and calls
getProxyHeaders() on the fallback path, which can invoke kubectl twice; update
onProxyReq to first validate the upstreamAuth strictly matches the Bearer scheme
(e.g. /^\s*Bearer\s+/i) before extracting and forwarding the token, and only
call getProxyHeaders() when that validation fails; also ensure getProxyHeaders()
is invoked at most once per request by computing a single headers variable when
needed and reusing it (references: onProxyReq, AUTH_METHOD,
req.headers.authorization, getProxyHeaders, proxyReq.setHeader).
---
Outside diff comments:
In `@packages/eval-hub/frontend/config/webpack.dev.js`:
- Around line 96-130: Validate the incoming Authorization header in onProxyReq:
only treat it as a forwardable token if req.headers.authorization matches the
Bearer scheme (/^Bearer\s+/i) before stripping and setting
x-forwarded-access-token (use the same regex used elsewhere), and only call
getProxyHeaders() when you actually need the fallback; cache the result on the
request object (e.g., req.__cachedProxyHeaders) so subsequent proxy handlers
reuse it instead of invoking getProxyHeaders() twice; update both onProxyReq
blocks to check AUTH_METHOD === 'user_token' AND a Bearer-formatted upstreamAuth
before forwarding, otherwise load and reuse req.__cachedProxyHeaders and set
those headers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 5dec798c-02e6-43f4-8f3b-44c37865491e
⛔ Files ignored due to path filters (1)
packages/model-registry/upstream/frontend/config/webpack.dev.jsis excluded by!**/upstream/**
📒 Files selected for processing (6)
packages/automl/frontend/config/webpack.dev.jspackages/autorag/frontend/config/webpack.dev.jspackages/eval-hub/frontend/config/webpack.dev.jspackages/gen-ai/frontend/config/webpack.dev.jspackages/maas/frontend/config/webpack.dev.jspackages/mlflow/frontend/config/webpack.dev.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7372 +/- ##
==========================================
- Coverage 64.97% 63.98% -0.99%
==========================================
Files 2448 2513 +65
Lines 76175 77980 +1805
Branches 19221 19829 +608
==========================================
+ Hits 49492 49894 +402
- Misses 26683 28086 +1403 see 121 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Replace static `headers: getProxyHeaders()` with `onProxyReq` callback in all module webpack dev proxies. In federated mode, the main dashboard backend sets the authorization header (which may carry an impersonated token). The new callback extracts it and forwards as x-forwarded-access-token for the BFF, matching production convention. In standalone/internal auth modes, falls back to getProxyHeaders(). model-registry is excluded — handled upstream via kubeflow/hub#2544.
lucferbux
force-pushed
the
rhoaieng-59422
branch
from
7387086 to
e89fa04
Compare
|
Closing this in favor of #7065 |
1 participant
https://issues.redhat.com/browse/RHOAIENG-59422
Description
In federated dev mode (
make dev-start-federated), module API calls flow through a double proxy chain:The main dashboard backend sets the
authorizationheader on proxied requests — this can carry either the developer's kubectl token or an impersonated user's token (via the dev impersonation toggle). However, each module's webpack dev proxy used a staticheaders: getProxyHeaders()option that always overwritesx-forwarded-access-tokenwith the developer's kubectl token (resolved once at startup), effectively discarding any impersonation.Fix: Replace the static
headers:with anonProxyReqcallback in all module webpack.dev.js files. When the incoming request has anauthorizationheader andAUTH_METHOD === 'user_token'(federated mode), the callback extracts the token and forwards it asx-forwarded-access-token— matching the production convention BFFs already use. In standalone or internal auth modes, it falls back togetProxyHeaders().Modules changed
packages/maas/frontend/config/webpack.dev.jspackages/gen-ai/frontend/config/webpack.dev.jspackages/automl/frontend/config/webpack.dev.jspackages/autorag/frontend/config/webpack.dev.jspackages/mlflow/frontend/config/webpack.dev.jspathRewritepreservedpackages/eval-hub/frontend/config/webpack.dev.jsNo Makefile or
getProxyHeaders()changes — only the proxy wiring in webpack.dev.js.Behavior per mode
x-forwarded-access-tokenuser_token)getProxyHeaders()(same as before)internal)getProxyHeaders()/kubeflow-userid(same as before)How Has This Been Tested?
Tested on a live cluster with temporary debug logging added to the BFF
ExtractRequestIdentitymethod (logging the first 8 and last 4 chars of the received token). Validated with both maas and model-registry BFFs in federated mode.Test setup
npm run devcd packages/maas && make dev-start-federatedcd packages/model-registry && make dev-start-federatedResults — Admin (no impersonation)
Both BFFs receive the admin kubectl token (
sha256~m...6wd8):Model Registry:
MaaS:
Results — Impersonating dev user
Both BFFs receive a different token (
sha256~D...7l94) — the impersonated user's token:Model Registry:
MaaS:
RBAC works end-to-end: the impersonated user is correctly identified as non-admin, and permission-gated resources are properly denied.
Test Impact
This is a dev-only change (webpack dev server proxy configuration). No production code is affected. No new tests needed — the change is verified through manual testing of the dev impersonation flow.
Request review criteria:
Self checklist (all need to be checked):
After the PR is posted & before it merges:
main