fix(hooks): validate on_create hook commands against shell injection by JanTvrdik · Pull Request #85 · contember/okena

JanTvrdik · 2026-03-27T15:52:07Z

Summary

Add validate_hook_command() that rejects dangerous shell metacharacters (;, &&, ||, |, backticks, $(, >, <) in on_create hook commands
apply_on_create() now returns Option<ShellType> — logs a warning and skips the hook when validation fails
All three call sites updated to handle the new Option return type gracefully

Closes #70

Test plan

Unit tests for validate_hook_command covering simple commands (pass) and all injection vectors (reject)
Integration test confirming apply_on_create returns None for malicious input
Existing apply_on_create_with_env_vars test updated and passing
Full cargo build and cargo test pass

Co-Authored-By: Claude Code

Add validate_hook_command() that rejects dangerous shell metacharacters (;, &&, ||, |, backticks, $(), >, <) in on_create hook commands from project-level settings. Untrusted repos could exploit format!()-based interpolation to chain malicious commands. apply_on_create() now returns Option<ShellType> and logs a warning when validation fails instead of executing the command. Closes contember#70 Co-Authored-By: Claude Code

Copilot

Pull request overview

This PR mitigates shell command injection risk in the terminal.on_create hook by validating hook commands for dangerous shell metacharacters and skipping unsafe hooks instead of blindly interpolating them into a shell script.

Changes:

Added validate_hook_command() and a denylist of dangerous shell metacharacters for terminal.on_create.
Updated apply_on_create() to return Option<ShellType> and skip execution (with a warning) when validation fails.
Updated call sites and expanded tests to cover the new validation behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
`crates/okena-workspace/src/hooks.rs`	Adds validation + changes `apply_on_create` to return `Option`, plus new unit/integration tests.
`src/workspace/actions/execute.rs`	Updates terminal spawn path to handle `apply_on_create()` returning `None`.
`src/views/root/terminal_actions.rs`	Updates shell-switch action to handle `apply_on_create()` returning `None`.
`crates/okena-views-terminal/src/layout/terminal_pane/mod.rs`	Updates terminal pane creation to handle `apply_on_create()` returning `None`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-27T15:55:09Z

crates/okena-workspace/src/hooks.rs

+const DANGEROUS_SHELL_PATTERNS: &[&str] = &[";", "&&", "||", "|", "`", "$(", ">", "<"];
+
+/// Validate that a hook command does not contain shell injection metacharacters.
+/// Project-level hooks from untrusted repos could abuse these to run arbitrary commands.
+/// Returns `Ok(())` if the command is safe, or `Err` with a description of what was rejected.
+pub fn validate_hook_command(cmd: &str) -> Result<(), String> {
+    for pattern in DANGEROUS_SHELL_PATTERNS {
+        if cmd.contains(pattern) {
+            return Err(format!(
+                "hook command contains dangerous shell metacharacter '{}': {}",
+                pattern, cmd


validate_hook_command still allows command chaining via newline (\n/\r) and single & (background/command separator on POSIX; command separator on cmd.exe). Since apply_on_create interpolates on_create_cmd into a shell string, an attacker can still inject additional commands without using any of the currently blocked patterns. Consider rejecting &, \n, and \r (and add unit tests for these cases) so the validation actually closes the injection vector.

Copilot · 2026-03-27T15:55:09Z

crates/okena-workspace/src/hooks.rs

+            return Err(format!(
+                "hook command contains dangerous shell metacharacter '{}': {}",
+                pattern, cmd
+            ));
+        }
+    }
+    Ok(())
+}
+
 /// Apply the `terminal.on_create` command by wrapping the shell to run
 /// the command first, then `exec` into the original shell.
 /// Environment variables are exported so they persist in the shell session.
 /// Produces: `sh -c 'export K=V; ...; <on_create_cmd>; exec <shell_cmd>'`
-pub fn apply_on_create(shell: &ShellType, on_create_cmd: &str, env_vars: &HashMap<String, String>) -> ShellType {
+///
+/// Returns `None` if the command fails validation (shell injection detected).
+pub fn apply_on_create(shell: &ShellType, on_create_cmd: &str, env_vars: &HashMap<String, String>) -> Option<ShellType> {
+    if let Err(e) = validate_hook_command(on_create_cmd) {
+        log::warn!("Skipping terminal.on_create hook: {}", e);
+        return None;


The validation error string includes the full hook command, and this gets logged at warn level. Hook commands can contain secrets (tokens, passwords) and logging them may leak sensitive data into logs/telemetry. Consider logging only the rejected metacharacter (and perhaps the hook source), or truncating/redacting the command in the log message.

Copilot · 2026-03-27T15:55:09Z

crates/okena-workspace/src/hooks.rs

+/// We reject hook commands from project-level settings that contain these,
+/// since they could chain or redirect arbitrary commands.
+const DANGEROUS_SHELL_PATTERNS: &[&str] = &[";", "&&", "||", "|", "`", "$(", ">", "<"];
+
+/// Validate that a hook command does not contain shell injection metacharacters.
+/// Project-level hooks from untrusted repos could abuse these to run arbitrary commands.


Docstrings for DANGEROUS_SHELL_PATTERNS/validate_hook_command say the rejection is for “project-level settings”, but apply_on_create calls validation for any resolved hook (project/parent/global). Either clarify the documentation to match current behavior, or plumb through whether the hook is project-scoped so global/user-configured hooks aren’t unexpectedly restricted.

Suggested change

/// We reject hook commands from project-level settings that contain these,

/// since they could chain or redirect arbitrary commands.

const DANGEROUS_SHELL_PATTERNS: &[&str] = &[";", "&&", "||", "|", "`", "$(", ">", "<"];

/// Validate that a hook command does not contain shell injection metacharacters.

/// Project-level hooks from untrusted repos could abuse these to run arbitrary commands.

/// Hook commands from any scope (project/parent/global) that contain these

/// are considered unsafe, since they could chain or redirect arbitrary commands.

const DANGEROUS_SHELL_PATTERNS: &[&str] = &[";", "&&", "||", "|", "`", "$(", ">", "<"];

/// Validate that a hook command does not contain shell injection metacharacters.

/// This is intended to protect against untrusted hook configurations (for example,

/// commands defined in project repositories) but can be applied to any hook scope.

matej21

Good approach with the metacharacter blocklist. A few gaps to close:

Newline (\n) is missing from DANGEROUS_SHELL_PATTERNS — it's a valid command separator in sh -c and bypasses all current checks. Please add it.
The validation applies to all config levels (global, parent, project). A legitimate global hook with pipes (e.g. grep ... | head) will be silently rejected. Consider scoping validation to project-level hooks only, or at minimum documenting this restriction.
Silent failure (warn log only) — users won't notice their hook was skipped. Consider surfacing a visible warning in the terminal.

Copilot AI review requested due to automatic review settings March 27, 2026 15:52

Copilot started reviewing on behalf of JanTvrdik March 27, 2026 15:52 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

matej21 reviewed Mar 27, 2026

View reviewed changes

matej21 mentioned this pull request Mar 27, 2026

fix: validate shell_wrapper template against injection #81

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(hooks): validate on_create hook commands against shell injection#85

fix(hooks): validate on_create hook commands against shell injection#85
JanTvrdik wants to merge 1 commit intocontember:mainfrom
JanTvrdik:fix/shell-injection-on-create-hook

JanTvrdik commented Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

Copilot AI Mar 27, 2026

Uh oh!

matej21 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

JanTvrdik commented Mar 27, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

matej21 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants