T09 · Insecure Skill Coding Practices
Error
- Location
- redact_patterns.py:96
- Finding
- Generic API keys may remain exposed because unmatched capture groups produce invalid replacement boundaries<![CDATA[ ## Vulnerability Details **File Location**: `redact_patterns.py:96-99`; `redact_kit.py:93-101` **Vulnerability Type**: Incorrect capture-group handling causing incomplete or failed secret redaction **Risk Level**: High ### Vulnerable Code ```python # redact_patterns.py:96-99 pattern=re.compile( r'\b[A-Za-z0-9_-]{32,}\b|' # Generic long alphanumeric r'(?:api[_-]?key|apikey|token|secret)["\']?\s*[:=]\s*["\']?([A-Za-z0-9_-]{16,})' ), ``` ```python # redact_kit.py:93-101 for match in pattern.pattern.finditer(text): # Get match value if match.groups(): # If there are groups, use the first group value = match.group(1) start = match.start(1) end = match.end(1) else: value = match.group(0) start = match.start() end = match.end() ``` ### Technical Analysis The API-key regular expression has two alternatives. The first alternative detects a standalone token of 32 or more characters but does not populate capture group 1. The second alternative detects a labeled secret and places its value in capture group 1. The redaction engine uses `match.groups()` to decide whether group 1 should be used. That method returns the tuple of groups defined by the entire regular expression, even when a particular group did not participate in the selected alternative. For a standalone token matched by the first alternative: - `match.group(1)` returns `None`. - `match.start(1)` and `match.end(1)` return `-1`. - The replacement logic subsequently operates on invalid semantic boundaries. - Report mode can fail when it evaluates `match.original_value[:30]` on `None`. As a result, the output can be corrupted while the original token remains partially or fully exposed. This defeats the primary security purpose of the project. ### Attack Path 1. A user processes text containing a standalone token of at least 32 characters, such as a long API credential without an `api_key=`, `token=`, or similar prefix ...[truncated 1095 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions - Do not infer the intended replacement value merely from the existence of capture groups. - Prefer regular expressions in which the complete sensitive value is consistently represented by `match.group(0)`. - Alternatively, use one named group for each alternative and select the first group whose value is not `None`. - Validate that `start >= 0`, `end >= start`, and `value is not None` before creating a `RedactionMatch`. Fail closed if these invariants are violated. - Separate generic and labeled API-key detection into distinct `RedactionPattern` objects if their match semantics differ. - Add regression tests for both alternatives, including standalone long tokens, labeled tokens, quoted values, and report mode. - Verify that the original credential is absent from `redacted_text`, not merely that a match object was produced. ]]>