codeant-ai-for-open-source[bot] commented on PR #35098: URL: https://github.com/apache/superset/pull/35098#issuecomment-3606192253
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/35098/files#diff-d44093359c09b08683ec45645adbeb7f263bf7ca6228df6482877efb4348cb04R141-R145'><strong>Reverse tabnabbing</strong></a><br>The code opens a new tab with window.open(makeUrl(...), '_blank') without using noopener/noreferrer or nulling window.opener. This can allow the opened page to access and manipulate the opener window (reverse tabnabbing). Consider mitigating by using noopener/noreferrer or setting opener to null after opening.<br> - [ ] <a href='https://github.com/apache/superset/pull/35098/files#diff-9689cb75a0ebe47604d59e0a54c904040cd60b45a283a87d0f8d77cac60751d8R78-R86'><strong>Unsafe type assumptions</strong></a><br>The patch assumes `app.config["THEME_DEFAULT"]` / `THEME_DARK` are mappings and that `theme.get("token", {})` returns a mapping with `.get`. If theme values are None or not dict-like, calling `.get` will raise. Validate types before using `.get` and before mutating nested structures.<br> - [ ] <a href='https://github.com/apache/superset/pull/35098/files#diff-75c4dde19b5a226c70d5a547f1df12090accf12162c97b69f31a07febe36e713R26-R28'><strong>Double-prefix / double-slash risk</strong></a><br>The current concatenation can produce '//' or duplicate subdirectory prefixes when `applicationRoot()` ends with '/' or when the caller already passes a path that already includes the app root. This may lead to incorrect URLs or broken static asset references in subdirectory deployments.<br> - [ ] <a href='https://github.com/apache/superset/pull/35098/files#diff-ab057099067874ff6f10f77be2e3e6ed55ba08b7f743a40d5f3905fb836a7ee0R35-R47'><strong>Prefix boundary check</strong></a><br>The code checks `not url.startswith(app_root)` to avoid double-prefixing, but `startswith` can yield false positives (e.g. app_root='/superset' and url='/superset2/...'), causing incorrect behavior. Validate prefix boundaries precisely (match exact segment or trailing slash) to avoid skipping needed prefixes or skipping when unrelated paths share the same initial substring.<br> - [ ] <a href='https://github.com/apache/superset/pull/35098/files#diff-75c4dde19b5a226c70d5a547f1df12090accf12162c97b69f31a07febe36e713R42-R44'><strong>Full URLs / protocol-relative URLs</strong></a><br>`ensureAppRoot` / `makeUrl` always prefix the provided path. If a full URL (e.g. 'http://...', 'https://...', or protocol-relative '//...') or a data URI is passed, it should not be prefixed. Otherwise external links will be corrupted.<br> - [ ] <a href='https://github.com/apache/superset/pull/35098/files#diff-ab057099067874ff6f10f77be2e3e6ed55ba08b7f743a40d5f3905fb836a7ee0R45-R47'><strong>Normalization of APPLICATION_ROOT</strong></a><br>`app_root` is used with `rstrip("/")` later but the code does not normalize leading/trailing slashes before checks. If `APPLICATION_ROOT` is configured with or without a leading/trailing slash (e.g., 'superset' vs '/superset/'), behavior may be inconsistent. Ensure `app_root` is normalized (leading slash present, no trailing slash) before checks/concatenation.<br> - [ ] <a href='https://github.com/apache/superset/pull/35098/files#diff-9689cb75a0ebe47604d59e0a54c904040cd60b45a283a87d0f8d77cac60751d8R71-R76'><strong>Path normalization</strong></a><br>The code concatenates `app_root` and paths (e.g. `app.config['APP_ICON']`, theme token URLs) without normalizing `app_root`. If `app_root` contains or lacks leading/trailing slashes unexpectedly (e.g. "superset", "/superset/", "/superset"), this can produce double slashes or missing slashes in generated URLs. Normalize `app_root` (ensure leading slash, remove trailing slash) before using it for prefixing.<br> - [ ] <a href='https://github.com/apache/superset/pull/35098/files#diff-9eae68e91246131eff0f5f2aa591a54745ec80dd2375f108ccb8f961b16b2a93R44-R44'><strong>Import side-effects</strong></a><br>The new top-level import of `makeUrl` brings in `src/utils/pathUtils` at module load time. That module may read bootstrap data from the DOM (or perform other side effects) during import which can cause test flakiness or runtime errors (SSR or widget initialization ordering). Consider lazy/dynamic importing or otherwise avoiding side-effectful module initialization at module scope.<br> </td></tr> </table> -- 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]
