Skip to content

Conversation

@topperge
Copy link

@topperge topperge commented Mar 23, 2025

Fixes #1916

Implemented common fixes for HTML escapes that have appeared when commands are generated in the RooCode Extension and then executed in the terminal


For more details, open the Copilot Workspace session.


Important

Fixes HTML escape issues in terminal commands by sanitizing inputs using sanitizeHtmlEscapes() in multiple files.

  • Behavior:
    • Fixes HTML escape issues in terminal commands by introducing sanitizeHtmlEscapes() in execute-command.ts, Terminal.ts, and TerminalProcess.ts.
    • Ensures commands and paths are sanitized before execution to prevent errors.
  • Functions:
    • Adds sanitizeHtmlEscapes() to handle HTML escape sequences in execute-command.ts, Terminal.ts, and TerminalProcess.ts.
    • Updates getExecuteCommandDescription() in execute-command.ts to use sanitizeHtmlEscapes() for command and cwd parameters.
    • Updates runCommand() in Terminal.ts and run() in TerminalProcess.ts to sanitize commands.
  • Misc:
    • Updates createTerminal() in TerminalRegistry.ts to sanitize cwd using sanitizeHtmlEscapes().

This description was created by Ellipsis for f5f230a. It will automatically update as commits are pushed.

Fixes RooCodeInc#1916

Implemented common fixes for HTML escapes that have appeared when commands are generated in the RooCode Extension and then executed in the terminal

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/RooVetGit/Roo-Code/issues/1916?shareId=XXXX-XXXX-XXXX-XXXX).
@changeset-bot
Copy link

changeset-bot bot commented Mar 23, 2025

⚠️ No Changeset found

Latest commit: f5f230a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Mar 23, 2025

const cwdString = cwd.toString()
const newTerminal = new Terminal(this.nextTerminalId++, terminal, cwdString)
const sanitizedCwd = TerminalProcess.sanitizeHtmlEscapes(cwdString)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling TerminalProcess.sanitizeHtmlEscapes(cwdString) here is problematic because the sanitizeHtmlEscapes method in TerminalProcess is declared as a private instance method and not static. To reuse this functionality across modules (as needed here for HTML escape sanitization), consider extracting it to a shared utility module or making it a public static method. This follows our modular design and prevents cross-module private access.

@mrubens
Copy link
Collaborator

mrubens commented Mar 24, 2025

@KJ7LNW would you be able to give feedback on this one since it touches the terminal code? Thank you!

@KJ7LNW
Copy link
Contributor

KJ7LNW commented Mar 24, 2025

@KJ7LNW would you be able to give feedback on this one since it touches the terminal code? Thank you!

If I understand correctly, this is an HTML entity escaping problem before being passed to the command, in which case it is not the responsibility of the terminal handler to process escape sequences

However, if we are really getting & symbols into the XML requests from the model, then this really should be handled at the global xml parser because terminal is not the only place that this could be a problem.

@topperge, please

  1. reproduce the issue in Roo
  2. Click this button at the top of the task image and download the conversation with the model
  3. paste the conversation here:
```xml
... paste ...
```

@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap Mar 26, 2025
teddyOOXX pushed a commit to teddyOOXX/Roo-Code that referenced this pull request Mar 28, 2025
teddyOOXX pushed a commit to teddyOOXX/Roo-Code that referenced this pull request Mar 28, 2025
…ooCodeInc#1917) (RooCodeInc#1942)

* Reapply "Add IS_DEV and Hot Reloading to debug. (RooCodeInc#1895)" (RooCodeInc#1917)

This reverts commit 25ea46a.

* Update TODO to be more explicit. Update logic for checking IS_DEV

* Update TODO with even more explanation.  (Now with 2x more explanation per explanation
@mrubens
Copy link
Collaborator

mrubens commented Apr 1, 2025

I think #2120 should cover most of this, but let me know if you're seeing anything sneak through!

@mrubens mrubens closed this Apr 1, 2025
@github-project-automation github-project-automation bot moved this from PR [Pre Approval Review] to Done in Roo Code Roadmap Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RooCode Doesn't Properly Handle HTML Escapes when running commands in the terminal

3 participants