Skip to content

fix(security): Handle multi-token command substitution in sendText sanitizer#236

Open
aakashmandavilli96 wants to merge 1 commit intoaws:1.7from
aakashmandavilli96:fix/sanitize-multi-token-cmd-substitution
Open

fix(security): Handle multi-token command substitution in sendText sanitizer#236
aakashmandavilli96 wants to merge 1 commit intoaws:1.7from
aakashmandavilli96:fix/sanitize-multi-token-cmd-substitution

Conversation

@aakashmandavilli96
Copy link
Copy Markdown
Collaborator

Problem

The terminal sendText sanitizer only escaped $() within single whitespace-delimited tokens. Multi-token payloads like $(curl google.com)/test.py span two tokens ($(curl and google.com)/test.py), so the $( was never matched and the command substitution executed:

/opt/conda/bin/python "/home/sagemaker-user/$(curl google.com)/test.py"

Fix

Adds a first-pass regex that matches complete $(...), ${...}, and backtick constructs (including internal spaces) followed by / before per-token processing.

Three-pass approach:

  1. Escape complete substitution constructs followed by / (handles multi-token payloads)
  2. Escape patterns inside double-quoted paths containing /
  3. Escape residual patterns in unquoted path-like tokens containing /

Testing

  • Patch applies cleanly via sh ./scripts/install.sh
  • Compilation succeeds
  • Manually verified: $(curl google.com)/test.py\$(curl google.com)/test.py

Related: aws/code-editor#207

…nitizer

Add first-pass regex to match complete $(...), ${...}, and backtick
constructs followed by / before per-token processing. This prevents
bypass via multi-token payloads like $(curl google.com)/test.py where
the space inside $() caused the old per-token matcher to miss it.

Three-pass approach:
1. Escape complete substitution constructs followed by /
2. Escape patterns inside double-quoted paths with /
3. Escape residual patterns in unquoted path-like tokens
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants