codeant-ai-for-open-source[bot] commented on code in PR #37280:
URL: https://github.com/apache/superset/pull/37280#discussion_r2707286638
##########
superset-embedded-sdk/src/index.ts:
##########
@@ -249,13 +249,14 @@ export async function embedDashboard({
ourPort.emit('guestToken', { guestToken });
log('sent guest token');
+ let refreshTokenTimerId: NodeJS.Timeout | null = null;
async function refreshGuestToken() {
const newGuestToken = await fetchGuestToken();
ourPort.emit('guestToken', { guestToken: newGuestToken });
- setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(newGuestToken));
+ refreshTokenTimerId = setTimeout(refreshGuestToken,
getGuestTokenRefreshTiming(newGuestToken));
}
- setTimeout(refreshGuestToken, getGuestTokenRefreshTiming(guestToken));
+ refreshTokenTimerId = setTimeout(refreshGuestToken,
getGuestTokenRefreshTiming(guestToken));
Review Comment:
**Suggestion:** Using `NodeJS.Timeout` and `setTimeout` without the DOM
`window` context can cause a type mismatch in browser builds (where
`setTimeout` returns a number). Also scheduling with an unbounded value
returned from `getGuestTokenRefreshTiming` can result in a negative delay being
passed to `setTimeout`, which is treated as 0 and can create a very
tight/continuous refresh loop. Change the timer id type to `number | null`,
call `window.setTimeout` to get a numeric id in DOM environments, and clamp the
delay to a non-negative value. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Embedded dashboard infinite refresh network requests.
- ❌ Guest token emission floods iframe comms.
- ⚠️ Build type mismatch in browser TypeScript.
```
</details>
```suggestion
let refreshTokenTimerId: number | null = null;
async function refreshGuestToken() {
const newGuestToken = await fetchGuestToken();
ourPort.emit('guestToken', { guestToken: newGuestToken });
const delay = Math.max(0, getGuestTokenRefreshTiming(newGuestToken));
refreshTokenTimerId = window.setTimeout(refreshGuestToken, delay);
}
{
const initialDelay = Math.max(0, getGuestTokenRefreshTiming(guestToken));
refreshTokenTimerId = window.setTimeout(refreshGuestToken, initialDelay);
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the exported function `embedDashboard` in
`superset-embedded-sdk/src/index.ts`
(the async function that contains the timer code; see the timer declaration
at lines
252-259). The function schedules the refresh timers at line 259.
2. Ensure the supplied `fetchGuestToken()` returns a JWT whose expiry makes
`getGuestTokenRefreshTiming(...)` return a negative value. The timing
computation is in
`superset-embedded-sdk/src/guestTokenRefresh.ts` (getGuestTokenRefreshTiming
at lines
~26-33) which returns `ttl - REFRESH_TIMING_BUFFER_MS` and can be negative
for near-expiry
tokens.
3. When `embedDashboard` runs, the code calls `setTimeout(...,
getGuestTokenRefreshTiming(guestToken))` at line 259 with a negative delay.
In browsers
negative delays are treated as 0, so the timer fires immediately.
4. Each immediate fire runs `refreshGuestToken` (defined at lines 252-256)
which again
computes a potentially negative delay and calls `setTimeout` (line 255).
This results in
effectively immediate successive refreshes (tight loop) and continuous
network requests
until the page is reloaded.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-embedded-sdk/src/index.ts
**Line:** 252:259
**Comment:**
*Type Error: Using `NodeJS.Timeout` and `setTimeout` without the DOM
`window` context can cause a type mismatch in browser builds (where
`setTimeout` returns a number). Also scheduling with an unbounded value
returned from `getGuestTokenRefreshTiming` can result in a negative delay being
passed to `setTimeout`, which is treated as 0 and can create a very
tight/continuous refresh loop. Change the timer id type to `number | null`,
call `window.setTimeout` to get a numeric id in DOM environments, and clamp the
delay to a non-negative value.
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.
```
</details>
--
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]
