Skip to content

Conversation

@mrubens
Copy link
Collaborator

@mrubens mrubens commented Aug 25, 2025

Important

Replaces subshell detection with dangerous substitution detection in command validation to enhance security against parameter expansion vulnerabilities.

  • Behavior:
    • Replaces containsSubshell with containsDangerousSubstitution in command-validation.ts to detect dangerous parameter expansions.
    • Updates getCommandDecision to prevent auto-approval for commands with dangerous substitutions.
  • Tests:
    • Adds tests for containsDangerousSubstitution in command-validation.spec.ts to cover various dangerous patterns like ${var@P}, octal, hex, and unicode escapes.
    • Removes subshell detection tests and adds tests for dangerous substitution handling in getCommandDecision.
  • Misc:
    • Updates comments and documentation to reflect the change from subshell detection to dangerous substitution detection.

This description was created by Ellipsis for 038ae47. You can customize this summary. It will automatically update as commits are pushed.

@mrubens mrubens requested review from cte and jr as code owners August 25, 2025 15:16
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 25, 2025
@mrubens mrubens changed the title Remove unused code Handle substitution patterns in command validation Aug 25, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I've reviewed the changes and found that the implementation correctly addresses the security vulnerability by replacing subshell detection with dangerous substitution detection. The code quality is excellent with comprehensive test coverage. I have a few suggestions for improvement.


// Return true if any subshell pattern is detected
return commandSubstitutionPatterns || subshellGroupingPattern
export function containsDangerousSubstitution(source: string): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great implementation of the dangerous substitution detection! The function correctly identifies the ${var@P} vulnerability and other dangerous parameter expansions. Is it intentional that we're not checking for Unicode escape sequences like \u0060 which could also represent backticks? This might be worth considering for completeness.

const hereStringWithSubstitution = /<<<\s*(\$\(|`)/.test(source)

// Return true if any dangerous pattern is detected
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

Performance consideration: The function now checks 4 different regex patterns sequentially. While this is likely not a bottleneck, have you considered combining some patterns or short-circuiting on the most common cases first if this function is called frequently?

expect(isAutoApprovedSingleCommand("npm test", [])).toBe(false)
})
})

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent test coverage for the dangerous substitution patterns! The tests comprehensively cover the exact exploit pattern from the security report. Consider adding a test case for Unicode escape sequences (\u0060) if you decide to support them.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 25, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 25, 2025
@mrubens mrubens merged commit 3dd3d7b into main Aug 25, 2025
18 of 21 checks passed
@mrubens mrubens deleted the subshell_escaping_fixes branch August 25, 2025 16:41
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 25, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants