You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "martyngigg (via GitHub)" <gi...@apache.org> on 2024/09/03 11:23:00 UTC
[PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
martyngigg opened a new pull request, #30134:
URL: https://github.com/apache/superset/pull/30134
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Introduces changes in the front and backend to allow Superset to be deployed under a prefixed-URL path rather than the root of the domain. A new environment variable `BASE_PATH` has been introduced into webpack, alongside the existing `ASSET_BASE_URL`, to define the prefix.
The frontend has been updated to include the prefix in any resource links along with two new helper utilities, `assetUrl` & `pathUtils`, to help construct the paths. The `SupersetClient` has been update to remove the unused `baseUrl` field and rename it as `basePath` such that the object can be initialized with the base path and calling any endpoints through this class can use the same relative link as before. QUESTION: Have I broken an API here?
The backend has been refactored to avoid using hardcoded paths in redirects and instead uses `Flask.url_for` to construct reference. This requires that the proxy server set the `X-Forwarded-Prefix` header along with setting `ENABLE_PROXY_FIX=True` in `superset_config.py`
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1935664082
##########
superset-frontend/src/features/home/ChartTable.tsx:
##########
@@ -193,7 +194,7 @@
),
buttonStyle: 'tertiary',
onClick: () => {
- window.location.assign('/chart/add');
+ window.location.assign(ensureAppRoot('/chart/add'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1265)
##########
superset-frontend/src/dashboard/components/SaveModal.tsx:
##########
@@ -154,7 +155,9 @@
} else {
this.onSave(data, dashboardId, saveType).then((resp: JsonResponse) => {
if (saveType === SAVE_TYPE_NEWDASHBOARD && resp.json?.result?.id) {
- window.location.href = `/superset/dashboard/${resp.json.result.id}/`;
+ window.location.href = ensureAppRoot(
+ `/superset/dashboard/${resp.json.result.id}/`,
+ );
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1264)
##########
superset-frontend/src/features/home/DashboardTable.tsx:
##########
@@ -196,19 +197,20 @@
),
buttonStyle: 'tertiary',
onClick: () => {
- window.location.assign('/dashboard/new');
+ window.location.assign(ensureAppRoot('/dashboard/new'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1267)
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -459,7 +460,7 @@
{menu.label}
</Link>
) : (
- <a href={menu.url}>
+ <a href={ensureAppRoot(menu.url || '')}>
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1269)
##########
superset-frontend/src/pages/AnnotationLayerList/index.tsx:
##########
@@ -277,7 +278,7 @@
};
const onLayerAdd = (id?: number) => {
- window.location.href = `/annotationlayer/${id}/annotation`;
+ window.location.href = ensureAppRoot(`/annotationlayer/${id}/annotation`);
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1270)
##########
superset-frontend/src/pages/DashboardList/index.tsx:
##########
@@ -682,7 +683,7 @@
),
buttonStyle: 'primary',
onClick: () => {
- window.location.assign('/dashboard/new');
+ window.location.assign(ensureAppRoot('/dashboard/new'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1271)
##########
superset-frontend/src/features/home/SavedQueries.tsx:
##########
@@ -264,7 +266,7 @@
name: t('View All »'),
buttonStyle: 'link',
onClick: () => {
- window.location.href = '/savedqueryview/list';
+ window.location.href = ensureAppRoot('/savedqueryview/list');
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1268)
##########
superset-frontend/src/features/home/EmptyState.tsx:
##########
@@ -84,7 +85,7 @@
<Button
buttonStyle="default"
onClick={() => {
- window.location.href = url;
+ window.location.href = ensureAppRoot(url);
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1266)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "jeanpommier (via GitHub)" <gi...@apache.org>.
jeanpommier commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2621875701
Hi @martyngigg
I believe you might have missed some places in the frontend:
This is kind of a hedge case, but if you edit a dashboard, go to "save as", select overwrite, it is redirecting to /superset/dashboard/yourdashslug after you save a dashboard edit (no prefix anymore)
I believe it might be due to https://github.com/martyngigg/superset/blob/allow_prefixed_paths/superset-frontend/src/dashboard/actions/dashboardState.js#L410 messing up the browser history also, at some point.
Reproduce it:
- edit a dashboard
- save as
- overwrite
- you get redirected to the prefix-less path
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho merged PR #30134:
URL: https://github.com/apache/superset/pull/30134
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-3037046980
labeled https://github.com/apache/superset/issues/33304 as "good first issue" for hopefully someone to pick up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "BrianJavier99 (via GitHub)" <gi...@apache.org>.
BrianJavier99 commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2724701721
> > > > > > Thanks for tackling this... it's a popular request, no doubt about it. If you think it's relevant, we might also want to add an entry in the documentation somewhere.
> > > > >
> > > > >
> > > > > Yes agreed I definitely need to add some documentation for how to use this. I wanted to wait until I was more confident that the general approach was acceptable before working on describing how to use it. In terms of a review as part of #26319 is there anything I need to do at the moment?
> > > >
> > > >
> > > > Thanks for these changes. Can you please mention values for BASE_PATH and ASSET_BASE_URL. Just wanted to check these changes with superset and nginx.
> > >
> > >
> > > Sorry for the delay in replying to this. I've had zero time to spend on this lately.
> > > The latest commits rebase on the current master and I have also added some documentation to the configuring superset pages that describe the new variables with an example `nginx.conf`. Does that help?
> >
> >
> > Thanks a lot for the response. We tried the changes as recommended and observed that most of the things are working when we specify prefix. However there are few things where prefix is not applied correctly as shown in the image below (chart menu has prefix applied correctly) 
> > Also dashboard menu items export to PDF/image not working (Dashboard -> Download -> Export to PDF/Download as Image)
>
> I tried to fix above issue of missing prefixes in top-level menu. Please check this PR [2](https://github.com/martyngigg/superset/pull/2)
Was this PR reverted? I think this was supposed to fix the issue that I have mentioned in my [previous comment](https://github.com/apache/superset/pull/30134#issuecomment-2720261157)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2789641137
> @landryb Thanks for reporting that, should be fixed now
@landryb I'm not sure I understand why this broke as `ensureAppRoot` is still in places such as [RightMenu](https://github.com/apache/superset/blob/d463c0027aa9d9dc1d92444b74e424d5a278263c/superset-frontend/src/features/home/RightMenu.tsx#L330). Should those calls be now taken out of there?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2734105270
@antonibote you can look at various CI step titled "Build & publish docker iages", I think they do push to a place where you may be able to pull from. But since CI isn't succeeding I'm not sure if I'd invest the time to test an image we know has issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930224223
##########
superset-frontend/src/utils/getBootstrapData.ts:
##########
@@ -25,3 +26,17 @@ export default function getBootstrapData(): BootstrapData {
const dataBootstrap = appContainer?.getAttribute('data-bootstrap');
return dataBootstrap ? JSON.parse(dataBootstrap) : DEFAULT_BOOTSTRAP_DATA;
}
+
+// eslint-disable-next-line import/no-mutable-exports
+const APPLICATION_ROOT_NO_TRAILING_SLASH =
+ getBootstrapData().common.application_root.replace(/\/$/, '');
+
+/**
+ * @param dom_sanitize If true, run the application root through dompurify before returning
+ * @returns The configured application root
+ */
+export function applicationRoot(dom_sanitize = false): string {
+ return dom_sanitize
+ ? DOMPurify.sanitize(APPLICATION_ROOT_NO_TRAILING_SLASH)
Review Comment:
Looking at the changed files in #31996 it seems like there are existing check annotations mentioning the same security warning in several other places, e.g. [GenericLink](https://github.com/apache/superset/pull/31996/files#diff-652975a3a5f9c68f1ba3fd9ee7a49068a35f124dd52ef33d34c8c0d7498c01b8).
Would it be worth simplifying the code here to remove the sanitization and those issues be addressed elsewhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "wuqicyber (via GitHub)" <gi...@apache.org>.
wuqicyber commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2574980414
> Happy New Year! 新年快乐！
>
> I've investigated further and found the issue with "Export to PDF" & "Download image" not working on dashboards. The issue seems to be the interaction of the headless browser and `WEBDRIVER_BASEURL`. In the docker compose setup `superset_config.py` suggests setting `WEBDRIVER_BASEURL=http://superset_app:8088/`, so accessing the superset instance directly and skipping the nginx reverse proxy. This is fine when there is no url prefix set as the paths that both the reverse proxy and the superset app see are the same. When there is a url prefix set it is baked into the javascript files that webpack builds and when accessing the superset app directly the extra prefix part of the path raises an error in the superset app and the screenshot generation fails.我已经进一步调查并发现“导出为 PDF”和“下载图片”在仪表板上无法工作的问题。问题似乎是 headless 浏览器和 `WEBDRIVER_BASEURL` 之间的交互。在 docker compose 设置中， `superset_con
fig.py` 建议设置 `WEBDRIVER_BASEURL=http://superset_app:8088/` ，因此直接访问 superset 实例并跳过 nginx 反向代理。如果没有设置 URL 前缀，这是可以的，因为反向代理和 superset 应用看到的路径是相同的。当设置了 URL 前缀时，它会被烘焙到 webpack 构建的 javascript 文件中，并且当直接访问 superset 应用时，路径的额外前缀部分会在 superset 应用中引发错误，截图生成失败。
>
> A fix/workaround (??) seems to be to set `WEBDRIVER_BASEURL = f"http://superset_nginx{base_path}/"` but this also requires removing this [with statement](https://github.com/apache/superset/blob/109e6c69ff25b82b603a4daff40207250788e6b1/superset/utils/urls.py#L35) that seems to set a test context that doesn't know about the prefix. I'm not sure of the knock on effects of this. @rusackas Would you know someone who could comment on this or possibly suggest an alternative approach?一个修复/解决方案似乎是设置 `WEBDRIVER_BASEURL = f"http://superset_nginx{base_path}/"` ，但这也需要删除这个 with 语句，该语句似乎设置了一个不知道前缀的测试上下文。我不确定这会有什么副作用。您认识可以对此发表评论或可能提出替代方法的人吗？
@martyngigg try APPLICATION_ROOT, by setting it to perfix, i found it works
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "jeanpommier (via GitHub)" <gi...@apache.org>.
jeanpommier commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2575019633
> I'll try again since it certainly seems to just require an environment variable :)
Hi @martyngigg
The trick is, you still need to set the BASE_PATH env var for the frontend (needs for now to be built with the prefix), but not for the superset backend, since this is the WSGI server that handles the prefix.
Here is a patch that should work, to run it with the gunicorn server. I've recently rebased your PR against master, so you might get a bit of noise in the docker-compo: [gunicorn_script_name.patch.gz](https://github.com/user-attachments/files/18331559/gunicorn_script_name.patch.gz)
And a similar patch to run the same principle with the flask dev server. I still haven't looked at a common way to get this to work, but it shouldn't be too complicated (I guess there is some flag somewhere indicating if we run the dev server):
[flask_devserver_script_name.patch.gz](https://github.com/user-attachments/files/18331605/flask_devserver_script_name.patch.gz)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "jeanpommier (via GitHub)" <gi...@apache.org>.
jeanpommier commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1925620113
##########
docker/.env:
##########
@@ -54,8 +54,9 @@ REDIS_HOST=redis
REDIS_PORT=6379
FLASK_DEBUG=true
+SUPERSET_APP_ROOT="/"
SUPERSET_ENV=development
-SUPERSET_LOAD_EXAMPLES=yes
+SUPERSET_LOAD_EXAMPLES=no
Review Comment:
you'll probably want to keep this to `yes`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r2031474394
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ensureAppRoot } from './pathUtils';
+
+export const navigateTo = (
+ url: string,
+ options?: { newWindow?: boolean; assign?: boolean },
+) => {
+ if (options?.newWindow) {
+ window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
+ } else if (options?.assign) {
+ window.location.assign(ensureAppRoot(url));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/2005)
##########
superset-frontend/src/utils/navigationUtils.ts:
##########
@@ -0,0 +1,44 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ensureAppRoot } from './pathUtils';
+
+export const navigateTo = (
+ url: string,
+ options?: { newWindow?: boolean; assign?: boolean },
+) => {
+ if (options?.newWindow) {
+ window.open(ensureAppRoot(url), '_blank', 'noopener noreferrer');
+ } else if (options?.assign) {
+ window.location.assign(ensureAppRoot(url));
+ } else {
+ window.location.href = ensureAppRoot(url);
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/2006)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2786386903
> @kgabryje Thank you so much for looking at this and figuring out the issues with the tests that I couldn't work out. I don't really have any experience with antd but I've noticed quite a few comments like "// TODO: Remove antd" across various components - is there a long term plan to remove it?
AFAIK, yes - there's an ongoing effort to upgrade antd to v5 and to remove direct imports from antd throughout the code. (CC @geido)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "ravikantkml (via GitHub)" <gi...@apache.org>.
ravikantkml commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2361393258
> > Thanks for tackling this... it's a popular request, no doubt about it. If you think it's relevant, we might also want to add an entry in the documentation somewhere.
>
> Yes agreed I definitely need to add some documentation for how to use this. I wanted to wait until I was more confident that the general approach was acceptable before working on describing how to use it. In terms of a review as part of #26319 is there anything I need to do at the moment?
Thanks for these changes.
Can you please mention values for BASE_PATH and ASSET_BASE_URL. Just wanted to check these changes with superset and nginx.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "ravikantkml (via GitHub)" <gi...@apache.org>.
ravikantkml commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2454660025
> > > > Thanks for tackling this... it's a popular request, no doubt about it. If you think it's relevant, we might also want to add an entry in the documentation somewhere.
> > >
> > >
> > > Yes agreed I definitely need to add some documentation for how to use this. I wanted to wait until I was more confident that the general approach was acceptable before working on describing how to use it. In terms of a review as part of #26319 is there anything I need to do at the moment?
> >
> >
> > Thanks for these changes. Can you please mention values for BASE_PATH and ASSET_BASE_URL. Just wanted to check these changes with superset and nginx.
>
> Sorry for the delay in replying to this. I've had zero time to spend on this lately.
>
> The latest commits rebase on the current master and I have also added some documentation to the configuring superset pages that describe the new variables with an example `nginx.conf`. Does that help?
Thanks a lot for the response.
We tried the changes as recommended and observed that most of the things are working when we specify prefix. However there are few things where prefix is not applied correctly as shown in the image below (chart menu has prefix applied correctly)
Also dashboard menu items export to PDF/image not working (Dashboard -> Download -> Export to PDF/Download as Image)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "jeanpommier (via GitHub)" <gi...@apache.org>.
jeanpommier commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2553189513
Hi,
Good job there, thank you very much for tackling this. I have just discovered that Superset did not support that and was feeling disappointed.
I can see this PR is still flagged as DRAFT. What's left to do to get it "Ready for Review" ?
@martyngigg do you need help in completing this PR ? I'm quite a superset newbie, but I'm willing to provide some help if necessary to get this merged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "jeanpommier (via GitHub)" <gi...@apache.org>.
jeanpommier commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2573172590
Hello ! Happy new year !
> This is fine when there is no url prefix set as the paths that both the reverse proxy and the superset app see are the same. When there is a url prefix set it is baked into the javascript files that webpack builds and when accessing the superset app directly the extra prefix part of the path raises an error in the superset app and the screenshot generation fails.
Well, I might have a solution for this one. I didn't get the time to look at the image generation task you proposed, but I was rather investigating around the path prefix topic. My concern was to avoid relying on a reverse-proxy for the path-prefix thing. IMO, it makes things complicated. For instance the issue you met above: dev setup (without docker compo) and prod-like setup with a reverse proxy works differently, with a different frontend where the path is hard-coded.
Looking on the backend side of it, my first idea was that we maybe didn't use the Flask blueprints to their full extent, but it turns out this would not be that easy to adapt to our needs. And maybe not a clean solution.
***But***, I stumbled across [this blog post](https://dlukes.github.io/flask-wsgi-url-prefix.html) where I discovered that WSGI protocol supports a parameter that sets such a path prefix, and that is handled on the WSGI side of it (e.g. gunicorn). I tested it (on top of your PR since we need the url_for links) and it works like a charm.
**What does it change ?**
Supposing you choose `analytics` as the path prefix, the backend is accessed on http://superset_app:8088/analytics/. and the reverse-proxy has a much simpler job of just routing /analytics to http://superset_app:8088/analytics/.
On your above-mentioned issue, this means that the paths will concord between the front and back end, so it should work without even needing the WEBDRIVER_BASEURL fix you propose.
I'm still quite uncomfortable at the necessity to build the frontend to support a path other than root. I haven't made experiments yet on this, but wouldn't it be conceivable that the frontend looks for a config instruction from the backend to get this information ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "surajsrivathsa (via GitHub)" <gi...@apache.org>.
surajsrivathsa commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2700145177
Hello All,
I am trying to run the superset with these changes in my local with docker-compose.yml and ran into some problems. It would be helpful if you have any insights on this.
I tried with both SUPERSET_APP_ROOT with both default "/" and a non-root path "/analytics/reports.
But i get below login screen and welcome screen which is not real screen.
Should i set any other configs to get this working? Thanks in advance for your help
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2796066795
> @martyngigg anyway you can fix the issues that landed through this PR on `master`? I tried and fix some but didn't have full context. Would prefer not reverting this PR
Sure. I'll take a look now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2791864170
Thank you to everyone for helping to get this merged! 🎉 😀
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2414174751
> > > Thanks for tackling this... it's a popular request, no doubt about it. If you think it's relevant, we might also want to add an entry in the documentation somewhere.
> >
> >
> > Yes agreed I definitely need to add some documentation for how to use this. I wanted to wait until I was more confident that the general approach was acceptable before working on describing how to use it. In terms of a review as part of #26319 is there anything I need to do at the moment?
>
> Thanks for these changes. Can you please mention values for BASE_PATH and ASSET_BASE_URL. Just wanted to check these changes with superset and nginx.
Sorry for the delay in replying to this. I've had zero time to spend on this lately.
The latest commits rebase on the current master and I have also added some documentation to the configuring superset pages that describe the new variables with an example `nginx.conf`. Does that help?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1922719303
##########
superset-frontend/src/features/home/EmptyState.tsx:
##########
@@ -171,7 +173,7 @@
<Button
buttonStyle="primary"
onClick={() => {
- window.location.href = favRedirects[tableName];
+ window.location.href = ensureBasePath(favRedirects[tableName]);
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1244)
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -495,7 +496,9 @@
{isFrontendRoute(child.url) ? (
<Link to={child.url || ''}>{menuItemDisplay}</Link>
) : (
- <a href={child.url}>{menuItemDisplay}</a>
+ <a href={ensureBasePath(child.url || '')}>
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1249)
##########
superset-frontend/src/features/home/ChartTable.tsx:
##########
@@ -193,7 +194,7 @@
),
buttonStyle: 'tertiary',
onClick: () => {
- window.location.assign('/chart/add');
+ window.location.assign(ensureBasePath('/chart/add'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1242)
##########
superset-frontend/src/pages/DashboardList/index.tsx:
##########
@@ -682,7 +683,7 @@
),
buttonStyle: 'primary',
onClick: () => {
- window.location.assign('/dashboard/new');
+ window.location.assign(ensureBasePath('/dashboard/new'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1252)
##########
superset-frontend/src/dashboard/components/SaveModal.tsx:
##########
@@ -154,7 +155,9 @@
} else {
this.onSave(data, dashboardId, saveType).then((resp: JsonResponse) => {
if (saveType === SAVE_TYPE_NEWDASHBOARD && resp.json?.result?.id) {
- window.location.href = `/superset/dashboard/${resp.json.result.id}/`;
+ window.location.href = ensureBasePath(
+ `/superset/dashboard/${resp.json.result.id}/`,
+ );
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1241)
##########
superset-frontend/src/features/home/DashboardTable.tsx:
##########
@@ -196,19 +197,20 @@
),
buttonStyle: 'tertiary',
onClick: () => {
- window.location.assign('/dashboard/new');
+ window.location.assign(ensureBasePath('/dashboard/new'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1243)
##########
superset-frontend/src/features/home/SavedQueries.tsx:
##########
@@ -264,7 +266,7 @@
name: t('View All »'),
buttonStyle: 'link',
onClick: () => {
- window.location.href = '/savedqueryview/list';
+ window.location.href = ensureBasePath('/savedqueryview/list');
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1250)
##########
superset-frontend/src/pages/AnnotationLayerList/index.tsx:
##########
@@ -277,7 +278,7 @@
};
const onLayerAdd = (id?: number) => {
- window.location.href = `/annotationlayer/${id}/annotation`;
+ window.location.href = ensureBasePath(`/annotationlayer/${id}/annotation`);
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1251)
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -460,7 +461,7 @@
{menu.label}
</Link>
) : (
- <a href={menu.url}>
+ <a href={ensureBasePath(menu.url || '')}>
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1248)
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -313,11 +314,11 @@
>
{isFrontendRoute(window.location.pathname) ? (
<GenericLink className="navbar-brand" to={brand.path}>
- <img src={brand.icon} alt={brand.alt} />
+ <img src={assetUrl(brand.icon)} alt={brand.alt} />
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1245)
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -313,11 +314,11 @@
>
{isFrontendRoute(window.location.pathname) ? (
<GenericLink className="navbar-brand" to={brand.path}>
- <img src={brand.icon} alt={brand.alt} />
+ <img src={assetUrl(brand.icon)} alt={brand.alt} />
</GenericLink>
) : (
<a className="navbar-brand" href={brand.path} tabIndex={-1}>
- <img src={brand.icon} alt={brand.alt} />
+ <img src={assetUrl(brand.icon)} alt={brand.alt} />
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1247)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930013792
##########
superset-frontend/src/utils/pathUtils.ts:
##########
@@ -0,0 +1,37 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { applicationRoot } from 'src/utils/getBootstrapData';
+
+/**
+ * Takes a string path to a resource and prefixes it with the application root that is
+ * defined in the application configuration. The path is unsanitized.
+ * @param path A string path to a resource
+ */
+export function ensureAppRootUnsanitized(path: string): string {
Review Comment:
I'm not sure if I understand what a Sanitized or Unsanitized path means in this context (?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930002094
##########
Dockerfile:
##########
@@ -62,9 +62,9 @@ RUN --mount=type=bind,source=./superset-frontend/package.json,target=./package.j
--mount=type=cache,target=/root/.cache \
--mount=type=cache,target=/root/.npm \
if [ "$DEV_MODE" = "false" ]; then \
- npm ci; \
+ npm ci; \
Review Comment:
why removing the indentation here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930020931
##########
superset-frontend/src/utils/getBootstrapData.ts:
##########
@@ -25,3 +26,17 @@ export default function getBootstrapData(): BootstrapData {
const dataBootstrap = appContainer?.getAttribute('data-bootstrap');
return dataBootstrap ? JSON.parse(dataBootstrap) : DEFAULT_BOOTSTRAP_DATA;
}
+
+// eslint-disable-next-line import/no-mutable-exports
+const APPLICATION_ROOT_NO_TRAILING_SLASH =
+ getBootstrapData().common.application_root.replace(/\/$/, '');
+
+/**
+ * @param dom_sanitize If true, run the application root through dompurify before returning
+ * @returns The configured application root
+ */
+export function applicationRoot(dom_sanitize = false): string {
+ return dom_sanitize
+ ? DOMPurify.sanitize(APPLICATION_ROOT_NO_TRAILING_SLASH)
Review Comment:
I'm not sure if i understand why we need to sanitize the app route every time we read it(?) Seems it'd be a very static thing(?) Do we have trust issues with the bootstrap data? Also concerned about parsing/reparsing bootstrap data every time we encounter a URL. Assuming that just the basic menu has dozens of links, we'd be running `JSON.parse` for each one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930153697
##########
Dockerfile:
##########
@@ -62,9 +62,9 @@ RUN --mount=type=bind,source=./superset-frontend/package.json,target=./package.j
--mount=type=cache,target=/root/.cache \
--mount=type=cache,target=/root/.npm \
if [ "$DEV_MODE" = "false" ]; then \
- npm ci; \
+ npm ci; \
Review Comment:
Sorry, this wasn't intentional. My IDE seems hell bent on doing this. I'll fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930170846
##########
superset-frontend/src/utils/getBootstrapData.ts:
##########
@@ -25,3 +26,17 @@ export default function getBootstrapData(): BootstrapData {
const dataBootstrap = appContainer?.getAttribute('data-bootstrap');
return dataBootstrap ? JSON.parse(dataBootstrap) : DEFAULT_BOOTSTRAP_DATA;
}
+
+// eslint-disable-next-line import/no-mutable-exports
+const APPLICATION_ROOT_NO_TRAILING_SLASH =
+ getBootstrapData().common.application_root.replace(/\/$/, '');
+
+/**
+ * @param dom_sanitize If true, run the application root through dompurify before returning
+ * @returns The configured application root
+ */
+export function applicationRoot(dom_sanitize = false): string {
+ return dom_sanitize
+ ? DOMPurify.sanitize(APPLICATION_ROOT_NO_TRAILING_SLASH)
Review Comment:
This was an attempt to address a security warning that CodeQL highlighted in multiple places when assigning the result of the new `ensureAppRootSanitized` function in `pathUtils` to a `href` element, for example:
<img width="821" alt="image" src="https://github.com/user-attachments/assets/f07a1797-d98e-49ff-982c-6e092e235f36" />
Reflecting on this, yes that will be a lot of repeated work (and I'm not crazy about the new names `ensureAppRootSanitized` & `ensureAppRootUnsanitized` in `pathUtils` but I at least wanted to be clear they were doing something with the root itself).
Would an acceptable solution for this case be to cache it as a module level variable in `getBootstrapData`? while a wider solution to reparsing the bootstrap data and centralizing the routing code is created?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "antonibote (via GitHub)" <gi...@apache.org>.
antonibote commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2734091230
Hi, is there a docker image of this PR on docker hub?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2786375803
> @martyngigg @mistercrunch I fixed the Cypress tests (pretty ugly solution but I had to add wildcards to all `cy.intercept` statements - apparently it ignores baseUrl) and moved external navigations to a util function to centralize the usage of `ensureAppRoot`. There are still a few places where we use `ensureAppRoot` which I couldn't replace - for example some antd components take a relative path, but they resolve it with the origin and it's not possible to configure the base url. I suppose putting `<base>` tag in the head could solve this problem. However, this would block us from using a different base url for static assets
@kgabryje Thank you so much for looking at this and figuring out the issues with the tests that I couldn't work out. I don't really have any experience with antd but I've noticed quite a few comments like "// TODO: Remove antd" across various components - is there a long term plan to remove it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1750907836
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -78,17 +82,15 @@
csrfToken = undefined,
guestToken = undefined,
guestTokenHeaderName = 'X-GuestToken',
- unauthorizedHandler = defaultUnauthorizedHandler,
+ unauthorizedHandler = undefined,
}: ClientConfig = {}) {
const url = new URL(
host || protocol
? `${protocol || 'https:'}//${host || 'localhost'}`
- : baseUrl,
- // baseUrl for API could also be relative, so we provide current location.href
- // as the base of baseUrl
- window.location.href,
+ : '/',
+ DEFAULT_BASE_URL,
);
- this.baseUrl = url.href.replace(/\/+$/, ''); // always strip trailing slash
+ this.basePath = basePath.replace(/\/+$/, ''); // always strip trailing slash
Review Comment:
## Polynomial regular expression used on uncontrolled data
This [regular expression](1) that depends on [library input](2) may run slow on strings with many repetitions of '/'.
This [regular expression](1) that depends on [library input](3) may run slow on strings with many repetitions of '/'.
[Show more details](https://github.com/apache/superset/security/code-scanning/1228)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2572849976
Happy New Year!
I've investigated further and found the issue with "Export to PDF" & "Download image" not working on dashboards. The issue seems to be the interaction of the headless browser and `WEBDRIVER_BASEURL`. In the docker compose setup `superset_config.py` suggests setting `WEBDRIVER_BASEURL=http://superset_app:8088/`, so accessing the superset instance directly and skipping the nginx reverse proxy. This is fine when there is no url prefix set as the paths that both the reverse proxy and the superset app see are the same. When there is a url prefix set it is baked into the javascript files that webpack builds and when accessing the superset app directly the extra prefix part of the path raises an error in the superset app and the screenshot generation fails.
A fix/workaround (??) seems to be to set `WEBDRIVER_BASEURL = f"http://superset_nginx{base_path}/"` but this also requires removing this [with statement](https://github.com/apache/superset/blob/109e6c69ff25b82b603a4daff40207250788e6b1/superset/utils/urls.py#L35) that seems to set a test context that doesn't know about the prefix. I'm not sure of the knock on effects of this. @rusackas Would you know someone who could comment on this or possibly suggest an alternative approach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2573633490
> Hello ! Happy new year !
>
> > This is fine when there is no url prefix set as the paths that both the reverse proxy and the superset app see are the same. When there is a url prefix set it is baked into the javascript files that webpack builds and when accessing the superset app directly the extra prefix part of the path raises an error in the superset app and the screenshot generation fails.
>
> Well, I might have a solution for this one. I didn't get the time to look at the image generation task you proposed, but I was rather investigating around the path prefix topic. My concern was to avoid relying on a reverse-proxy for the path-prefix thing. IMO, it makes things complicated. For instance the issue you met above: dev setup (without docker compo) and prod-like setup with a reverse proxy works differently, with a different frontend where the path is hard-coded. Looking on the backend side of it, my first idea was that we maybe didn't use the Flask blueprints to their full extent, but it turns out this would not be that easy to adapt to our needs. And maybe not a clean solution.
>
> _**But**_, I stumbled across [this blog post](https://dlukes.github.io/flask-wsgi-url-prefix.html) where I discovered that WSGI protocol supports a parameter that sets such a path prefix, and that is handled on the WSGI side of it (e.g. gunicorn). I tested it (on top of your PR since we need the url_for links) and it works like a charm.
>
> **What does it change ?**
>
> Supposing you choose `analytics` as the path prefix, the backend is accessed on http://superset_app:8088/analytics/. and the reverse-proxy has a much simpler job of just routing /analytics to http://superset_app:8088/analytics/. On your above-mentioned issue, this means that the paths will concord between the front and back end, so it should work without even needing the WEBDRIVER_BASEURL fix you propose.
@jeanpommier Thanks for exploring and trying out the idea with having the backend also run on the prefix. I remember trying something like this a while ago and for some reason it didn't work properl. I think I may have been a bit hasty in dropping the idea and it was likely my lack of understanding of Superset that was the problem. I certainly agree relying on the proxy for the prefix handling is complicated. I'll try again since it certainly seems to just require an environment variable :)
> I'm still quite uncomfortable at the necessity to build the frontend to support a path other than root. I haven't made experiments yet on this, but wouldn't it be conceivable that the frontend looks for a config instruction from the backend to get this information ?
Yes this is also annoying. Now I'm a bit more familiar with the code base I wondered about including it in this https://github.com/apache/superset/blob/a986a61b5ff0c98d4e0704a1c2d7016b1e3ec721/superset/views/base.py#L333 config dictionary that is rendered into the templates? Anyone know if this is a good/bad idea?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2614959204
Loosely related -> https://github.com/apache/superset/pull/31996
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-3002952610
5.0 was cut in Feb ... took a while to stabilize and release the branch. 6.0 should be a fast-follow though, hoping around a month from now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2796091331
> It looks like the Python unit tests on HEAD of master seem to have a different failure:
>
> ```
> /opt/hostedtoolcache/Python/3.10.16/x64/lib/python3.10/site-packages/_pytest/config/__init__.py:331: PluggyTeardownRaisedWarning: A plugin raised an exception during an old-style hookwrapper teardown.
> Plugin: helpconfig, Hook: pytest_cmdline_parse
> ConftestImportFailure: ModuleNotFoundError: No module named 'wsgiref.types' (from /home/runner/work/superset/superset/tests/conftest.py)
> For more information see https://pluggy.readthedocs.io/en/stable/api_reference.html#pluggy.PluggyTeardownRaisedWarning
> config = pluginmanager.hook.pytest_cmdline_parse(
> ImportError while loading conftest '/home/runner/work/superset/superset/tests/conftest.py'.
> tests/conftest.py:41: in <module>
> from tests.integration_tests.test_app import app
> tests/integration_tests/test_app.py:20: in <module>
> from superset.app import create_app
> superset/__init__.py:20: in <module>
> from superset.app import create_app # noqa: F401
> superset/app.py:21: in <module>
> from wsgiref.types import StartResponse, WSGIApplication, WSGIEnvironment
> E ModuleNotFoundError: No module named 'wsgiref.types'
> ```
>
> Is that expected?
I think that the problem is that wsgiref types is available since Python 3.11 and master branch runs on 3.10. But I can’t understand why those tests passed in this branch though…
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "BrianJavier99 (via GitHub)" <gi...@apache.org>.
BrianJavier99 commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2720261157
Not sure if this is already reported, but:
+ > Data > Create Dataset button still doesn't reflect the prefixed URL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2783693455
@martyngigg @mistercrunch I fixed the Cypress tests and moved external navigations to a util function to centralize the usage of `ensureAppRoot`. There are still a few places where we use `ensureAppRoot` which I couldn't replace - for example some antd components take a relative path, but they resolve it with the origin and it's not possible to configure the base url.
I suppose putting `<base>` tag in the head could solve this problem. However, this would block us from using a different base url for static assets
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2752354816
> @mistercrunch Regarding the testing, what do you think of something like https://github.com/apache/superset/commit/7ccb69c31a0f37f9ff1940f00c177410998f431a
Looks like this doubles the size of the matrix, not sure if that's reasonable for a single feature... Would have to do something similar for backend tests (pytest) and normal unit tests (jest). For pytest, it's not sharded and could probably do say "mysql + app_root" and "postgres no-app-route", so it wouldn't affect the CI workload.
Exploring options here, maybe a nightly or only on `push` events on `master` would contain the workload quite a bit and allow us to catch-on-merge.
Personal preference would be "only run the extra workload" on push to `master`, which is a fraction of the work from doing it on every PR push. Curious what other committers/PMC think though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2337658818
> Thanks for tackling this... it's a popular request, no doubt about it. If you think it's relevant, we might also want to add an entry in the documentation somewhere.
Yes agreed I definitely need to add some documentation for how to use this. I wanted to wait until I was more confident that the general approach was acceptable before working on describing how to use it. In terms of a review as part of #26319 is there anything I need to do at the moment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930173044
##########
superset-frontend/src/utils/pathUtils.ts:
##########
@@ -0,0 +1,37 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { applicationRoot } from 'src/utils/getBootstrapData';
+
+/**
+ * Takes a string path to a resource and prefixes it with the application root that is
+ * defined in the application configuration. The path is unsanitized.
+ * @param path A string path to a resource
+ */
+export function ensureAppRootUnsanitized(path: string): string {
Review Comment:
I'm not wild on the names but was trying to emphasize that this does nothing to the app root itself before prefixing it (see the next comment below regarding a security warning I was trying to address).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930002094
##########
Dockerfile:
##########
@@ -62,9 +62,9 @@ RUN --mount=type=bind,source=./superset-frontend/package.json,target=./package.j
--mount=type=cache,target=/root/.cache \
--mount=type=cache,target=/root/.npm \
if [ "$DEV_MODE" = "false" ]; then \
- npm ci; \
+ npm ci; \
Review Comment:
Why removing/altering the indentation here? Guessing maybe your IDE did it? Indentation may not be perfect/consistent in Dockerfile(s) but we should keep some indent on if / else blocks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2737381286
Ok spoke with a few Preset folks and we'd like to help pushing this through, happy to help here. In order to merge this PR:
- resolve merge conflicts
- get through CI checks
- add note in `config.py` about this config flag being in BETA
- rule out the security issue raised by CodeQL, we generally need to be cautious when composing any user-provided things in URL watching for XSS. There's an opportunity for the wrapping function to sanitize urls centrally here...
- remove package-lock.json from this PR (shouldn't be affected)
- figure out how to jam this in the existing test matrixes - without doubling the number of unit tests. That means running existing matrix elements with/without APP_ROOT config. Say we we have mysql/postgres matrix, you make one instance be mysql-with-approot, postgress-without-approot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "jeanpommier (via GitHub)" <gi...@apache.org>.
jeanpommier commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2587710240
> Yes this is also annoying. Now I'm a bit more familiar with the code base I wondered about including it in superset/views/base.py#L333 `bootstrap_data` config dictionary that is rendered into the templates? Anyone know if this is a good/bad idea?
Hi @martyngigg from me point of view, it looks like the perfect place, but I'm no Superset developer.
I've been playing around quite a bit on this topic, but am still struggling about dynamic configuration of the path prefix for the frontend (i.e. without dedicated build). Did you make progress on your side ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2795186301
Debugging something funky that happening with this PR, somehow it failed when it merged on master and digging in the last commit's run here https://github.com/apache/superset/actions/runs/14382934496/job/40331066514, it appears that it skipped the python-related test, presumably because of an issue with our logic that checks which files have been touched by the PR and triggers/skips based on whether it sees the backend/frontend getting touched.
I'm guessing the issue is around the logic checking for files touched by the PR, and specific to [maybe] having a merge-based workflow instead of a rebase-type workflow.
Regardless, tests fail here and it's a bit tricky to sort this out, could use some help here to fix master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "wqbbs911 (via GitHub)" <gi...@apache.org>.
wqbbs911 commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2871099481
> For those who want to try deploying superset to k8s using the Helm chart before its official release, the following modifications are required:
>
> 1. custom image tag, I found it from github ci https://github.com/apache/superset/actions/runs/14366622555/job/40281019265 , use the py311 version, since wsgiref.types does not exist on Python 3.10, there is a pr to fix https://github.com/apache/superset/pull/33072/files#diff-9689cb75a0ebe47604d59e0a54c904040cd60b45a283a87d0f8d77cac60751d8L21
>
> ```
> image:
> repository: apachesuperset.docker.scarf.sh/apache/superset
> tag: 09b92e7-py311
> pullPolicy: IfNotPresent
> ```
>
> 2. install the required dependencies. refer to [ModuleNotFoundError: No module named 'psycopg2' during k8 installation #31431](https://github.com/apache/superset/discussions/31431), but need to use `uv` instead of `pip`
>
> ```
> bootstrapScript: |
> #!/bin/bash
>
> # Install system-level dependencies
> apt-get update && apt-get install -y \
> python3-dev \
> default-libmysqlclient-dev \
> build-essential \
> pkg-config
>
> # Install required Python packages
> uv pip install --native-tls \
> authlib \
> psycopg2-binary \
> mysqlclient
>
> # Create bootstrap file if it doesn't exist
> if [ ! -f ~/bootstrap ]; then
> echo "Running Superset with uid {{ .Values.runAsUser }}" > ~/bootstrap
> fi
> ```
>
> 3. Add `SUPERSET_APP_ROOT` env variables, and update health check paths
>
> ```
> supersetNode:
> env:
> SUPERSET_APP_ROOT: /your-path
> livenessProbe:
> httpGet:
> path: /your-path/health
> readinessProbe:
> httpGet:
> path: /your-path/health
> startupProbe:
> httpGet:
> path: /your-path/health
> ```
seems it doesn't work in 5.0.0rc2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2741058295
Thanks @mistercrunch - that's great. I've unfortunately not had any time to look back at this recently so any help would be appreciated.
I think the main things I'm struggling with at the moment are:
- "rule out the security issue raised by CodeQL"
- "figure out how to jam this in the existing test matrixes"
I can deal with the other points (looking at my calendar, likely next week) but any help on the above 2 issues would be great.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "wqbbs911 (via GitHub)" <gi...@apache.org>.
wqbbs911 commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-3002382366
5.0.0 was released, seems still doesn't work
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "ryuuc (via GitHub)" <gi...@apache.org>.
ryuuc commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2792143575
For those who want to try deploying superset to k8s using the Helm chart before its official release, the following modifications are required:
1. custom image tag, I found it from github ci https://github.com/apache/superset/actions/runs/14366622555/job/40281019265 , use the py311 version, since wsgiref.types does not exist on Python 3.10, there is a pr to fix https://github.com/apache/superset/pull/33072/files#diff-9689cb75a0ebe47604d59e0a54c904040cd60b45a283a87d0f8d77cac60751d8L21
```
image:
repository: apachesuperset.docker.scarf.sh/apache/superset
tag: 09b92e7-py311
pullPolicy: IfNotPresent
```
2. install the required dependencies. refer to https://github.com/apache/superset/discussions/31431, but need to use `uv` instead of `pip`
```
bootstrapScript: |
#!/bin/bash
# Install system-level dependencies
apt-get update && apt-get install -y \
python3-dev \
default-libmysqlclient-dev \
build-essential \
pkg-config
# Install required Python packages
uv pip install --native-tls \
authlib \
psycopg2-binary \
mysqlclient
# Create bootstrap file if it doesn't exist
if [ ! -f ~/bootstrap ]; then
echo "Running Superset with uid {{ .Values.runAsUser }}" > ~/bootstrap
fi
```
3. Add `SUPERSET_APP_ROOT` env variables, and update health check paths
```
supersetNode:
env:
SUPERSET_APP_ROOT: /your-path
livenessProbe:
httpGet:
path: /your-path/health
readinessProbe:
httpGet:
path: /your-path/health
startupProbe:
httpGet:
path: /your-path/health
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2789617400
@landryb Thanks for reporting that, should be fixed now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2796872052
@kgabryje Thanks. #33902 contains fixes for the touched files logic that meant that the tests didn't run here. #33095 contains the fixes for those tests that I hadn't realised I'd broken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2332079354
Thanks for tackling this... it's a popular request, no doubt about it. If you think it's relevant, we might also want to add an entry in the documentation somewhere.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2557290471
Dear all,
Thank you so much for taking the time to test out these changes and I apologise (again!) for the lack of response to any comments. @ravikantkml Thanks for the PR fixing the menu links - I think that looks okay and I'll merge it.
I think the remaining issues are fixing the issues with screenshot generation. I had a brief look at this today and I'm having trouble getting them working even without any prefixed path set - I must have an issue in my development setup.
Today is my last day of work before Christmas break but I will have time when I return to pick this up properly. I'll aim to get it to "Ready for Review" as soon as possible after the holidays. @jeanpommier Thanks for the offer of help too - If you wanted to try and tackle the screenshot generation problem then that would be super helpful but equally I'm happy to look at it after the break.
Happy Holidays! 🎅🏻
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "harri2012 (via GitHub)" <gi...@apache.org>.
harri2012 commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2507460290
Very good, looking forward to merging the code soon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "landryb (via GitHub)" <gi...@apache.org>.
landryb commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2565454323
testing the PR here outside of the docker context (eg pip install from that github branch branch, rebuild the assets with BASE_PATH set), that work is much welcome.
some notes so far.. i think it should be mentioned that APP_ICON and FAVICON needs BASE_PATH prepended, otherwise it generates 404's here with the default value from `config.py`
looking a bit more at the code/PR, all assets below STATIC_ASSETS_PREFIX also need to default to BASE_PATH ? that fixes the FAVICON while here.
with this config the superset homepage loads fine with all its assets/images/favicon:
```
BASE_PATH = "/dashboards"
APP_ICON = BASE_PATH + "/static/assets/images/superset-logo-horiz.png"
STATIC_ASSETS_PREFIX = BASE_PATH
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2602969229
Korbit doesn't automatically review large (500+ lines changed) pull requests such as this one. If you want me to review anyway, use `/korbit-review`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2598763722
> > Yes this is also annoying. Now I'm a bit more familiar with the code base I wondered about including it in superset/views/base.py#L333 `bootstrap_data` config dictionary that is rendered into the templates? Anyone know if this is a good/bad idea?
>
> Hi @martyngigg from me point of view, it looks like the perfect place, but I'm no Superset developer.
>
> I've been playing around quite a bit on this topic, but am still struggling about dynamic configuration of the path prefix for the frontend (i.e. without dedicated build). Did you make progress on your side ?
Hi @jeanpommier,
I've managed to make a little progress with this. Firstly I've improved the situation with the backend such that its no longer the reverse-proxy that strips the prefix. I've added a middleware on the Flask app that does it so now the prefix is understood by the application so it works with the Flask dev server, gunicorn and Nginx. [Changeset](https://github.com/martyngigg/superset/commit/a84951a1e078ac8bf6bf4cdd90662dacce144cf6)
Secondly, I've got a work in progress [commit](https://github.com/martyngigg/superset/commit/328b0adc9be8416fe22524ba9f2bb33f090a7784) that attempts to push the `application_root` through the bootstrap data now that the app knows about it. It mostly works with the exception of issues with stylesheet imports through webpack that generate the wrong path. I need to explore further but the webpack [docs](https://github.com/webpack/docs/wiki/configuration#outputpublicpath) suggest `__webpack_public_path__` might be a way to go.
and also improved the backed to remove the need for stripping. I've successfully implemented a small middleware class in the backend
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1925641965
##########
docker/.env:
##########
@@ -54,8 +54,9 @@ REDIS_HOST=redis
REDIS_PORT=6379
FLASK_DEBUG=true
+SUPERSET_APP_ROOT="/"
SUPERSET_ENV=development
-SUPERSET_LOAD_EXAMPLES=yes
+SUPERSET_LOAD_EXAMPLES=no
Review Comment:
Nice catch. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1931312172
##########
superset-frontend/src/utils/getBootstrapData.ts:
##########
@@ -25,3 +26,17 @@ export default function getBootstrapData(): BootstrapData {
const dataBootstrap = appContainer?.getAttribute('data-bootstrap');
return dataBootstrap ? JSON.parse(dataBootstrap) : DEFAULT_BOOTSTRAP_DATA;
}
+
+// eslint-disable-next-line import/no-mutable-exports
+const APPLICATION_ROOT_NO_TRAILING_SLASH =
+ getBootstrapData().common.application_root.replace(/\/$/, '');
+
+/**
+ * @param dom_sanitize If true, run the application root through dompurify before returning
+ * @returns The configured application root
+ */
+export function applicationRoot(dom_sanitize = false): string {
+ return dom_sanitize
+ ? DOMPurify.sanitize(APPLICATION_ROOT_NO_TRAILING_SLASH)
Review Comment:
Not a security expert but from my understanding CodeQL is worried about dom-injection-type attacks, which probably don't apply to our usage in most cases (?). Pretty sure it wouldn't apply to the APP_ROOT that's coming from a trusted place (served by the backend) That would only apply in some specific cases. I'd assume that React anchor tags and react-router would already take care of that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "watercraft (via GitHub)" <gi...@apache.org>.
watercraft commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2536539163
For whom it may concern, I'm testing with a port of this work to 3.1.1rc1 available from watercraft:superset branch prefix
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2795219271
@martyngigg anyway you can fix the issues that landed through this PR on `master`? I tried and fix some but didn't have full context. Would prefer not reverting this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2750561405
> Not sure if this is already reported, but: 
>
> `+ > Data > Create Dataset` button still doesn't reflect the prefixed URL
Sorry for the delay in reply.
The latest push should have fixed this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r2038270948
##########
superset/views/dashboard/views.py:
##########
@@ -86,7 +85,11 @@ def new(self) -> FlaskResponse:
)
db.session.add(new_dashboard)
db.session.commit() # pylint: disable=consider-using-transaction
- return redirect(f"/superset/dashboard/{new_dashboard.id}/?edit=true")
+ return redirect(
+ url_for(
+ "Superset.dashboard", dashboard_id_or_slug=new_dashboard.id, edit=True
Review Comment:
Somehow this line broke some tests on `master` but I don't understand how it didn't get caught by CI (????).
Issues is with the capitalize `True` here... Maybe you IDE did this? Linter?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2796085534
It looks like the Python unit tests on HEAD of master seem to have a different failure:
```
/opt/hostedtoolcache/Python/3.10.16/x64/lib/python3.10/site-packages/_pytest/config/__init__.py:331: PluggyTeardownRaisedWarning: A plugin raised an exception during an old-style hookwrapper teardown.
Plugin: helpconfig, Hook: pytest_cmdline_parse
ConftestImportFailure: ModuleNotFoundError: No module named 'wsgiref.types' (from /home/runner/work/superset/superset/tests/conftest.py)
For more information see https://pluggy.readthedocs.io/en/stable/api_reference.html#pluggy.PluggyTeardownRaisedWarning
config = pluginmanager.hook.pytest_cmdline_parse(
ImportError while loading conftest '/home/runner/work/superset/superset/tests/conftest.py'.
tests/conftest.py:41: in <module>
from tests.integration_tests.test_app import app
tests/integration_tests/test_app.py:20: in <module>
from superset.app import create_app
superset/__init__.py:20: in <module>
from superset.app import create_app # noqa: F401
superset/app.py:21: in <module>
from wsgiref.types import StartResponse, WSGIApplication, WSGIEnvironment
E ModuleNotFoundError: No module named 'wsgiref.types'
```
Is that expected?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2799041769
Reporting that in `master`, simply clicking on "Datasets" in the navbar at the top leads to a 404. Same with `SQL Lab`, `Saved Queries` and many others:
<img width="1252" alt="Screenshot 2025-04-12 at 2 14 41 PM" src="https://github.com/user-attachments/assets/21fbe78a-3ff9-4b5b-ae78-5fb7d946a084" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "landryb (via GitHub)" <gi...@apache.org>.
landryb commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-3036299762
this has been merged a while ago and is working fine, but i think the swagger link doesnt have the prefix, eg going to https://instance/path/swagger/v1 tries to fetch `/api/v1/_openapi` without the prefix (which should be coming from `application_root` afaict), which yields a 404.
looking at the code but so far i fail to see where this should be configured/handled, eg in FAB or superset frontend itself or somewhere else...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2737338312
@ Ephemeral environment creation failed. Please check the Actions logs for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "jeanpommier (via GitHub)" <gi...@apache.org>.
jeanpommier commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-3164822423
Hi @Ayoub-BOUCHACHIA
I don't think it's been ported to 4.1.2.
Actually, AFAIK, it's not part of any release at the current time: it's been merged on master branch, but not released yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1956008268
##########
superset-frontend/src/components/Tags/Tag.tsx:
##########
@@ -70,7 +71,7 @@
<StyledTag data-test="tag" key={id} onClick={onClick} role={whatRole}>
{id ? (
<a
- href={`/superset/all_entities/?id=${id}`}
+ href={ensureAppRoot(`/superset/all_entities/?id=${id}`)}
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1272)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2360022029
I think this may be quite an undertaking and might have to be tackled in phases. Would love to see this done :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2414175050
@rusackas I think this is ready for review but in terms of it being related to [SIP-112](https://github.com/apache/superset/issues/26319) is that the right thing to do in terms of the process?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "ascendlin (via GitHub)" <gi...@apache.org>.
ascendlin commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2445990294
When clicking on the user list or role list, and then clicking on datasets, a 404 error appears.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2442145103
If you think it's ready for review, I'd say you can go ahead and move it from Draft to "Ready for Review" - though it looks like CI might need a little more love still.
As for the SIP, since @sfirke reopened the GitHub issue, I set the SIP itself back to "pre-discussion" state for its official consensus. Did you want to re-kindle that pre-existing SIP, or would it be easier to open a new one? I'm happy to help you carry that SIP through to fruition if you'd like. Feel free to DM me on [slack](http://bit.ly/join-superset-slack) if it's easier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2750830700
>
> > "figure out how to jam this in the existing test matrixes"
>
> Someone can help with this, maybe simply hooking this up env vars. Say if `superset/config.py` reads from `SUPERSET__URL_PREFIX` (or similar), we can probably easily dynamically parameterize this in the GHA matrixes. I think we have an example somewhere of JSON-combination of matrix parameter that aren't a full cartesian product.
>
> Dug out the similar use case using a dynamicaly-parameterized matrix here -> https://github.com/apache/superset/blob/master/.github/workflows/docker.yml#L18-L27
@mistercrunch Regarding the testing, what do you think of something like https://github.com/apache/superset/pull/30134/commits/7ccb69c31a0f37f9ff1940f00c177410998f431a
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2737285185
@eschutho Processing your ephemeral environment request [here](https://github.com/apache/superset/actions/runs/13951608876). Action: **up**. More information on [how to use or configure ephemeral environments](https://superset.apache.org/docs/contributing/howtos/#github-ephemeral-environments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1930020931
##########
superset-frontend/src/utils/getBootstrapData.ts:
##########
@@ -25,3 +26,17 @@ export default function getBootstrapData(): BootstrapData {
const dataBootstrap = appContainer?.getAttribute('data-bootstrap');
return dataBootstrap ? JSON.parse(dataBootstrap) : DEFAULT_BOOTSTRAP_DATA;
}
+
+// eslint-disable-next-line import/no-mutable-exports
+const APPLICATION_ROOT_NO_TRAILING_SLASH =
+ getBootstrapData().common.application_root.replace(/\/$/, '');
+
+/**
+ * @param dom_sanitize If true, run the application root through dompurify before returning
+ * @returns The configured application root
+ */
+export function applicationRoot(dom_sanitize = false): string {
+ return dom_sanitize
+ ? DOMPurify.sanitize(APPLICATION_ROOT_NO_TRAILING_SLASH)
Review Comment:
I'm not sure if i understand why we need to sanitize the app route every time we read it(?) Seems it'd be a very static thing(?) Do we have trust issues with the bootstrap data? Also concerned about parsing/reparsing bootstrap data every time we read print a URL. Assuming just the basic menu has dozens of links, we'd be running `JSON.parse` for each one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "landryb (via GitHub)" <gi...@apache.org>.
landryb commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2790028286
> @landryb Thanks for reporting that, should be fixed now
thanks a lot, i've locally applied d463c0027aa and it fixes the broken links i had seen
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2722562653
> Not sure if this is already reported
Yeah my main worry with this PR is that it doesn't put controls in place to make sure all urls use the prefix function on the backend and frontend. Wondering how we can best achieve this. Some ideas:
- write custom lint rules (`eslint` and `ruff`) looking for URLy strings and erroring
- run e2e tests with the prefix, now we might need this in a matrix of tests somehow because we want to confirm that tests pass with AND without a prefix set in config
- good old code review
- ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "jeanpommier (via GitHub)" <gi...@apache.org>.
jeanpommier commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2607763498
Not a review, but at first look, it looks really neat. And quite some work done here !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "Dinesh-4320 (via GitHub)" <gi...@apache.org>.
Dinesh-4320 commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2493109022
The solution really works. Solved a long term issue in our deployment behind proxies. Hoping to get merged soon with official code base
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "Ayoub-BOUCHACHIA (via GitHub)" <gi...@apache.org>.
Ayoub-BOUCHACHIA commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-3164413819
I'm using Superset 4.1.2, but the solution isn't working for me. Can anyone explain why?
Here’s the relevant output from my logs:
```bash
[notice] A new release of pip is available: 25.0.1 -> 25.2
[notice] To update, run: pip install --upgrade pip
Starting web app (using development server)...
Usage: flask run [OPTIONS]
Try 'flask run --help' for help.
Error: The factory "create_app(superset_app_root='/archipel')" in module 'superset' could not be called with the specified arguments.
+ '[' 0 -eq 1 ']'
+ [[ 1 -gt 0 ]]
+ [[ dev == \d\e\v ]]
+ echo 'Skipping gunicorn, running app in dev mode'
+ bash /app/docker/docker-bootstrap.sh app
Skipping gunicorn, running app in dev mode
Installing local overrides at /app/docker/requirements-local.txt
Requirement already satisfied: psycopg2-binary==2.9.6 in /usr/local/lib/python3.10/site-packages (from -r /app/docker/requirements-local.txt (line 1)) (2.9.6)
WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager, possibly rendering your system unusable. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv. Use the --root-user-action option if you know what you are doing and want to suppress this warning.
[notice] A new release of pip is available: 25.0.1 -> 25.2
[notice] To update, run: pip install --upgrade pip
Starting web app (using development server)...
Usage: flask run [OPTIONS]
Try 'flask run --help' for help.
Error: The factory "create_app(superset_app_root='/archipel')" in module 'superset' could not be called with the specified arguments.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1766191485
##########
docker/docker-frontend.sh:
##########
@@ -17,19 +17,23 @@
#
set -e
+# zstd exe is required by simple-zstd package
+apt-get update
+apt-get install -y zstd
Review Comment:
there's a recent fix in master for this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2789808093
@martyngigg This broke only in FAB views - in those, the Menu is server side rendered
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "landryb (via GitHub)" <gi...@apache.org>.
landryb commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2788694820
testing this in a downstream project which got rebased on the last commits from this branch, superset being deployed under `/dashboards/`, when im on https://FQDN/dashboards/logmodelview/list/ the sqllab entry in the `SQL` menu points at `/sqllab/` instead of `/dashboards/sqllab/`
that's the same issue for the other entries in the `SQL`menu, and for the `/tablemodelview/list/` entry in the menu, they all lack the configured prefix. The two leftmost entries in the menu have the correct prefix in their link (so clicking on them lead to the right page).
i'm seeing the same issue when im on the other entries from the settings menu, eg if on `/dashboards/users/list/` the same sqllab/tablemodelview links lack the prefix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2795217033
Related: https://github.com/apache/superset/pull/33092
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r2011635990
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -325,7 +326,11 @@
</Menu.Item>
) : (
<Menu.Item key={item.name} css={styledChildMenu}>
- {item.url ? <a href={item.url}> {item.label} </a> : item.label}
+ {item.url ? (
+ <a href={ensureAppRoot(item.url)}> {item.label} </a>
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/2001)
##########
superset-frontend/src/components/Tags/Tag.tsx:
##########
@@ -81,7 +82,7 @@
{' '}
{id ? (
<a
- href={`/superset/all_entities/?id=${id}`}
+ href={ensureAppRoot(`/superset/all_entities/?id=${id}`)}
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/2000)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2741230081
> "rule out the security issue raised by CodeQL"
Wondering if it'd make sense to generalize a `urlFor(url, sanitize=True)` (renaming the method to match the backend (?)) which would sanitize by default. Could expose other functionality like receiving an object of `params` and be able to url-encode to rison VS standard-querystring - not sure if it's in-scope for this PR or not...
> "figure out how to jam this in the existing test matrixes"
Someone can help with this, thinking we'll need the ability to set a `superset_config.py` in CI, maybe simply hooking up env vars. Say if `superset/config.py` reads from `SUPERSET__URL_PREFIX` (or similar), we can probably easily dynamically parameterize this in the matrix. I think we have an example somewhere of JSON-combination of matrix parameter that aren't a full cartesian product.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2799933758
> Reporting that in `master`, simply clicking on "Datasets" in the navbar at the top leads to a 404. Same with `SQL Lab`, `Saved Queries` and many others: <img alt="Screenshot 2025-04-12 at 2 14 41 PM" width="1252" src="https://private-user-images.githubusercontent.com/487433/433086113-21fbe78a-3ff9-4b5b-ae78-5fb7d946a084.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NDQ1NDczNzEsIm5iZiI6MTc0NDU0NzA3MSwicGF0aCI6Ii80ODc0MzMvNDMzMDg2MTEzLTIxZmJlNzhhLTNmZjktNGI1Yi1hZTc4LTVmYjdkOTQ2YTA4NC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwNDEzJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDQxM1QxMjI0MzFaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1mMGIwZGJkY2JjYzYwOWYxODZkMmUxOThmMTNmMGY4NzQ0NmY3ZDNjYTA4YjZjYTE5NzE0NWMzMGI5ZTA2NTNmJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.mN2tYbpK_EqhlTTUyQaHNMa-7_8faqmvMsh3FoSJtdo
">
Opened a fix here https://github.com/apache/superset/pull/33114
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2702818018
@surajsrivathsa , looks like your frontend build didn't work, can you fire up `docker-compose up --build` on `master` and see if that works? Another trick to rule out local setups/modifications is to start fresh in a new folder with a fresh `git clone`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #30134:
URL: https://github.com/apache/superset/pull/30134#discussion_r1925566521
##########
superset-frontend/src/features/home/ChartTable.tsx:
##########
@@ -193,7 +194,7 @@
),
buttonStyle: 'tertiary',
onClick: () => {
- window.location.assign('/chart/add');
+ window.location.assign(ensureAppRootSanitized('/chart/add'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1255)
##########
superset-frontend/src/features/home/DashboardTable.tsx:
##########
@@ -196,19 +197,20 @@
),
buttonStyle: 'tertiary',
onClick: () => {
- window.location.assign('/dashboard/new');
+ window.location.assign(ensureAppRootSanitized('/dashboard/new'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1256)
##########
superset-frontend/src/features/home/EmptyState.tsx:
##########
@@ -171,7 +173,7 @@
<Button
buttonStyle="primary"
onClick={() => {
- window.location.href = favRedirects[tableName];
+ window.location.href = ensureAppRootSanitized(favRedirects[tableName]);
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1257)
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -461,7 +462,7 @@
{menu.label}
</Link>
) : (
- <a href={menu.url}>
+ <a href={ensureAppRootSanitized(menu.url || '')}>
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1258)
##########
superset-frontend/src/pages/AnnotationLayerList/index.tsx:
##########
@@ -277,7 +278,7 @@
};
const onLayerAdd = (id?: number) => {
- window.location.href = `/annotationlayer/${id}/annotation`;
+ window.location.href = ensureAppRootSanitized(`/annotationlayer/${id}/annotation`);
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1261)
##########
superset-frontend/src/dashboard/components/SaveModal.tsx:
##########
@@ -154,7 +155,9 @@
} else {
this.onSave(data, dashboardId, saveType).then((resp: JsonResponse) => {
if (saveType === SAVE_TYPE_NEWDASHBOARD && resp.json?.result?.id) {
- window.location.href = `/superset/dashboard/${resp.json.result.id}/`;
+ window.location.href = ensureAppRootSanitized(
+ `/superset/dashboard/${resp.json.result.id}/`,
+ );
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1254)
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -496,7 +497,9 @@
{isFrontendRoute(child.url) ? (
<Link to={child.url || ''}>{menuItemDisplay}</Link>
) : (
- <a href={child.url}>{menuItemDisplay}</a>
+ <a href={ensureAppRootSanitized(child.url || '')}>
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1259)
##########
superset-frontend/src/pages/DashboardList/index.tsx:
##########
@@ -682,7 +683,7 @@
),
buttonStyle: 'primary',
onClick: () => {
- window.location.assign('/dashboard/new');
+ window.location.assign(ensureAppRootSanitized('/dashboard/new'));
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1262)
##########
superset-frontend/src/features/home/SavedQueries.tsx:
##########
@@ -264,7 +266,7 @@
name: t('View All »'),
buttonStyle: 'link',
onClick: () => {
- window.location.href = '/savedqueryview/list';
+ window.location.href = ensureAppRootSanitized('/savedqueryview/list');
Review Comment:
## DOM text reinterpreted as HTML
[DOM text](1) is reinterpreted as HTML without escaping meta-characters.
[Show more details](https://github.com/apache/superset/security/code-scanning/1260)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: Allow superset to be deployed under a prefixed URL [superset]
Posted by "martyngigg (via GitHub)" <gi...@apache.org>.
martyngigg commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2624698921
> Hi @martyngigg I believe you might have missed some places in the frontend: This is kind of a hedge case, but if you edit a dashboard, go to "save as", select overwrite, it is redirecting to /superset/dashboard/yourdashslug after you save a dashboard edit (no prefix anymore)
>
> I believe it might be due to https://github.com/martyngigg/superset/blob/allow_prefixed_paths/superset-frontend/src/dashboard/actions/dashboardState.js#L410 messing up the browser history also, at some point.
>
> Reproduce it:
>
> * edit a dashboard
>
> * save as
>
> * overwrite
>
> * you get redirected to the prefix-less path
Thanks! I think I've fixed this now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: Allow superset to be deployed under a prefixed URL [superset]
Posted by "ravikantkml (via GitHub)" <gi...@apache.org>.
ravikantkml commented on PR #30134:
URL: https://github.com/apache/superset/pull/30134#issuecomment-2468415844
> > > > > Thanks for tackling this... it's a popular request, no doubt about it. If you think it's relevant, we might also want to add an entry in the documentation somewhere.
> > > >
> > > >
> > > > Yes agreed I definitely need to add some documentation for how to use this. I wanted to wait until I was more confident that the general approach was acceptable before working on describing how to use it. In terms of a review as part of #26319 is there anything I need to do at the moment?
> > >
> > >
> > > Thanks for these changes. Can you please mention values for BASE_PATH and ASSET_BASE_URL. Just wanted to check these changes with superset and nginx.
> >
> >
> > Sorry for the delay in replying to this. I've had zero time to spend on this lately.
> > The latest commits rebase on the current master and I have also added some documentation to the configuring superset pages that describe the new variables with an example `nginx.conf`. Does that help?
>
> Thanks a lot for the response. We tried the changes as recommended and observed that most of the things are working when we specify prefix. However there are few things where prefix is not applied correctly as shown in the image below (chart menu has prefix applied correctly) 
>
> Also dashboard menu items export to PDF/image not working (Dashboard -> Download -> Export to PDF/Download as Image)
I tried to fix above issue of missing prefixes in top-level menu. Please check this PR [2](https://github.com/martyngigg/superset/pull/2)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org