fix: validate shell_wrapper template against injection

[JanTvrdik]

Summary

• Add validate_shell_wrapper() that rejects templates containing shell metacharacters outside {shell}
• If validation fails, skip the wrapper and use the original shell (with warning log)
• Add unit tests for valid wrappers and various injection patterns

Closes #71

Test plan

• cargo test passes (includes new validation tests)
• Valid wrappers like "direnv exec . {shell}" still work
• Injection attempts like "malicious; {shell}" are rejected

Co-Authored-By: Claude Code

[Copilot]

Pull request overview

This PR mitigates shell command injection risk in the terminal.shell_wrapper setting by validating wrapper templates and falling back to the unwrapped shell when the template is deemed unsafe.

Changes:

• Added validate_shell_wrapper() and a metacharacter denylist check around {shell}.
• Updated apply_shell_wrapper() to warn and skip wrapping when validation fails.
• Adjusted/added unit tests to cover accepted wrappers and rejected injection patterns.

---

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

[Copilot]

validate_shell_wrapper() still allows command separators not in DANGEROUS_PATTERNS, so injection is still possible (e.g. "malicious & {shell}" or "malicious\n{shell}" will pass validation but execute malicious). Consider expanding validation to reject single & and any newline/CR characters (and add unit tests for these cases).

Suggested change

	const DANGEROUS_PATTERNS: &[&str] = &[";", "&&", "||", "|", "`", "$(", ">", "<"];
	/// Validate a `shell_wrapper` template against shell injection.
	///
	/// Splits the wrapper on `{shell}` and checks that the surrounding parts do not contain
	/// dangerous shell metacharacters (`;`, `&&`, `||`, `|`, `` ` ``, `$(`, `>`, `<`).
	/// Returns `Ok(())` if the template is safe, or `Err(message)` describing the problem.
	pub fn validate_shell_wrapper(wrapper: &str) -> Result<(), String> {
	const DANGEROUS_PATTERNS: &[&str] = &[";", "&&", "&", "||", "|", "`", "$(", ">", "<"];
	/// Validate a `shell_wrapper` template against shell injection.
	///
	/// Splits the wrapper on `{shell}` and checks that the surrounding parts do not contain
	/// dangerous shell metacharacters (`;`, `&&`, `&`, `||`, `|`, `` ` ``, `$(`, `>`, `<`),
	/// and that the template does not contain any newline or carriage-return characters.
	/// Returns `Ok(())` if the template is safe, or `Err(message)` describing the problem.
	pub fn validate_shell_wrapper(wrapper: &str) -> Result<(), String> {
	// Disallow multi-line or CR-terminated wrappers, which can be used to inject commands.
	if wrapper.contains('\n') || wrapper.contains('\r') {
	return Err(format!(
	"shell_wrapper must be a single line without newlines or carriage returns: {:?}",
	wrapper,
	));
	}

[Copilot]

validate_shell_wrapper() doesn’t enforce that the template contains {shell} (and ideally exactly once). As written, a wrapper like "nix develop" would pass validation and apply_shell_wrapper() would run it without ever exec'ing into the intended shell, likely breaking terminal startup. Recommend returning an error when {shell} is missing or appears multiple times, and adding a focused test for this behavior.

Suggested change

	let parts: Vec<&str> = wrapper.split("{shell}").collect();
	let parts: Vec<&str> = wrapper.split("{shell}").collect();
	let occurrences = parts.len().saturating_sub(1);
	if occurrences == 0 {
	return Err(format!(
	"shell_wrapper must contain the placeholder {{shell}} exactly once: {:?}",
	wrapper,
	));
	} else if occurrences > 1 {
	return Err(format!(
	"shell_wrapper must contain the placeholder {{shell}} exactly once, but it appears {} times: {:?}",
	occurrences, wrapper,
	));
	}

[matej21]

Same general approach as #85, consistent. Two things:

1. No check that {shell} actually appears in the wrapper. A template like "just_a_program" passes validation but produces broken behavior (shell never executes). Worth adding a guard.
2. The old doc comment about sh -c metacharacter wrapping is still partially present — please update it to match the new validation behavior.

Also — since #81 and #85 share the DANGEROUS_SHELL_PATTERNS concept, consider extracting the pattern list and validate_* logic into a shared helper to keep them in sync.