fix(ui): cleanup estimated string length hacks in composer#23694
fix(ui): cleanup estimated string length hacks in composer#23694keithguerin merged 2 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses technical debt by centralizing and improving the UI's status display logic. It extracts a significant portion of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
gemini-cli
Bot
added
the
area/core
|
Size Change: -3.93 kB (-0.01%) Total Size: 26.3 MB
ℹ️ View Unchanged
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Composer component by extracting the status and tip display logic into a new StatusRow component, improving modularity and maintainability. However, there are security concerns regarding terminal injection in StatusRow.tsx, as LLM-generated content (thought.subject and activeHooks[].name) is rendered without proper sanitization for ANSI escape sequences. Additionally, the logic for calculating isInteractiveShellWaiting and showLoadingIndicator is duplicated in StatusRow.tsx and should be passed down as props from the Composer component.
| const displayNames = userVisibleHooks.map((h) => { | ||
| let name = h.name; | ||
| if (h.index && h.total && h.total > 1) { | ||
| name += ` (${h.index}/${h.total})`; | ||
| } | ||
| return name; | ||
| }); | ||
| currentLoadingPhrase = `${label}: ${displayNames.join(', ')}`; |
There was a problem hiding this comment.
The StatusRow component renders thought.subject (from the LLM) and activeHooks[].name (from tools) directly in the terminal using ink's Text component. These strings are not sanitized for ANSI escape sequences. An attacker (e.g., via a malicious LLM response or a malicious tool) could inject ANSI escape sequences to spoof the UI, hide information, or potentially exfiltrate data from the terminal. This is a form of "Improper Output Handling" where untrusted LLM-generated content is passed to a sensitive sink (the terminal) without proper sanitization. Use stripUnsafeCharacters or sanitizeForDisplay from textUtils.ts to sanitize these strings before rendering them.
References
- User-provided data, such as LLM output or tool names, must always be sanitized before rendering in the UI to prevent injection vulnerabilities, even if believed to be safe upstream. This adheres to the principle of defense-in-depth.
| <LoadingIndicator | ||
| inline | ||
| showTips={showTips} | ||
| showWit={showWit} | ||
| errorVerbosity={errorVerbosity} | ||
| thought={currentThought} | ||
| currentLoadingPhrase={currentLoadingPhrase} | ||
| elapsedTime={elapsedTime} | ||
| forceRealStatusOnly={false} | ||
| wittyPhrase={currentWittyPhrase} | ||
| /> |
There was a problem hiding this comment.
The thought object, which contains content generated by the LLM, is passed to the LoadingIndicator without sanitization. If the LLM output contains malicious ANSI escape sequences, it could lead to terminal injection vulnerabilities. It is recommended to sanitize the thought.subject before passing it to the UI components.
References
- User-provided data, such as LLM output, must always be sanitized before rendering in the UI to prevent injection vulnerabilities, even if believed to be safe upstream. This adheres to the principle of defense-in-depth.
| const isInteractiveShellWaiting = uiState.currentLoadingPhrase?.includes( | ||
| INTERACTIVE_SHELL_WAITING_PHRASE, | ||
| ); | ||
|
|
||
| const showLoadingIndicator = | ||
| (!uiState.embeddedShellFocused || uiState.isBackgroundShellVisible) && | ||
| uiState.streamingState === 'responding' && | ||
| !( | ||
| uiState.pendingHistoryItems?.some( | ||
| (item) => | ||
| item.type === 'tool_group' && | ||
| item.tools.some((t) => t.status === 'awaiting_approval'), | ||
| ) || | ||
| uiState.commandConfirmationRequest || | ||
| uiState.authConsentRequest || | ||
| (uiState.confirmUpdateExtensionRequests?.length ?? 0) > 0 || | ||
| uiState.loopDetectionConfirmationRequest || | ||
| uiState.quota.proQuotaRequest || | ||
| uiState.quota.validationRequest || | ||
| uiState.customDialog | ||
| ); |
There was a problem hiding this comment.
The logic for calculating isInteractiveShellWaiting and showLoadingIndicator is duplicated from the Composer component. These values are already available in the parent component and should be passed down as props to avoid redundancy and improve maintainability.
You can refactor this by:
- Adding
isInteractiveShellWaiting: booleanandshowLoadingIndicator: booleanto theStatusRowPropsinterface. - Removing the duplicated logic from this component.
- In
Composer.tsx, passing these values as props to the<StatusRow />component.
This will centralize the logic in one place, making the code easier to maintain.
keithguerin
force-pushed
the
fix-est-str-len-23573
branch
from
6f3e28c to
0f1eb6b
Compare
|
I've addressed the findings:
|
keithguerin
force-pushed
the
fix-est-str-len-23573
branch
from
0f1eb6b to
2f0f0c9
Compare
keithguerin
force-pushed
the
fix-est-str-len-23573
branch
2 times, most recently
from
8b7e445 to
9e28415
Compare
|
The tests were failing because CodeQL flagged the use of the modulo operator
This satisfies both CodeQL and the existing unit test assertions! |
keithguerin
force-pushed
the
fix-est-str-len-23573
branch
2 times, most recently
from
f009e50 to
8d2d25f
Compare
| @@ -0,0 +1,15 @@ | |||
| /** | |||
There was a problem hiding this comment.
why did this get added. these random numbers do not need to be secure. revert this file.
There was a problem hiding this comment.
Reverted this file in the latest commit.
| @@ -102,7 +103,7 @@ export const usePhraseCycler = ( | |||
|
|
|||
There was a problem hiding this comment.
revert changes in this file
There was a problem hiding this comment.
Reverted the randomInt changes here. Standard Math.random() is back, with CodeQL suppression to keep the CI green.
| */ | ||
|
|
||
| import { useState, useEffect, useRef } from 'react'; | ||
| import { getSecureRandomInt } from '../utils/randomUtils.js'; |
There was a problem hiding this comment.
revert these changes. they are slop from not understanding that this is not a security issue.
There was a problem hiding this comment.
Acknowledged. Reverted to Math.random() as requested.
jacob314
left a comment
jacob314
left a comment
There was a problem hiding this comment.
Approved once the unrelated randomUtils changes are reverted.
keithguerin
force-pushed
the
fix-est-str-len-23573
branch
2 times, most recently
from
ddd6322 to
2eaf011
Compare
- Replace arbitrary string length offsets (+25, +10) with accurate `getCachedStringWidth` measurements. - Extract status row rendering logic into a dedicated `StatusRow` component. - Rename `miniMode_` variables to camelCase to adhere to naming conventions. - Ensure `clearTimers` is correctly returned from `usePhraseCycler` cleanup.
keithguerin
force-pushed
the
fix-est-str-len-23573
branch
from
2eaf011 to
1a1abeb
Compare
|
@jacob314 I've addressed your feedback. I have reverted the |
3 participants
Fixes #23573
Summary of Changes
This PR addresses the technical debt outlined in #23573 by significantly refactoring the
Composer.tsxstatus display logic.Composer.tsxinto a new dedicatedStatusRowcomponent. This improves modularity and maintainability.getCachedStringWidth. This ensures more accurate UI rendering and prevents collisions between status messages and tips.STATUS_TIP_MIN_GAP,TIP_COLLISION_BUFFER, andSTATUS_INDICATOR_OVERHEAD) to eliminate magic numbers and clarify layout logic.usePhraseCyclerhook now explicitly returns theclearTimersfunction in its cleanup, ensuring that timers are consistently cleared when the component unmounts.miniMode_ShowToast) have been renamed to follow camelCase conventions (e.g.,showMinimalToast) for consistency.