codeant-ai-for-open-source[bot] commented on PR #36390: URL: https://github.com/apache/superset/pull/36390#issuecomment-3605270547
## 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/36390/files#diff-bfad2751d38e1a043dc3ad8c140fc2e4cdf8345c0df42c744edbdd5e721d29cbR44-R48'><strong>Security Issue</strong></a><br>MQTT broker URL, username, and password are hard-coded in a frontend bundle, which effectively exposes broker credentials to any user of the dashboard. These values should be externalized and protected (e.g., via backend proxy or server-side config) rather than shipped to the browser.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-90a7083a20beba74b4e1f11ab248fba1ab6466b08f167cee3d092c727dcc0b68R149-R153'><strong>Sensitive Data</strong></a><br>MQTT broker URL, username, and especially password are hard-coded in frontend code, meaning they will be shipped to every browser and can be trivially extracted. The reviewer should confirm this is acceptable for the deployment or move these values to a secure backend or configuration layer.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-ec4bd7cb15a3170ce228455f2d838ae95f87c9efc9e3ff24fb5f4ace29b599ecR37-R39'><strong>Sensitive Config</strong></a><br>The `DashboardAlertsMeta` type includes MQTT credentials (`mqttUsername`, `mqttPassword`) as part of the dashboard layout metadata, which is typically persisted and sent to the browser. This may unintentionally expose broker credentials to all users who can view the dashboard. The reviewer should validate whether credentials are truly needed on the client and, if so, whether they are handled via a safer mechanism (e.g., server-side proxy) instead of storing them in layout meta.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-d44f4aab90f11377026bb11846aa4b4b971b3b88f644c3a1cb40752847412fe2R429-R451'><strong>Validation</strong></a><br>The `apiHeaders` and `apiPayload` fields are described and documented as JSON but are currently free-form strings without any validation. If invalid JSON is saved, downstream code that parses these fields may throw at execution time rather than surfacing errors in the configuration form. It would be safer to add JSON validation rules in the form so misconfigurations are caught early.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-68eae70bc67f0004ba108d71b4a77c4e9c3d5d883d69acbf4d003147b2f80cfcR185-R196'><strong>Possible Bug</strong></a><br>MQTT topic validation uses the raw `value` for the wildcard `#` position check without trimming whitespace first. This means inputs like `sensors/# ` (with trailing space) are rejected even though they would be saved as a valid trimmed topic, which can confuse users or block valid submissions.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-e771c54a5f4f6c95be03d98683162a3e4973c8def1ef71013de0f40778885196R288-R307'><strong>Button UX</strong></a><br>The button is disabled whenever configuration is missing, regardless of `editMode`. In edit mode this may prevent interacting with the button content (e.g., editing the title via `EditableTitle`) until configuration is complete, which could be confusing or block basic layout editing. Consider whether the disabled state should be applied only in view mode.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-aa2ede5fe8e8a857ca900ccc27578e88779290b31efbd7cc61677d3d4b780e89R133-R208'><strong>SSRF Risk</strong></a><br>The new `/v1/proxy/` endpoint acts as a generic outbound HTTP proxy for any authenticated user without host or IP restrictions, which can introduce a Server-Side Request Forgery (SSRF) risk (e.g., access to internal services or cloud metadata endpoints). This should be reviewed to ensure appropriate allowlists/denylists, IP/port checks, and other mitigations are in place before exposing it.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-aa2ede5fe8e8a857ca900ccc27578e88779290b31efbd7cc61677d3d4b780e89R133-R149'><strong>Access Control</strong></a><br>The `proxy_external_api` endpoint does not use the `@has_access_api` decorator while other API endpoints do, and instead manually checks `g.user`. This may bypass existing permission/role checks or API token flows that `has_access_api` enforces. Confirm that this endpoint is intentionally less restricted and that it aligns with Superset's standard API authorization model.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-259a7a48fa7bbdb6edd62a32c514db47e0625e2bb8504d012b4fcf31915be408R118-R153'><strong>Security Issue</strong></a><br>The script prints the generated Superset secret key to stdout and uses a very weak default admin password (`admin`). This can leak sensitive information in shared terminals or logs and encourages insecure defaults for production deployments.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-e61bfb812546b11eb8feefa1aa68d678039748cbd90ea867bc23863cdc11e7abR146-R175'><strong>Possible Bug</strong></a><br>Database initialization detection uses `SELECT 1` to decide whether to run `superset db upgrade` and `superset init`. A fresh PostgreSQL instance with the `superset` database created will still return success for `SELECT 1`, so initial migrations and setup may never run, leaving Superset uninitialized even though the script thinks the database is ready.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-259a7a48fa7bbdb6edd62a32c514db47e0625e2bb8504d012b4fcf31915be408R30-R35'><strong>Possible Bug</strong></a><br>The Docker Compose availability check allows systems that only have the legacy `docker-compose` binary to pass, but the rest of the script always uses `docker compose`. On such systems, later `docker compose` commands will fail at runtime even though the initial check succeeded. Consider normalizing to one command via a variable and using it consistently.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-e61bfb812546b11eb8feefa1aa68d678039748cbd90ea867bc23863cdc11e7abR38-R201'><strong>Possible Bug</strong></a><br>The script allows either `docker compose` or `docker-compose` to satisfy the dependency check, but all subsequent calls hard-code `docker compose`. On systems that only have `docker-compose` installed, the check will pass but every `docker compose ...` invocation will fail at runtime.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-6d70f30b3f4c341ec80a1ffb6e382a5174c0625d239a57b778a34cbdb5b3116bR24-R33'><strong>Possible Bug</strong></a><br>The `grep` check for `@google/model-viewer` does not distinguish between "string not found" and "package.json missing" (or other `grep` errors), so a missing `superset-frontend/package.json` will surface only as a non-blocking warning instead of a clear hard failure, which can hide misconfiguration in CI or new environments.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-a6852a68ced1f6179f15ea996abdfbfaa3c3d3839bec685312a91810cbfabab0R67-R85'><strong>Possible Bug</strong></a><br>The default metadata for `BUTTON_TYPE` sets `apiHeaders` and `apiPayload` to empty strings. If the consuming code expects these fields to be objects (e.g., parsed JSON or key/value maps) rather than plain strings, this mismatch could lead to runtime errors when reading, merging, or serializing these values. The reviewer should confirm that downstream usage of these fields correctly handles string defaults or adjust the defaults to match the expected type.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-8f78f3866f02305f1df51579c939174b2c53e8295ad13bd9d2ab763176a792c1R29-R47'><strong>Sensitive Credentials</strong></a><br>Non-default database passwords are hardcoded into the committed environment file (e.g., DATABASE_PASSWORD and POSTGRES_PASSWORD). These may be reused elsewhere and could represent real credentials, so they should be externalized or replaced with clearly dummy values and overridden locally.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-8f78f3866f02305f1df51579c939174b2c53e8295ad13bd9d2ab763176a792c1R27-R60'><strong>Connectivity Configuration</strong></a><br>The database and Redis hosts are switched to host.docker.internal with nonstandard ports, which will break the default docker-compose network assumptions unless corresponding services run on the host. This may prevent the stack from starting or cause confusing connection errors for other developers.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-a327f3e78ad99f98e52a27334944cad1970aeb8034539bd45bb07e6644af72beR21-R29'><strong>Possible Bug</strong></a><br>The current image-existence check using `docker images %IMAGE_TAG%` likely never fails even when the image is missing, since `docker images` usually returns a zero exit code with empty results. This may cause the script to proceed to `docker save` and only fail later, resulting in a confusing user experience and unreliable validation of the image tag.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-456e6ee553102f09ece397bff6e57bc2fbc8be138feea8be81044220f6a72972R343-R362'><strong>Documentation Bug</strong></a><br>The documented IoT endpoint domain is inconsistent: earlier it uses `https://app.snap4idcity.com`, while the note block references `https://app.snap4idtcity.com` (extra `t`). This mismatch can confuse users and should be corrected to the intended domain.<br> - [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-734e56652f886b4c194799a7b3640a1f06c50dd1200918d2b0a453d38647203bR345-R361'><strong>Possible Bug</strong></a><br>The example external API hostname (`https://app.snap4idcity.com`) does not match the hostname in the note block (`https://app.snap4idtcity.com`), which likely contains a typo and may lead users to configure the wrong domain in their CSP.<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]
