feat: implement interactive Rewind Viewer component - #15718
Conversation
Summary of ChangesHello @Adib234, 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 delivers a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new "RewindViewer" component and its tests. A high-severity potential Command Injection vulnerability has been identified in "packages/cli/src/ui/components/RewindViewer.tsx". This is due to untrusted user input from conversation history being passed to an "onRewind" handler without proper sanitization, which could lead to arbitrary command execution if the handler re-runs the prompt. The "onRewind" handler must implement strict input validation and sanitization. Furthermore, the "RewindViewer" component has a critical bug in message selection due to reliance on non-unique message IDs, potentially causing incorrect rewinding. The tests also exhibit issues, such as using non-unique IDs that mask this bug and an incorrect mock for a core utility function.
| onConfirm={(outcome) => { | ||
| if (outcome === RewindOutcome.Cancel) { | ||
| clearSelection(); | ||
| } else { | ||
| const userPrompt = interactions.find( | ||
| (m) => m.id === selectedMessageId, | ||
| ); | ||
| if (userPrompt) { | ||
| const originalUserText = userPrompt.content | ||
| ? partToString(userPrompt.content) | ||
| : ''; | ||
| const cleanedText = stripReferenceContent(originalUserText); | ||
| onRewind(selectedMessageId, cleanedText, outcome); | ||
| } | ||
| } | ||
| }} |
There was a problem hiding this comment.
A high-severity Command Injection vulnerability exists in the "onConfirm" handler. User prompts from conversation history are processed, and "stripReferenceContent" is not a security sanitizer. This means malicious payloads (e.g., a shell command) in user prompts could pass through unmodified as "cleanedText" to the "onRewind" callback. If "onRewind" re-executes this prompt, it leads to arbitrary command execution. The "onRewind" handler must treat the "newText" parameter as untrusted user input and apply proper validation and sanitization before any sensitive operations.
Furthermore, the component's logic for handling message selection is flawed as it relies on "message.id", which is not guaranteed to be unique. If multiple messages share the same ID, "find()" will always pick the first message, potentially leading to rewinding to an incorrect point. Consider managing the selected message state locally within "RewindViewer" by storing the entire "MessageRecord" object upon selection to address this.
There was a problem hiding this comment.
I'm not sure why a shell command would appear in the rewind viewer because it doesn't get recorded in ChatRecordingService
There was a problem hiding this comment.
Also message selection is done in onSelect and it sets the selectedMessageId as userPrompt.id
There was a problem hiding this comment.
Thanks for the clarification, Adib234. Let me elaborate on the points raised:
Command Injection Vulnerability
Your point about the ChatRecordingService not recording shell commands is well-taken. However, the security concern here operates on a principle of defense-in-depth. While the ChatRecordingService might currently filter out shell commands, this is not a guarantee against future vulnerabilities or unexpected scenarios:
- Future Changes: The
ChatRecordingService's filtering logic could change, or a bug could allow malicious content to be recorded. - Compromised Client: A compromised client could directly inject malicious content into the conversation history.
stripReferenceContent's Purpose: ThestripReferenceContentfunction is designed for UI display purposes (removing verbose reference content), not for security sanitization. It only removes specific markers and does not neutralize arbitrary malicious payloads (e.g.,$(rm -rf /)).onRewind's Responsibility: TheonRewindcallback receivescleanedText. If thiscleanedTextis subsequently used in a context where it could be executed (e.g., passed to a shell, anevalfunction, or a tool that executes commands), it must be treated as untrusted user input and properly sanitized or validated at that point of use. Relying solely on upstream filtering is a single point of failure.
Therefore, to ensure robust security, the onRewind handler (or the function it calls with newText) should implement proper validation and sanitization of the newText parameter before any sensitive operations, regardless of how the input was initially recorded.
Flawed Message Selection
You are correct that onSelect sets selectedMessageId using userPrompt.id. However, the issue lies with the subsequent use of interactions.find((m) => m.id === selectedMessageId) within the onConfirm handler. As demonstrated in the RewindViewer.test.tsx file (e.g., lines 110-111, 154-157, 189-190, 227-228, 259-260), multiple MessageRecord objects can share the same id (e.g., both user and Gemini messages often have id: '1').
If interactions contains multiple messages with the same id, Array.prototype.find() will always return the first matching element. This means that if you select a message that happens to have the same id as an earlier message in the interactions array, the onConfirm handler might inadvertently retrieve the wrong message, leading to an incorrect rewind point.
To address this, I recommend using a truly unique identifier for selection. The items array already generates unique key values using both msg.id and idx (key: ${msg.id || 'msg'}-${idx}``). You could leverage this idx or store the entire `MessageRecord` object in the `selectedMessageId` state to ensure that the correct message is always retrieved.
For example, you could modify the onSelect and onConfirm logic to use the index from the items array:
// In useRewindLogic or similar state management
const [selectedIndex, setSelectedIndex] = useState<number | null>(null);
// ...
// In onSelect for BaseSelectionList
onSelect={(itemWrapper) => {
selectMessageByIndex(itemWrapper.index);
}}
// In onConfirm handler
const userPrompt = interactions[selectedIndex];This would guarantee that the exact message selected by the user is the one processed during confirmation.
Adib234
changed the title
0ce347d to
f068be3
Compare
2ddfa74 to
4c4cb62
Compare
gemini-cli
Bot
added
status/need-issue
2 participants
Summary
This PR introduces the RewindViewer component, providing a dedicated, interactive interface for navigating conversation history and selecting rewind points. It features intelligent content filtering to keep the history clean and integrates with the RewindConfirmation flow to safely revert both chat history and code changes.
Details
Interactive History Browsing
Intelligent Content Filtering
stripReferenceContent: A new utility that identifies and removes large blocks of "referenced file content" often injected into prompts. This ensures the viewer shows the user's actual intent rather than walls of technical context.Integrated Rewind Flow
Related Issues
After this PR gets merged here are the other PRs that will get merged in order
#15720
How to Validate
Pre-Merge Checklist