bito-code-review[bot] commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3700310937
##########
superset-frontend/packages/superset-ui-chart-controls/src/types.ts:
##########
@@ -507,7 +508,7 @@ export type ColorFormatters = {
objectFormatting?: ObjectFormattingEnum;
getColorFromValue: (
value: number | string | boolean | null,
- ) => string | undefined;
+ ) => RgbaColor | string | undefined;
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect type annotation</b></div>
<div id="fix">
The `getColorFromValue` return type now incorrectly includes `RgbaColor`.
Tracing the implementation in `getColorFormatters.ts:277-305`, all return paths
produce hex color strings (via `addAlpha()` at lines 285/300, `baseHexColor` at
lines 295/305, or direct `colorScheme` string at lines 279/282/290) or
`undefined`. No code path returns an `RgbaColor` object.
</div>
</div>
<small><i>Code Review Run #98a0f8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -16,70 +16,203 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { getCategoricalSchemeRegistry } from '@superset-ui/core';
+import { useMemo } from 'react';
+import { getCategoricalSchemeRegistry, rgbaToHex } from '@superset-ui/core';
+import { t } from '@apache-superset/core/translation';
import {
ColorPicker,
type RGBColor,
type ColorValue,
} from '@superset-ui/core/components';
import ControlHeader from '../ControlHeader';
+import { useTheme, type SupersetTheme } from '@apache-superset/core/theme';
+
+const SPECIAL_COLORS = {
+ Red: { r: 150, g: 0, b: 0, a: 0.2 },
+ Green: { r: 0, g: 150, b: 0, a: 0.2 },
+} as const;
+
+type SpecialColorKey = keyof typeof SPECIAL_COLORS;
+export type ColorPickerValue = RGBColor | SpecialColorKey | string;
export interface ColorPickerControlProps {
- onChange?: (color: RGBColor) => void;
- value?: RGBColor;
+ onChange?: (color: ColorPickerValue) => void;
+ value?: ColorPickerValue;
name?: string;
label?: string;
description?: string;
renderTrigger?: boolean;
hovered?: boolean;
warning?: string;
+ presets?: { label: string; colors: string[] }[];
+ ariaLabel?: string;
+ resolveThemeTokens?: boolean;
}
-function rgbToHex(rgb: RGBColor): string {
- const { r, g, b, a = 1 } = rgb;
- const toHex = (value: number) => {
- const hex = Math.round(value).toString(16);
- return hex.length === 1 ? `0${hex}` : hex;
- };
+const normalizeColorToHex = (color: string): string => {
+ if (!color) return '';
+
+ if (color.startsWith('#')) {
+ return color.toLowerCase();
+ }
- const hexColor = `#${toHex(r)}${toHex(g)}${toHex(b)}`;
+ const div = document.createElement('div');
+ div.style.color = color;
+ const normalized = div.style.color;
- if (a !== undefined && a !== 1) {
- return `${hexColor}${toHex(Math.round(a * 255))}`;
+ const match = normalized.match(
+ /rgba?\((\d+),\s*(\d+),\s*(\d+)(?:,\s*([\d.]+))?\)/,
+ );
+ if (match) {
+ return rgbaToHex({
+ r: parseInt(match[1], 10),
+ g: parseInt(match[2], 10),
+ b: parseInt(match[3], 10),
+ a: match[4] !== undefined ? parseFloat(match[4]) : 1,
+ }).toLowerCase();
}
- return hexColor;
+ return color.toLowerCase();
+};
+
+const getReverseThemeColorMap = (
+ themeColors: Record<string, string>,
+): Record<string, string> => {
+ const reverseMap: Record<string, string> = {};
+ if (!themeColors) return reverseMap;
+
+ Object.entries(themeColors).forEach(([name, value]) => {
+ if (typeof value === 'string') {
+ reverseMap[normalizeColorToHex(value)] = name;
+ }
+ });
+
+ return reverseMap;
+};
+
+function toDisplayHex(
+ value: ColorPickerValue | undefined,
+ themeColors: Record<string, string>,
+): string | undefined {
+ if (!value) return undefined;
+
+ if (typeof value === 'string') {
+ if (value in SPECIAL_COLORS) {
+ return rgbaToHex(SPECIAL_COLORS[value as SpecialColorKey]).toLowerCase();
+ }
+ if (themeColors && value in themeColors) {
+ return themeColors[value].toLowerCase();
+ }
+ return value.toLowerCase();
+ }
+
+ return rgbaToHex(value).toLowerCase();
}
+const extractThemeColors = (
+ theme: SupersetTheme | undefined | null,
+): Record<string, string> => {
+ if (!theme || typeof theme !== 'object') {
+ return {};
+ }
+
+ if (
+ 'colors' in theme &&
+ typeof theme.colors === 'object' &&
+ theme.colors !== null
+ ) {
+ return theme.colors as Record<string, string>;
+ }
+
+ return theme as unknown as Record<string, string>;
+};
+
export default function ColorPickerControl({
onChange,
value,
+ presets: customPresets,
+ ariaLabel,
+ resolveThemeTokens = false,
...headerProps
}: ColorPickerControlProps) {
const categoricalScheme = getCategoricalSchemeRegistry().get();
- const presetColors = categoricalScheme?.colors.slice(0, 9) || [];
+ const defaultPresets = categoricalScheme?.colors.slice(0, 9) || [];
+ const theme = useTheme();
+
+ const themeColors = useMemo<Record<string, string>>(
+ () => extractThemeColors(theme),
+ [theme],
+ );
+
+ const reverseMap = useMemo(
+ () => getReverseThemeColorMap(themeColors),
+ [themeColors],
+ );
+
+ const presets = useMemo(() => {
+ if (customPresets) {
+ return customPresets.map(item => ({
+ label: item.label,
+ colors: item.colors.map(color => {
+ if (color in SPECIAL_COLORS) {
+ return rgbaToHex(
+ SPECIAL_COLORS[color as SpecialColorKey],
+ ).toLowerCase();
+ }
+ if (themeColors && color in themeColors) {
+ return themeColors[color].toLowerCase();
+ }
+ return String(color).toLowerCase();
+ }),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Object injection in presets color mapping</b></div>
<div id="fix">
Similar to line 104, the check `color in themeColors` at line 163 is
vulnerable to prototype pollution when processing custom preset colors.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
SPECIAL_COLORS[color as SpecialColorKey],
).toLowerCase();
}
if (themeColors &&
Object.prototype.hasOwnProperty.call(themeColors, color as string)) {
return themeColors[color as string].toLowerCase();
}
return String(color).toLowerCase();
}),
````
</div>
</details>
</div>
<small><i>Code Review Run #98a0f8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -16,70 +16,203 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { getCategoricalSchemeRegistry } from '@superset-ui/core';
+import { useMemo } from 'react';
+import { getCategoricalSchemeRegistry, rgbaToHex } from '@superset-ui/core';
+import { t } from '@apache-superset/core/translation';
import {
ColorPicker,
type RGBColor,
type ColorValue,
} from '@superset-ui/core/components';
import ControlHeader from '../ControlHeader';
+import { useTheme, type SupersetTheme } from '@apache-superset/core/theme';
+
+const SPECIAL_COLORS = {
+ Red: { r: 150, g: 0, b: 0, a: 0.2 },
+ Green: { r: 0, g: 150, b: 0, a: 0.2 },
+} as const;
+
+type SpecialColorKey = keyof typeof SPECIAL_COLORS;
+export type ColorPickerValue = RGBColor | SpecialColorKey | string;
export interface ColorPickerControlProps {
- onChange?: (color: RGBColor) => void;
- value?: RGBColor;
+ onChange?: (color: ColorPickerValue) => void;
+ value?: ColorPickerValue;
name?: string;
label?: string;
description?: string;
renderTrigger?: boolean;
hovered?: boolean;
warning?: string;
+ presets?: { label: string; colors: string[] }[];
+ ariaLabel?: string;
+ resolveThemeTokens?: boolean;
}
-function rgbToHex(rgb: RGBColor): string {
- const { r, g, b, a = 1 } = rgb;
- const toHex = (value: number) => {
- const hex = Math.round(value).toString(16);
- return hex.length === 1 ? `0${hex}` : hex;
- };
+const normalizeColorToHex = (color: string): string => {
+ if (!color) return '';
+
+ if (color.startsWith('#')) {
+ return color.toLowerCase();
+ }
- const hexColor = `#${toHex(r)}${toHex(g)}${toHex(b)}`;
+ const div = document.createElement('div');
+ div.style.color = color;
+ const normalized = div.style.color;
- if (a !== undefined && a !== 1) {
- return `${hexColor}${toHex(Math.round(a * 255))}`;
+ const match = normalized.match(
+ /rgba?\((\d+),\s*(\d+),\s*(\d+)(?:,\s*([\d.]+))?\)/,
+ );
+ if (match) {
+ return rgbaToHex({
+ r: parseInt(match[1], 10),
+ g: parseInt(match[2], 10),
+ b: parseInt(match[3], 10),
+ a: match[4] !== undefined ? parseFloat(match[4]) : 1,
+ }).toLowerCase();
}
- return hexColor;
+ return color.toLowerCase();
+};
+
+const getReverseThemeColorMap = (
+ themeColors: Record<string, string>,
+): Record<string, string> => {
+ const reverseMap: Record<string, string> = {};
+ if (!themeColors) return reverseMap;
+
+ Object.entries(themeColors).forEach(([name, value]) => {
+ if (typeof value === 'string') {
+ reverseMap[normalizeColorToHex(value)] = name;
+ }
+ });
+
+ return reverseMap;
+};
+
+function toDisplayHex(
+ value: ColorPickerValue | undefined,
+ themeColors: Record<string, string>,
+): string | undefined {
+ if (!value) return undefined;
+
+ if (typeof value === 'string') {
+ if (value in SPECIAL_COLORS) {
+ return rgbaToHex(SPECIAL_COLORS[value as SpecialColorKey]).toLowerCase();
+ }
+ if (themeColors && value in themeColors) {
+ return themeColors[value].toLowerCase();
+ }
+ return value.toLowerCase();
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Object injection vulnerability in themeColors
lookup</b></div>
<div id="fix">
Dynamic property access `value in themeColors` at line 104 could allow
prototype pollution if `value` contains malicious keys like `__proto__` or
`constructor`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
if (value in SPECIAL_COLORS) {
return rgbaToHex(SPECIAL_COLORS[value as
SpecialColorKey]).toLowerCase();
}
if (themeColors && Object.prototype.hasOwnProperty.call(themeColors,
value)) {
return themeColors[value as string].toLowerCase();
}
return value.toLowerCase();
}
````
</div>
</details>
</div>
<small><i>Code Review Run #98a0f8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/explore/components/controls/ColorPickerControl.tsx:
##########
@@ -16,70 +16,203 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { getCategoricalSchemeRegistry } from '@superset-ui/core';
+import { useMemo } from 'react';
+import { getCategoricalSchemeRegistry, rgbaToHex } from '@superset-ui/core';
+import { t } from '@apache-superset/core/translation';
import {
ColorPicker,
type RGBColor,
type ColorValue,
} from '@superset-ui/core/components';
import ControlHeader from '../ControlHeader';
+import { useTheme, type SupersetTheme } from '@apache-superset/core/theme';
+
+const SPECIAL_COLORS = {
+ Red: { r: 150, g: 0, b: 0, a: 0.2 },
+ Green: { r: 0, g: 150, b: 0, a: 0.2 },
+} as const;
+
+type SpecialColorKey = keyof typeof SPECIAL_COLORS;
+export type ColorPickerValue = RGBColor | SpecialColorKey | string;
export interface ColorPickerControlProps {
- onChange?: (color: RGBColor) => void;
- value?: RGBColor;
+ onChange?: (color: ColorPickerValue) => void;
+ value?: ColorPickerValue;
name?: string;
label?: string;
description?: string;
renderTrigger?: boolean;
hovered?: boolean;
warning?: string;
+ presets?: { label: string; colors: string[] }[];
+ ariaLabel?: string;
+ resolveThemeTokens?: boolean;
}
-function rgbToHex(rgb: RGBColor): string {
- const { r, g, b, a = 1 } = rgb;
- const toHex = (value: number) => {
- const hex = Math.round(value).toString(16);
- return hex.length === 1 ? `0${hex}` : hex;
- };
+const normalizeColorToHex = (color: string): string => {
+ if (!color) return '';
+
+ if (color.startsWith('#')) {
+ return color.toLowerCase();
+ }
- const hexColor = `#${toHex(r)}${toHex(g)}${toHex(b)}`;
+ const div = document.createElement('div');
+ div.style.color = color;
+ const normalized = div.style.color;
- if (a !== undefined && a !== 1) {
- return `${hexColor}${toHex(Math.round(a * 255))}`;
+ const match = normalized.match(
+ /rgba?\((\d+),\s*(\d+),\s*(\d+)(?:,\s*([\d.]+))?\)/,
+ );
+ if (match) {
+ return rgbaToHex({
+ r: parseInt(match[1], 10),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unsafe regex pattern in color parsing</b></div>
<div id="fix">
The regex pattern `/rgba?\(\d+,\s*\d+,\s*\d+(?:,\s*[\d.]+)?\)/` may be
vulnerable to ReDoS attacks with crafted input strings containing repeated
characters.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
const div = document.createElement('div');
div.style.color = color;
const normalized = div.style.color || '';
const match = /^rgba?\((\d+),\s+(\d+),\s+(\d+)(?:,\s*([\d.]+))?\)$/.exec(
normalized,
);
if (match) {
return rgbaToHex({
r: parseInt(match[1], 10),
````
</div>
</details>
</div>
<small><i>Code Review Run #98a0f8</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
