codeant-ai-for-open-source[bot] commented on PR #36201: URL: https://github.com/apache/superset/pull/36201#issuecomment-3629437324
## 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/36201/files#diff-53246e6303a6e892e31fc3931538833b9bd4af6f9ce2288271df6f0c8227ce60R272-R309'><strong>Risky sandboxed execution</strong></a><br>Multiple calls to `sandboxedEval` are used (data mutator, label generator, icon generator). Ensure the sandbox implementation actually isolates execution and consider limiting execution time / resource usage. Also, any exceptions thrown by the evaluated code are currently not caught in all places — these should be handled to avoid destabilising the layer rendering.<br> - [ ] <a href='https://github.com/apache/superset/pull/36201/files#diff-53246e6303a6e892e31fc3931538833b9bd4af6f9ce2288271df6f0c8227ce60R290-R311'><strong>Unvalidated user JS output</strong></a><br>The code executes user-provided JavaScript (via sandboxedEval) to generate label and icon configuration objects and then spreads the results into the GeoJsonLayer props. The generator's return value is only guarded by `typeof generator === 'function'` and then passed directly to compute*Options functions (which only filter keys). This can lead to runtime errors (wrong types for deck.gl props), unexpected behaviour, or a crash if the generator throws. Consider validating the generator return shape and surrounding execution with try/catch and fallbacks.<br> - [ ] <a href='https://github.com/apache/superset/pull/36201/files#diff-53246e6303a6e892e31fc3931538833b9bd4af6f9ce2288271df6f0c8227ce60R145-R230'><strong>Weak runtime type-safety for options</strong></a><br>computeGeoJsonTextOptionsFromJsOutput and computeGeoJsonIconOptionsFromJsOutput only filter keys by name but do not validate that the values have the expected types (e.g., functions, arrays, numbers). This can pass invalid values into deck.gl and cause runtime errors. Consider asserting types for common props (e.g., `getText` should be a function or string-producing accessor) before returning.<br> - [ ] <a href='https://github.com/apache/superset/pull/36201/files#diff-b465adef04aeff7ee59e7387b8e418c67f28c6ee5a040b40f62deafcbf2d6f79R190-R315'><strong>Arbitrary JS execution</strong></a><br>The new controls expose JavaScript config generators (`label_javascript_config_generator` and `icon_javascript_config_generator`) that accept user-provided JS and are later evaluated to configure deck.gl. Even though this is gated by FeatureFlag.EnableJavascriptControls, evaluate how and where that JS is executed, whether it is sandboxed, and what protections exist against arbitrary script execution, data exfiltration, or XSS-like behaviors.<br> - [ ] <a href='https://github.com/apache/superset/pull/36201/files#diff-b465adef04aeff7ee59e7387b8e418c67f28c6ee5a040b40f62deafcbf2d6f79R241-R256'><strong>Icon URL validation / CSP risk</strong></a><br>The `icon_url` text control allows arbitrary URLs and only references CSP in the description. There is no validator to block dangerous schemes (e.g., `javascript:`, `data:`) or to ensure HTTPS. This can lead to failed loads under strict CSP or unexpected behaviors — validate input and/or sanitize before use.<br> - [ ] <a href='https://github.com/apache/superset/pull/36201/files#diff-b465adef04aeff7ee59e7387b8e418c67f28c6ee5a040b40f62deafcbf2d6f79R49-R56'><strong>Defensive defaults</strong></a><br>The default label config generator uses `f.properties.name` directly. If a feature object is missing `properties` or `name`, the accessor may throw when executed in some environments. Use defensive access (optional chaining) in the default example to avoid runtime errors for edge-case GeoJSON.<br> - [ ] <a href='https://github.com/apache/superset/pull/36201/files#diff-7f154d56dbf5bb75f6fa4d1e1c2b3bf74487b7449f22545409f600f69e07c960R99-R105'><strong>Public API change</strong></a><br>The function `jsFunctionControl` was changed from an internal function to a named export. Verify this was intentional: exporting it makes it part of the module's public surface and may require adding it to consuming modules' imports or the package's barrel exports. Confirm there are no naming collisions with other exports and that consumers handle the new export correctly.<br> - [ ] <a href='https://github.com/apache/superset/pull/36201/files#diff-04bd024a9c7407e91394978b7a5b781410ae1c93069e2da1ce1805aaf48b8eddR62-R71'><strong>Brittle assertions</strong></a><br>Several tests use full-object equality (toEqual) including function properties and exact nested objects. These assertions are brittle: small, unrelated implementation changes (additional keys, reordering, or small numeric differences) will break the tests even if behavior is correct. Prefer matching subsets (toMatchObject / objectContaining) or asserting behavior of returned functions rather than strict object equality.<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]
