codeant-ai-for-open-source[bot] commented on code in PR #39171: URL: https://github.com/apache/superset/pull/39171#discussion_r3561099393
########## superset-frontend/src/core/storage/ephemeralState.test.ts: ########## @@ -0,0 +1,144 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { SupersetClient } from '@superset-ui/core'; +import { createEphemeralState } from './ephemeralState'; + +jest.mock('@superset-ui/core', () => ({ + SupersetClient: { + get: jest.fn(), + put: jest.fn(), + delete: jest.fn(), + }, +})); + +const mockGet = SupersetClient.get as jest.Mock; +const mockPut = SupersetClient.put as jest.Mock; +const mockDelete = SupersetClient.delete as jest.Mock; + +beforeEach(() => { + jest.clearAllMocks(); + mockGet.mockResolvedValue({ json: { result: null } }); + mockPut.mockResolvedValue({}); + mockDelete.mockResolvedValue({}); +}); + +test('get calls correct URL with publisher/name pattern', async () => { + const store = createEphemeralState('myorg.myext'); + await store.get('job_progress'); + expect(mockGet).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/ephemeral/job_progress', + }); +}); + +test('get returns result from response', async () => { + mockGet.mockResolvedValue({ json: { result: { pct: 42 } } }); + const store = createEphemeralState('myorg.myext'); + const result = await store.get('job_progress'); + expect(result).toEqual({ pct: 42 }); +}); + +test('get returns null when result is absent', async () => { + mockGet.mockResolvedValue({ json: {} }); + const store = createEphemeralState('myorg.myext'); + expect(await store.get('key')).toBeNull(); +}); + +test('set calls correct URL and includes value and ttl in body', async () => { + const store = createEphemeralState('myorg.myext'); + await store.set('job_progress', { pct: 42 }, { ttl: 300 }); + expect(mockPut).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/ephemeral/job_progress', + body: JSON.stringify({ value: { pct: 42 }, ttl: 300, codec: 'json' }), + headers: { 'Content-Type': 'application/json' }, + }); +}); + +test('set passes codec from options', async () => { + const store = createEphemeralState('myorg.myext'); + await store.set('job_progress', 'sk-...', { ttl: 300, codec: 'pickle' }); + expect(mockPut).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/ephemeral/job_progress', + body: JSON.stringify({ value: 'sk-...', ttl: 300, codec: 'pickle' }), + headers: { 'Content-Type': 'application/json' }, + }); Review Comment: **Suggestion:** This test encodes `pickle` as a valid REST codec, but the server-side API only allows safe codecs (`json`/`base64`) and rejects `pickle` with HTTP 400. Keeping this expectation in tests documents an invalid client contract and can mask real integration failures; use an allowed codec in this test. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Ephemeral REST writes using pickle codec are rejected 400. - ⚠️ Tests advertise unsupported codec, misleading extension implementers. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The frontend test `set passes codec from options` in `superset-frontend/src/core/storage/ephemeralState.test.ts:73-80` calls `store.set('job_progress', 'sk-...', { ttl: 300, codec: 'pickle' })` and asserts that the client sends `body: JSON.stringify({ value: 'sk-...', ttl: 300, codec: 'pickle' })` to `/api/v1/extensions/myorg/myext/storage/ephemeral/job_progress`. 2. In production, `createEphemeralState` in `superset-frontend/src/core/storage/ephemeralState.ts:72-92` forwards this `codec: 'pickle'` unchanged: `resolveSetPayload` at `binaryCodec.ts:58-63` returns `{ value: 'sk-...', codec: 'pickle' }`, and `SupersetClient.put` issues the same JSON body the test expects. 3. On the backend, the `set_ephemeral` REST handler in `superset/extensions/storage/api.py:29-35, 99-107` reads `codec = body.get("codec", DEFAULT_CODEC)` and checks `if codec not in SAFE_CODECS:` returning `response_400(f"Codec '{codec}' is not allowed over the REST API.")` (api.py:103-107). `SAFE_CODECS` is defined in `superset/extensions/storage/codecs.py:42-46` as `{"json", "base64"}`, explicitly excluding `"pickle"` for safety. 4. As a result, any real client following this test's contract and sending `codec: 'pickle'` to the ephemeral storage endpoint will receive HTTP 400 rather than succeed, while the test—using a mocked `SupersetClient`—continues to pass, documenting an invalid codec choice and risking future extension code adopting `pickle` over REST and failing at runtime. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fcbc8209c2e3403a90841ea7e4865c1e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fcbc8209c2e3403a90841ea7e4865c1e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/core/storage/ephemeralState.test.ts **Line:** 75:80 **Comment:** *Api Mismatch: This test encodes `pickle` as a valid REST codec, but the server-side API only allows safe codecs (`json`/`base64`) and rejects `pickle` with HTTP 400. Keeping this expectation in tests documents an invalid client contract and can mask real integration failures; use an allowed codec in this test. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=04574434b6a9b660a731da8a94d055b1802ae388c5f6d14abd93a53f3ec6059e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=04574434b6a9b660a731da8a94d055b1802ae388c5f6d14abd93a53f3ec6059e&reaction=dislike'>👎</a> ########## superset-frontend/src/core/storage/binaryCodec.ts: ########## @@ -0,0 +1,70 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** Convert bytes to a base64 string, for transport over the JSON REST API. */ +export function bytesToBase64(bytes: Uint8Array): string { + let binary = ''; + bytes.forEach(byte => { + binary += String.fromCharCode(byte); + }); + return btoa(binary); +} + +function isBinaryValue(value: unknown): value is Uint8Array | ArrayBuffer { + return value instanceof Uint8Array || value instanceof ArrayBuffer; +} + +/** + * Infer which codec to use for `value` when the caller didn't specify one. + * + * "json" names the serialization format applied to encode the value for + * storage/transport -- it is not a claim about the value's shape. Per + * RFC 8259, a JSON text may be any JSON value (object, array, string, + * number, boolean, or null), so scalars like `1` or `"foo"` are encoded + * with the "json" codec too, alongside objects and arrays. + * + * Binary values (Uint8Array/ArrayBuffer) are the one case JSON cannot + * represent, so they're base64-encoded instead. + */ +function inferDefaultCodec(value: unknown): 'json' | 'base64' { + return isBinaryValue(value) ? 'base64' : 'json'; +} + +/** + * Resolve the codec and request-body value for a `set()` call. + * + * If the caller explicitly set `options.codec`, it is always respected + * as-is and `value` is passed through untouched -- the caller is + * responsible for encoding it correctly (e.g. base64-encoding it + * themselves before calling `set()`). Auto base64-encoding only happens + * when no codec was specified and `value` is binary. + */ +export function resolveSetPayload<T>( + value: T, + explicitCodec: string | undefined, +): { value: unknown; codec: string } { + if (explicitCodec !== undefined) { + return { value, codec: explicitCodec }; Review Comment: **Suggestion:** The explicit-codec fast path returns binary values unchanged, so callers using `codec: 'base64'` with a `Uint8Array`/`ArrayBuffer` will send JSON-serialized typed-array objects instead of a base64 string. The backend base64 codec expects a string and this will fail decoding at runtime. Keep honoring explicit codec, but still perform base64 encoding when the explicit codec is `base64` and the value is binary. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Ephemeral REST storage fails for base64-encoded binary values. - ⚠️ Persistent REST storage similarly mis-encodes binary payloads. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. An extension frontend obtains its context via `getContext()` wired through `superset-frontend/src/core/extensions/index.ts:37-40`, then calls `ctx.storage.ephemeral.set('icon', bytes, { ttl: 300, codec: 'base64' })`, where `bytes` is a `Uint8Array` (storage wiring in `superset-frontend/src/extensions/ExtensionContext.ts:44-52` uses `createEphemeralState(id)`). 2. `createEphemeralState` in `superset-frontend/src/core/storage/ephemeralState.ts:72-92` calls `resolveSetPayload(value, options.codec)`; with `options.codec === 'base64'`, the explicit-codec branch in `binaryCodec.ts:58-63` returns `{ value: bytes, codec: 'base64' }` without encoding, so `SupersetClient.put` sends a JSON body where `value` is a typed array that `JSON.stringify` serializes as an array of integers. 3. The backend endpoint `/api/v1/extensions/<publisher>/<name>/storage/ephemeral/<key>` implemented in `superset/extensions/storage/api.py:29-35, 99-109` reads `codec = body.get("codec", DEFAULT_CODEC)` and `value = body["value"]`, then calls `ExtensionEphemeralDAO.set(extension_id, key, value, ttl, codec=codec, shared=shared)` (api.py:116-118) with `codec == "base64"` and `value` as a JSON array. 4. `ExtensionEphemeralDAO.set` in `superset/extensions/storage/ephemeral_dao.py:129-148` computes `encoded = get_codec(codec).encode(value)`; for `codec == "base64"`, `get_codec` returns `Base64KeyValueCodec` whose `encode(self, value: str) -> bytes` in `superset/key_value/types.py:92-103` expects a base64 string. Passing a list of integers causes `base64.b64decode` to raise `TypeError`, wrapped as `KeyValueCodecEncodeException`, which bubbles up and fails the REST request, so extensions using `codec: "base64"` with binary values cannot successfully store data. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4f5ad00bc0cb499ba656cc3ae993e82a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4f5ad00bc0cb499ba656cc3ae993e82a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/core/storage/binaryCodec.ts **Line:** 62:63 **Comment:** *Api Mismatch: The explicit-codec fast path returns binary values unchanged, so callers using `codec: 'base64'` with a `Uint8Array`/`ArrayBuffer` will send JSON-serialized typed-array objects instead of a base64 string. The backend base64 codec expects a string and this will fail decoding at runtime. Keep honoring explicit codec, but still perform base64 encoding when the explicit codec is `base64` and the value is binary. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=95e2a2cc8374634398894316fd7db12e080b0871754f86ba14ca61cc9d6d3bdd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=95e2a2cc8374634398894316fd7db12e080b0871754f86ba14ca61cc9d6d3bdd&reaction=dislike'>👎</a> ########## superset-frontend/src/core/storage/persistentState.test.ts: ########## @@ -0,0 +1,249 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { SupersetClient } from '@superset-ui/core'; +import { createPersistentState } from './persistentState'; + +jest.mock('@superset-ui/core', () => ({ + SupersetClient: { + get: jest.fn(), + put: jest.fn(), + delete: jest.fn(), + }, +})); + +const mockGet = SupersetClient.get as jest.Mock; +const mockPut = SupersetClient.put as jest.Mock; +const mockDelete = SupersetClient.delete as jest.Mock; + +beforeEach(() => { + jest.clearAllMocks(); + mockGet.mockResolvedValue({ json: { result: null } }); + mockPut.mockResolvedValue({}); + mockDelete.mockResolvedValue({}); +}); + +test('get calls correct URL with publisher/name pattern', async () => { + const store = createPersistentState('myorg.myext'); + await store.get('prefs'); + expect(mockGet).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/persistent/prefs', + }); +}); + +test('get returns result from response', async () => { + mockGet.mockResolvedValue({ json: { result: { theme: 'dark' } } }); + const store = createPersistentState('myorg.myext'); + expect(await store.get('prefs')).toEqual({ theme: 'dark' }); +}); + +test('get returns null when result is absent', async () => { + mockGet.mockResolvedValue({ json: {} }); + const store = createPersistentState('myorg.myext'); + expect(await store.get('key')).toBeNull(); +}); + +test('set calls correct URL with value in body', async () => { + const store = createPersistentState('myorg.myext'); + await store.set('prefs', { theme: 'dark' }); + expect(mockPut).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/persistent/prefs', + body: JSON.stringify({ + value: { theme: 'dark' }, + encrypt: false, + codec: 'json', + }), + headers: { 'Content-Type': 'application/json' }, + }); +}); + +test('set passes codec from options', async () => { + const store = createPersistentState('myorg.myext'); + await store.set('prefs', 'sk-...', { codec: 'pickle' }); + expect(mockPut).toHaveBeenCalledWith({ + endpoint: '/api/v1/extensions/myorg/myext/storage/persistent/prefs', + body: JSON.stringify({ + value: 'sk-...', + encrypt: false, + codec: 'pickle', + }), + headers: { 'Content-Type': 'application/json' }, + }); Review Comment: **Suggestion:** This test similarly treats `pickle` as an acceptable frontend REST codec even though the backend rejects it for API safety, so it validates behavior that cannot succeed in real requests. Update the test to use an allowed codec and payload shape that matches the server contract. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Persistent REST writes using pickle codec fail with 400. - ⚠️ Tests validate impossible behavior against real storage API. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The test `set passes codec from options` in `superset-frontend/src/core/storage/persistentState.test.ts:76-87` calls `store.set('prefs', 'sk-...', { codec: 'pickle' })` and asserts that the client sends `body: JSON.stringify({ value: 'sk-...', encrypt: false, codec: 'pickle' })` to `/api/v1/extensions/myorg/myext/storage/persistent/prefs`. 2. In production, `createPersistentState` in `superset-frontend/src/core/storage/persistentState.ts:116-136` uses `resolveSetPayload(value, options?.codec)`; with `codec: 'pickle'`, the explicit branch in `binaryCodec.ts:58-63` returns `{ value: 'sk-...', codec: 'pickle' }`, and `SupersetClient.put` issues exactly the JSON body asserted in the test. 3. The backend persistent storage REST handler (co-located with ephemeral in `superset/extensions/storage/api.py:36-42, 220-226`) similarly computes `codec = body.get("codec", DEFAULT_CODEC)` and enforces `if codec not in SAFE_CODECS:` then returns `response_400(f"Codec '{codec}' is not allowed over the REST API.")`, with `SAFE_CODECS` from `superset/extensions/storage/codecs.py:42-46` excluding `"pickle"`. 4. Therefore any extension code that follows this test's pattern and uses `codec: 'pickle'` with the persistent REST API will be rejected with HTTP 400, while the test—mocking `SupersetClient`—continues to pass and encode a client contract (`pickle` over REST) that is explicitly forbidden by the backend, risking integration failures and confusion about which codecs are supported. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b77b7f7fa205406f933c3c97a33e6a53&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=b77b7f7fa205406f933c3c97a33e6a53&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/core/storage/persistentState.test.ts **Line:** 78:87 **Comment:** *Api Mismatch: This test similarly treats `pickle` as an acceptable frontend REST codec even though the backend rejects it for API safety, so it validates behavior that cannot succeed in real requests. Update the test to use an allowed codec and payload shape that matches the server contract. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=f1215aa71a32715f908d657579ed8bd9e5cf6023565fce6b45cdbfff2fa528e3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=f1215aa71a32715f908d657579ed8bd9e5cf6023565fce6b45cdbfff2fa528e3&reaction=dislike'>👎</a> ########## superset-frontend/src/core/storage/persistentState.ts: ########## @@ -0,0 +1,147 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import type { + JsonValue, + PersistentListOptions, + PersistentListResult, + PersistentSetOptions, + PersistentStorageAccessor, + PersistentStorageTier, +} from '@apache-superset/core/storage'; +import { SupersetClient } from '@superset-ui/core'; +import { resolveSetPayload } from './binaryCodec'; + +/** + * Create persistent state (database-backed) bound to an extension ID. + */ +export function createPersistentState( + extensionId: string, +): PersistentStorageTier { + const MAX_KEY_LENGTH = 255; + const [publisher, name] = extensionId.split('.'); + + const buildUrl = (key: string, shared?: boolean): string => { + if (key.length > MAX_KEY_LENGTH) { + throw new Error( + `Persistent storage key must be ${MAX_KEY_LENGTH} characters or less.`, + ); + } + const encodedPublisher = encodeURIComponent(publisher); + const encodedName = encodeURIComponent(name); + const encodedKey = encodeURIComponent(key); + const url = `/api/v1/extensions/${encodedPublisher}/${encodedName}/storage/persistent/${encodedKey}`; + return shared ? `${url}?shared=true` : url; + }; + + const buildListUrl = ( + options: PersistentListOptions, + isShared?: boolean, + ): string => { + const encodedPublisher = encodeURIComponent(publisher); + const encodedName = encodeURIComponent(name); + const url = `/api/v1/extensions/${encodedPublisher}/${encodedName}/storage/persistent`; + const params = new URLSearchParams(); + if (isShared) params.set('shared', 'true'); + if (options.resourceType) params.set('resource_type', options.resourceType); + if (options.resourceUuid) params.set('resource_uuid', options.resourceUuid); + params.set('page', String(options.page)); + params.set('page_size', String(options.pageSize)); + const query = params.toString(); + return query ? `${url}?${query}` : url; + }; + + const list = async <T = JsonValue>( + options: PersistentListOptions, + isShared: boolean, + ): Promise<PersistentListResult<T>> => { + const response = await SupersetClient.get({ + endpoint: buildListUrl(options, isShared), + }); + return { + entries: response.json?.result ?? [], + count: response.json?.count ?? 0, + }; + }; + + const shared: PersistentStorageAccessor = { + async get<T = JsonValue>(key: string): Promise<T | null> { + const response = await SupersetClient.get({ + endpoint: buildUrl(key, true), + }); + return (response.json?.result ?? null) as T | null; + }, + async set<T = JsonValue>( + key: string, + value: T, + options?: PersistentSetOptions, + ): Promise<void> { + const payload = resolveSetPayload(value, options?.codec); + await SupersetClient.put({ + endpoint: buildUrl(key, true), + body: JSON.stringify({ + value: payload.value, + encrypt: options?.encrypt ?? false, + codec: payload.codec, + }), + headers: { 'Content-Type': 'application/json' }, + }); + }, + async list<T = JsonValue>( + options: PersistentListOptions, + ): Promise<PersistentListResult<T>> { + return list<T>(options, true); + }, + async remove(key: string): Promise<void> { + await SupersetClient.delete({ endpoint: buildUrl(key, true) }); + }, + }; + + return { + async get<T = JsonValue>(key: string): Promise<T | null> { + const response = await SupersetClient.get({ endpoint: buildUrl(key) }); + return (response.json?.result ?? null) as T | null; + }, + async set<T = JsonValue>( + key: string, + value: T, + options?: PersistentSetOptions, + ): Promise<void> { + const payload = resolveSetPayload(value, options?.codec); + await SupersetClient.put({ + endpoint: buildUrl(key), + body: JSON.stringify({ + value: payload.value, + encrypt: options?.encrypt ?? false, + codec: payload.codec, + }), + headers: { 'Content-Type': 'application/json' }, + }); Review Comment: **Suggestion:** The REST backend only accepts safe codecs (`json`/`base64`) for extension API writes, but this code forwards any caller-provided codec directly. Calls like `codec: 'pickle'` will deterministically fail with 400 responses at runtime, so validate/whitelist allowed codecs in the frontend API contract before sending requests. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Persistent extension writes with unsafe codecs always 400. - ⚠️ Extensions see failed saves for explicit pickle codec. - ⚠️ Tier 3 storage contract differs between frontend/backend. - ⚠️ Extension authors must debug opaque REST 400 errors. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. An extension is loaded by the host, which constructs a per-extension context via `createExtensionContext()` at `superset-frontend/src/extensions/ExtensionContext.ts:61-64`; the loader wires this into `extensions.getContext` in `superset-frontend/src/extensions/ExtensionsLoader.ts:14-20`, so extension code can call `getContext()` to obtain `ctx`. 2. The extension code calls the Tier 3 API `ctx.storage.persistent.set('prefs', 'sk-...', { codec: 'pickle' })`; this is valid per the TypeScript interface `PersistentSetOptions` in `superset-frontend/packages/superset-core/src/storage/index.ts:75-79`, which documents `codec?: string` without constraining values. 3. That call reaches the persistent implementation `createPersistentState()` in `superset-frontend/src/core/storage/persistentState.ts:121-135`, which computes `const payload = resolveSetPayload(value, options?.codec);` and then issues `SupersetClient.put` with a JSON body containing `codec: payload.codec`. The unit test `superset-frontend/src/core/storage/persistentState.test.ts:17-27` verifies that when `codec: 'pickle'` is passed, the client sends `codec: 'pickle'` unchanged to `/api/v1/extensions/myorg/myext/storage/persistent/prefs`. 4. On the backend, the REST API handler `ExtensionStorageRestApi.set_persistent()` enforces the codec whitelist `SAFE_CODECS = {'json', 'base64'}` defined in `superset/extensions/storage/codecs.py:42-47`; the test `superset/tests/unit_tests/extensions/storage/test_api.py:5-18` shows that a PUT to `/api/v1/extensions/acme/dashboard/storage/persistent/prefs` with `json={"value": "sk-...", "codec": "pickle"}` returns HTTP 400, with the DAO `mock_dao.set` not called. As a result, any extension that sets `codec: 'pickle'` (or other unsupported identifiers) via the persistent storage API deterministically gets a 400 and its data is never persisted. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=07ced2c3e5d845c8867aeb8f97f6b32c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=07ced2c3e5d845c8867aeb8f97f6b32c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/core/storage/persistentState.ts **Line:** 126:135 **Comment:** *Api Mismatch: The REST backend only accepts safe codecs (`json`/`base64`) for extension API writes, but this code forwards any caller-provided codec directly. Calls like `codec: 'pickle'` will deterministically fail with 400 responses at runtime, so validate/whitelist allowed codecs in the frontend API contract before sending requests. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=17daafa0f8911cb082544b4d18d1ada2d47db2d0d4367d094b6aac4ffefdc986&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=17daafa0f8911cb082544b4d18d1ada2d47db2d0d4367d094b6aac4ffefdc986&reaction=dislike'>👎</a> ########## superset-frontend/src/extensions/ExtensionContext.ts: ########## @@ -0,0 +1,65 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import type { + common, + extensions as extensionsApi, +} from '@apache-superset/core'; +import { + createBrowserStorage, + createEphemeralState, + createPersistentState, +} from 'src/core/storage'; + +type Extension = common.Extension; +type ExtensionContextType = extensionsApi.ExtensionContext; + +/** + * Extension context with lazy-initialized services bound to the extension ID. + */ +class ExtensionContext implements ExtensionContextType { + readonly extension: Extension; + + private _storage?: ExtensionContextType['storage']; + + constructor(extension: Extension) { + this.extension = extension; + } + + get storage(): ExtensionContextType['storage'] { + if (!this._storage) { + const { id } = this.extension; + this._storage = { + local: createBrowserStorage(localStorage, id), + session: createBrowserStorage(sessionStorage, id), + ephemeral: createEphemeralState(id), + persistent: createPersistentState(id), + }; Review Comment: **Suggestion:** All storage tiers are initialized together, and this eagerly touches `localStorage`/`sessionStorage`; in environments where Web Storage is unavailable (or blocked), accessing `ctx.storage` throws before `ephemeral`/`persistent` can be used. Initialize browser tiers defensively or lazily per-tier so server-backed storage remains usable. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ ctx.storage access fails when Web Storage unavailable. - ⚠️ Ephemeral server cache inaccessible despite healthy backend. - ⚠️ Persistent Tier 3 storage unusable under blocked storage. - ⚠️ Extensions depending on ctx.storage cannot degrade gracefully. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. When an extension is loaded, the host constructs its context via `createExtensionContext(extension)` at `superset-frontend/src/extensions/ExtensionContext.ts:61-64`; `ExtensionsLoader` then injects this into the module-federated scope so `extensions.getContext()` returns that context (`superset-frontend/src/extensions/ExtensionsLoader.ts:14-20`). 2. Inside extension code, the developer calls `const ctx = getContext();` and then accesses `ctx.storage.ephemeral` or `ctx.storage.persistent` to use server-backed storage; this triggers the `storage` getter in `ExtensionContext` at `superset-frontend/src/extensions/ExtensionContext.ts:44-55`. 3. On the first access, `this._storage` is undefined, so the getter eagerly initializes all tiers at once: `this._storage = { local: createBrowserStorage(localStorage, id), session: createBrowserStorage(sessionStorage, id), ephemeral: createEphemeralState(id), persistent: createPersistentState(id) };` (lines 47-52). This directly reads the global `localStorage` and `sessionStorage` objects before constructing the ephemeral/persistent tiers. 4. In environments where Web Storage APIs are unavailable or blocked (for example, a browser or embedded frame where `window.localStorage` and/or `window.sessionStorage` access throws or is undefined), the call site `createBrowserStorage(localStorage, id)` fails before `_storage` is populated. Because the getter never completes, `ctx.storage.ephemeral` and `ctx.storage.persistent` cannot be reached, even though their implementations (`superset-frontend/src/core/storage/ephemeralState.ts:32-98` and `superset-frontend/src/core/storage/persistentState.ts:34-147`) and corresponding backend tiers (`superset-core/src/superset_core/extensions/storage/ephemeral.py` and `persistent.py`) would otherwise be usable. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d31d813346f447e18fbcf64a4a9bb752&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d31d813346f447e18fbcf64a4a9bb752&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/extensions/ExtensionContext.ts **Line:** 47:52 **Comment:** *Logic Error: All storage tiers are initialized together, and this eagerly touches `localStorage`/`sessionStorage`; in environments where Web Storage is unavailable (or blocked), accessing `ctx.storage` throws before `ephemeral`/`persistent` can be used. Initialize browser tiers defensively or lazily per-tier so server-backed storage remains usable. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=442765637cb3527cd52a3596165b47ca5d70384e03814f414d9141bda53f374b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39171&comment_hash=442765637cb3527cd52a3596165b47ca5d70384e03814f414d9141bda53f374b&reaction=dislike'>👎</a> -- 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]
