codeant-ai-for-open-source[bot] commented on code in PR #35021: URL: https://github.com/apache/superset/pull/35021#discussion_r2896679679
########## superset-frontend/src/pages/RedirectWarning/index.tsx: ########## @@ -0,0 +1,175 @@ +/** + * 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 { useState, useMemo, useCallback, useEffect } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { css, styled, useTheme } from '@apache-superset/core/theme'; +import { + Button, + Card, + Checkbox, + Flex, + Typography, +} from '@superset-ui/core/components'; +import { Icons } from '@superset-ui/core/components/Icons'; +import { getTargetUrl, isUrlTrusted, trustUrl, isAllowedScheme } from './utils'; + +const PageContainer = styled(Flex)` + ${({ theme }) => css` + height: calc(100vh - 64px); + background-color: ${theme.colorBgLayout}; + padding: ${theme.padding}px; + `} +`; + +const WarningCard = styled(Card)` + ${({ theme }) => css` + max-width: 520px; + width: 100%; + box-shadow: ${theme.boxShadowSecondary}; + `} +`; + +const WarningHeader = styled(Flex)` + ${({ theme }) => css` + padding: ${theme.paddingLG}px ${theme.paddingXL}px; + border-bottom: 1px solid ${theme.colorBorderSecondary}; + `} +`; + +const WarningBody = styled.div` + ${({ theme }) => css` + padding: ${theme.paddingXL}px; + `} +`; + +const UrlDisplay = styled(Flex)` + ${({ theme }) => css` + background-color: ${theme.colorFillQuaternary}; + border-radius: ${theme.borderRadiusSM}px; + padding: ${theme.paddingSM}px ${theme.padding}px; + margin-bottom: ${theme.margin}px; + `} +`; + +const UrlText = styled(Typography.Text)` + ${({ theme }) => css` + font-family: ${theme.fontFamilyCode}; + font-size: ${theme.fontSize}px; + word-break: break-all; + `} +`; + +const WarningFooter = styled(Flex)` + ${({ theme }) => css` + padding: ${theme.padding}px ${theme.paddingXL}px; + background-color: ${theme.colorFillAlter}; + border-top: 1px solid ${theme.colorBorderSecondary}; + `} +`; + +const WarningTitle = styled(Typography.Title)` + && { + margin: 0; + } +`; + +export default function RedirectWarning() { + const theme = useTheme(); + const [trustChecked, setTrustChecked] = useState(false); + + const targetUrl = useMemo(() => getTargetUrl(), []); + + // Redirect immediately if the URL is already trusted + useEffect(() => { + if (targetUrl && isAllowedScheme(targetUrl) && isUrlTrusted(targetUrl)) { + window.location.href = targetUrl; + } + }, [targetUrl]); + + const handleContinue = useCallback(() => { + if (!targetUrl || !isAllowedScheme(targetUrl)) return; + if (trustChecked) { + trustUrl(targetUrl); + } + window.location.href = targetUrl; + }, [trustChecked, targetUrl]); + + const handleReturn = useCallback(() => { + window.location.href = '/'; + }, []); Review Comment: **Suggestion:** The "Return to Superset" button currently sends users to the site root path `'/'`, which may not correspond to the Superset application (especially in deployments where Superset is mounted under a subpath), causing users to land on the wrong page instead of the Superset home screen; using the known Superset home route (e.g., `'/superset/welcome/'`) aligns with the rest of the app routing and avoids incorrect navigation. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ "Return to Superset" misroutes in subdirectory deployments. - ⚠️ Users leaving warning page may not reach Superset home. ``` </details> ```suggestion const handleReturn = useCallback(() => { window.location.href = '/superset/welcome/'; }, []); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Deploy Superset with an app root and/or subdirectory prefix, as exercised in `tests/integration_tests/test_subdirectory_deployments.py` (e.g. Superset served under `/analytics/superset`, see lines 105–142 where `/superset/welcome/` becomes `/analytics/superset/welcome/` via `AppRootMiddleware`). 2. Trigger an alerts/reports email containing an external link so that the backend redirect endpoint `/redirect/` (implemented in `superset/views/redirect.py:44-76`) is hit with a URL parameter pointing to an external site (e.g. `/analytics/superset/redirect/?url=https%3A%2F%2Fevil.com`). 3. For an external URL, `RedirectView.redirect_warning` at `superset/views/redirect.py:55-76` calls `render_app_template`, which loads the React frontend route `/redirect/` mapped to the `RedirectWarning` component in `superset-frontend/src/views/routes.tsx:183-201`. 4. On the `RedirectWarning` page (`superset-frontend/src/pages/RedirectWarning/index.tsx:93-175`), click the "Return to Superset" button rendered at lines 166–168, which invokes `handleReturn` defined at lines 114–116 and executes `window.location.href = '/'`. In a subdirectory deployment this sends the browser to the site root (`/`), not the Superset home route (e.g. `/analytics/superset/welcome/`), leaving the user on the wrong page instead of the Superset welcome screen. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/pages/RedirectWarning/index.tsx **Line:** 114:116 **Comment:** *Logic Error: The "Return to Superset" button currently sends users to the site root path `'/'`, which may not correspond to the Superset application (especially in deployments where Superset is mounted under a subpath), causing users to land on the wrong page instead of the Superset home screen; using the known Superset home route (e.g., `'/superset/welcome/'`) aligns with the rest of the app routing and avoids incorrect navigation. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35021&comment_hash=c2a1f198a5b55d01982bebabe535714905ae6cf7542ce95863b55300d465fb69&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F35021&comment_hash=c2a1f198a5b55d01982bebabe535714905ae6cf7542ce95863b55300d465fb69&reaction=dislike'>👎</a> -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
