[ZEPPELIN-6521] Preserve withCredentials on HTTP requests in production builds - #5377
Open
big-cir wants to merge 1 commit into
Open
[ZEPPELIN-6521] Preserve withCredentials on HTTP requests in production builds#5377big-cir wants to merge 1 commit into
big-cir wants to merge 1 commit into
Conversation
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.
What is this PR for?
In production builds the interceptor re-clones from the original
httpRequestwhen it addsX-Requested-With, discarding thewithCredentials: trueclone made one line earlier.clone()inherits from whatever it was cloned from (update.withCredentials ?? this.withCredentials), so the production request goes out with the defaultfalse.let httpRequestUpdated = httpRequest.clone({ withCredentials: true }); if (environment.production) { - httpRequestUpdated = httpRequest.clone({ setHeaders: { 'X-Requested-With': 'XMLHttpRequest' } }); + httpRequestUpdated = httpRequestUpdated.clone({ setHeaders: { 'X-Requested-With': 'XMLHttpRequest' } }); }Two things the ticket does not cover. First,
BaseUrlServicebuilds the REST base fromlocation, so every call to Zeppelin's own API is same-origin and the browser attaches cookies whether or not the flag is set; the flag has no reachable effect on those calls today. The classic UI's equivalent service remaps the port when the UI is served from the grunt dev server (zeppelin-web/src/components/base-url/base-url.service.js:26-29), which is what madewithCredentialsmeaningful there, and the new UI has no such path. So this is a correctness fix — the interceptor now applies both of its settings consistently across builds, the way the classic UI does (zeppelin-web/src/app/app.js:78and:161-163) — rather than a behaviour fix for API traffic. The second is the subject of the next section.Scope and related issues
Third-party URL fetches go through this interceptor too, and this change narrows what they can reach.
NoteImportComponentpasses a user-supplied URL straight toHttpClient(note-import.component.ts:49), so the "import note from URL" request carries the samewithCredentialsandX-Requested-Withas a call to Zeppelin's own API. A browser rejects a credentialed cross-origin response whoseAccess-Control-Allow-Originis*, and that wildcard is what public file hosts serve.Measured in Chromium against local servers reproducing each CORS configuration, with the
raw.githubusercontent.compreflight response checked directly:X-Requested-Withat preflight —raw.githubusercontent.comanswers the preflight with 403X-Requested-With, servesAccess-Control-Allow-Origin: *Only the second row changes, and it changes production to match what development already does, which is what this ticket asks for.
The underlying problem is that the interceptor does not distinguish Zeppelin's own API from an arbitrary URL. The classic UI handles it by overriding
withCredentials: falsefor exactly this request (zeppelin-web/src/components/note-import/note-import.controller.js:95-97); the new UI has no equivalent. That is a separate defect which predates this ticket and already breaks the feature in development builds today.Scoping the interceptor so that
withCredentialsandX-Requested-Withare applied only to Zeppelin's own API would resolve it, and would not change anything this PR does for API calls. I have not filed a ticket for it yet — I would rather hear whether that scoping belongs in this PR or in a follow-up.What type of PR is it?
Bug Fix
Todos
X-Requested-Withis still added in production buildsWhat is the Jira issue?
How should this be tested?
There is no unit-test runner for this app to add a spec to: the
zeppelinproject inangular.jsondeclares onlybuild,serve,extract-i18nandlint, andzeppelin-web-angular/srccontains no.spec.ts. A Playwright test is also a poor fit here, because the local suite runs againstng serve(playwright.config.js:16), whereenvironment.productionisfalseand the affected branch never executes — such a test would pass with or without this change unless run in CI mode. The change was verified three other ways instead.1. Lint
Exit 0. 15 pre-existing
member-orderingwarnings inprojects/zeppelin-visualization, none in the changed file;lint:reactclean;prettier --checkreports all files formatted.2.
HttpRequest.clone()inheritance, using the real classRunning both the old and the new form of the production branch through Angular's real
HttpRequestclass:3. Production bundle in a real browser
ng build --configuration productiontwice — once with this commit and once with it reverted, changing nothing else — servingdist/zeppelinstatically and instrumenting theXMLHttpRequest.prototype.withCredentialssetter.BaseUrlServicederives the REST base fromlocation, so the app issues its normal bootstrap calls against the static server; they 404, but Angular assignswithCredentialsbeforesend(), which is what is being observed.X-Requested-Withis present in both runs, which confirms the production branch really executed and that the change does not drop the header.Screenshots (if appropriate)
N/A
Questions: