codeant-ai-for-open-source[bot] commented on PR #36771: URL: https://github.com/apache/superset/pull/36771#issuecomment-3712155135
## 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/36771/files#diff-85dd80f8a0a6e0d25123012606004a7780f5bd08444360a3c9caa68e58c75f65R51-R51'><strong>Query parameter encoding</strong></a><br>The new endpoint builds the query string as `?q=${rison.encode(ids)}` without URL-encoding the Rison payload. If the Rison output contains characters that are not safe in URLs, requests may fail or be routed incorrectly. Use percent-encoding for the query component.<br> - [ ] <a href='https://github.com/apache/superset/pull/36771/files#diff-85dd80f8a0a6e0d25123012606004a7780f5bd08444360a3c9caa68e58c75f65R46-R57'><strong>URL prefixing assumption</strong></a><br>The change removed `ensureAppRoot` and relies on `SupersetClient.get()` to apply the app root prefix. Confirm that `SupersetClient.get()` always applies the expected prefix for all deployment configurations to avoid both double-prefixing or missing-prefix issues. Validate behavior across subdirectory deployments and CI tests.<br> - [ ] <a href='https://github.com/apache/superset/pull/36771/files#diff-21ada70fceb2dbfeb03eb7944bab78e2faeaece9767eea63dd0462a1419e6647R49-R76'><strong>App-root normalization</strong></a><br>The new `ensureUrlPrefix` uses `applicationRoot()` directly and treats any non-empty value as a prefix to apply. If `applicationRoot()` returns '/' (root) or returns a value with a trailing slash (e.g. '/superset/'), the current checks may mis-detect already-prefixed URLs or cause incorrect prefixing (or double slashes). Normalize `applicationRoot()` (handle '/' as no-op and strip trailing slashes) before performing startsWith checks and before calling `makeUrl()`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36771/files#diff-407b1e72d7f4462c85eca33d215cd9981b18db050f649dfaaac5748a262d09b8R423-R423'><strong>Double-prefix risk</strong></a><br>The PR now calls `makeUrl('/api/v1/sqllab/export_streaming/')` before passing the URL into `startExport`. `useStreamingExport` implements a URL-prefix guard that also prefixes unprefixed relative URLs. Verify that the guard correctly detects already-prefixed URLs to avoid double-prefixing in all edge cases (query string, hash, sibling paths). Also confirm behavior is consistent with other places that call `startExport`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36771/files#diff-d456718de2ea528bab95389aa260a5551dc20bd6fd60bb481e2a2b6f33ecae7eR19-R54'><strong>Global test state</strong></a><br>The test file mutates several globals (TextEncoder/TextDecoder, URL.createObjectURL/revokeObjectURL, and `global.fetch`) at module scope. This can leak state between tests (or packages) run in the same Jest process and cause surprising interactions. Prefer setting/mocking globals in per-test lifecycle hooks (beforeEach/afterEach) or using jest.spyOn so they can be restored.<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]
