[fix][core] keep server side rendering on the dashboard origin - #7927
Open
ar2rsawseen wants to merge 1 commit into
Open
[fix][core] keep server side rendering on the dashboard origin#7927ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
api/utils/render.js builds the url it opens by concatenating the configured dashboard host with the requested view, and /o/render lets the caller choose that view. With the default countlyConfig.path of "" the prefix is exactly "http://localhost", so a view that does not begin with "/" changes the host rather than the path, and the headless browser opens that instead of the dashboard. Parse the concatenated url and require its origin to match the dashboard's before navigating. What gets navigated to is the concatenation itself, unchanged, so a configured countlyConfig.path keeps behaving exactly as it did. Parsing settles the whole family of shapes at once, including the ones the url parser normalises away, and it agrees with what Chromium does with the same string since both use the WHATWG parser. Also restrict the renderer to the dashboard origin with request interception, the same control api/utils/pdf.js already uses. The dashboard serves all of its own assets, so nothing in a normal render is refused: measured against real Chromium, own origin images and stylesheets still load and the screenshot is still produced. A private range denylist would be the wrong control here. The intended render target is loopback, which api/utils/ssrf-protection.js blocks by design.
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.
Backport of #7926 to release.24.05. Same change, the file is identical in the relevant region.
What
api/utils/render.jsbuilds the url it opens by concatenating the configured dashboard host with the requested view:/o/rendertakes that view from the request (params.qstring.view). With the defaultcountlyConfig.pathof""the prefix is exactlyhttp://localhost, so a view that does not begin with/changes the host rather than the path:/#/dashboardhttp://localhost/#/dashboard, as intended@169.254.169.254/latest/...169.254.169.254:8500/v1/kv/?recurselocalhost:8500.internal.example/xlocalhost.internal.exampleThe headless browser then loads that from the server's own network position.
Change
countlyConfig.pathkeeps behaving exactly as it did. Parsing settles the whole family of shapes at once, including the ones the url parser normalises away (tab, newline, carriage return), and it agrees with what Chromium does with the same string since both use the WHATWG parser.api/utils/pdf.jsuses, so the page's subresources are bounded too and not just the navigation.A private range denylist would be the wrong control here: the intended render target is loopback, which
api/utils/ssrf-protection.jsblocks by design.Scope
Everything that drives a headless browser, with a verdict for each:
api/utils/render.js,page.goto(host + view)api/utils/render.js, login token navigationapi/utils/pdf.jsapi/utils/requestProcessor.js, the/o/rendercallerparams.qstring.viewthrough, now checked at the sinkplugins/dashboards/api/api.jscaller"/dashboard?ssr=true#/custom/" + id, unaffected and covered by the tests belowrender.jsand norenderViewcallerNeither caller sets
options.host, so the base always stays the configured dashboard.Verification
test/unit-tests/api.utils.render.js, 38 cases: every host rewriting shape is refused for each host form, and every view the product itself renders is allowed and returned byte identical. Swapping the check back for plain concatenation fails 13 of them, so the tests do pin the behaviour.renderViewandrenderPDFintegration suite (test/2.api/99.api.utils.render.js) passes against the patched file, 7 of 7, including both real screenshot cases.