surshar / claude-code
Claude Code is an agentic coding tool that lives in your terminal, understands your codebase, and helps you code faster by executing routine tasks, explaining complex code, and handling git workflows - all through natural language commands.
Built by Anthropic · github.com/anthropics
A finding quotes the offending line, so when the reviewer flagged a hardcoded secret the Stop hook's feedback and the previous_findings snapshot in ~/.claude/security carried the credential itself back into the main conversation and onto disk (#96276, follow-up report). Values recognized as credentials in vulnerableCode / fix / evidence text are now masked to **** plus the last two characters at every boundary to the main session and before they are stored: the value after a password / secret / token / api_key-style key (code, JSON, YAML, .env, XML, Dockerfile ENV), passwords in connection URLs, Authorization and curl -u credentials, CLI credential flags, well-known API-key formats and private-key blocks, plus, in findings classed as hardcoded secrets, other quoted literals and long opaque tokens. The reviewer prompts are unchanged so diff anchoring and the refute pass keep working on the raw quote; masking happens on the way out. The commit-review guidance formatter now reuses format_findings.
| @@ -94,6 +94,8 @@ | ||
| 94 | 94 | |
| 95 | 95 | The flip side: the reviewer cannot flag a problem inside a file it never sees, and a project's own `.claude/settings.json` decides what is hidden — the same trust Claude Code itself places in project settings. Each withheld path is named in the debug log (`secretpaths: withheld from review …`). |
| 96 | 96 | |
| 97 | +Anything else that changed in the working tree during the turn is reviewed, including files your own `PreToolUse` hooks would have stopped the main session from reading: the content reaches the reviewer as a diff, not through a tool call, so those hooks never run. What comes back into the main conversation is the finding, not the file, and before a finding is emitted or kept in session state the plugin masks the credential values it can recognize in the quoted code — the value after a `password` / `secret` / `token` / `api_key`-style key (code, JSON, YAML, `.env`, XML, Dockerfile `ENV`), passwords in connection URLs, `Authorization` and `curl -u` credentials, well-known API-key formats, private-key blocks, and, in a finding classed as a hardcoded secret, any other quoted literal or long opaque token — to `****` plus the last two characters, so the main session learns where the secret is without receiving it. This is pattern matching, not a guarantee: add a `Read(...)` deny rule for any file whose content must not reach the review endpoint at all. | |
| 98 | + | |
| 97 | 99 | Where that data goes depends on your Claude Code configuration: |
| 98 | 100 | - **Default (Anthropic API / subscription):** sent to `api.anthropic.com` and handled under Anthropic's [Commercial Terms](https://www.anthropic.com/legal/commercial-terms) and [Privacy Policy](https://www.anthropic.com/legal/privacy). |
| 99 | 101 | - **LLM gateway** (`ANTHROPIC_BASE_URL` set): sent to your gateway URL instead. The gateway operator's terms apply. |
| @@ -614,28 +614,7 @@ | ||
| 614 | 614 | """ |
| 615 | 615 | if not vulns: |
| 616 | 616 | return None |
| 617 | - severity_order = {"critical": 0, "high": 1, "medium": 2} | |
| 618 | - vulns = sorted(vulns, key=lambda v: severity_order.get(v.get("severity", "medium"), 2)) | |
| 619 | - by_file: Dict[str, list] = {} | |
| 620 | - for v in vulns: | |
| 621 | - by_file.setdefault(v.get("filePath", "unknown"), []).append(v) | |
| 622 | - lines = [ | |
| 623 | - "Security Review: Potential vulnerabilities detected", | |
| 624 | - "", | |
| 625 | - f"Affected files: {', '.join(by_file)}", | |
| 626 | - "The following issues were flagged by automated security review. Address each, or briefly note why it doesn't apply. Valid reasons to proceed without changes: the user explicitly asked for this and you've already surfaced the security tradeoffs, or the pattern isn't actually exploitable in this context. Do not dismiss findings solely because the service is internal-only — internal services are common SSRF/IDOR targets:", | |
| 627 | - "", | |
| 628 | - ] | |
| 629 | - n = 1 | |
| 630 | - for fp, vs in by_file.items(): | |
| 631 | - lines.append(f" {fp}:") | |
| 632 | - for v in vs: | |
| 633 | - sev = (v.get("severity") or "medium").upper() | |
| 634 | - lines.append(f" {n}. [{sev}] [{v.get('category', 'Unknown')}] {v.get('vulnerableCode', 'N/A')}") | |
| 635 | - lines.append(f" Suggested fix: {v.get('fix', 'N/A')}") | |
| 636 | - lines.append("") | |
| 637 | - n += 1 | |
| 638 | - return "\n".join(lines) | |
| 617 | + return review_api.format_findings(vulns) | |
| 639 | 618 | |
| 640 | 619 | |
| 641 | 620 | # CC truncates the rewakeSummary override at 300 chars. Cap a little under so |
| @@ -1704,9 +1683,10 @@ | ||
| 1704 | 1683 | lines.append("") |
| 1705 | 1684 | for i, concern in enumerate(concerns, 1): |
| 1706 | 1685 | severity = concern.get('severity', 'high').upper() |
| 1707 | - lines.append(f" {i}. [{severity}] [{concern.get('category', 'Unknown')}] {concern.get('area', '')}") | |
| 1708 | - lines.append(f" Evidence: {concern.get('evidenceLine', 'N/A')}") | |
| 1709 | - lines.append(f" Check: {concern.get('concern', '')}") | |
| 1686 | + cat = concern.get('category', 'Unknown') | |
| 1687 | + lines.append(f" {i}. [{severity}] [{cat}] {review_api.redact_secret_values(concern.get('area', ''), category=cat)}") | |
| 1688 | + lines.append(f" Evidence: {review_api.redact_secret_values(concern.get('evidenceLine', 'N/A'), category=cat)}") | |
| 1689 | + lines.append(f" Check: {review_api.redact_secret_values(concern.get('concern', ''), category=cat)}") | |
| 1710 | 1690 | lines.append("") |
| 1711 | 1691 | |
| 1712 | 1692 | return "\n".join(lines) |
| @@ -16,6 +16,7 @@ | ||
| 16 | 16 | |
| 17 | 17 | import json |
| 18 | 18 | import os |
| 19 | +import re | |
| 19 | 20 | from typing import Any |
| 20 | 21 | |
| 21 | 22 | import extensibility |
| @@ -344,6 +345,362 @@ | ||
| 344 | 345 | return candidates |
| 345 | 346 | |
| 346 | 347 | |
| 348 | +# --------------------------------------------------------------------------- | |
| 349 | +# Secret-value redaction for feedback text | |
| 350 | +# --------------------------------------------------------------------------- | |
| 351 | +# | |
| 352 | +# Findings quote the diff (`vulnerableCode`, `fix`, `evidenceLine`), and the | |
| 353 | +# formatted block is fed back into the main conversation and kept in session | |
| 354 | +# state. For a hardcoded-secret finding that quote IS the secret, so a value | |
| 355 | +# that only ever lived in a working-tree file gets copied into model context, | |
| 356 | +# the transcript and ~/.claude. The reviewer is deliberately NOT asked to | |
| 357 | +# elide values itself: diff anchoring, dedupe and the refute pass all compare | |
| 358 | +# the quoted code against the diff. Masking happens here, on the way out. | |
| 359 | +# Keep the key name and the last two characters so the finding still points | |
| 360 | +# at the right line; when unsure, mask: a garbled evidence line costs less | |
| 361 | +# than an echoed credential. Pattern matching, not a guarantee — the README | |
| 362 | +# says so and points at Read(...) deny rules for files that must never leave. | |
| 363 | + | |
| 364 | +# Identifier segments that make an assignment's left-hand side a credential. | |
| 365 | +_SECRET_KEY_WORDS = frozenset(( | |
| 366 | + "password", "passwd", "pwd", "pass", "passphrase", "secret", "secrets", | |
| 367 | + "token", "apikey", "apitoken", "authtoken", "accesstoken", "refreshtoken", | |
| 368 | + "secretkey", "privatekey", "accesskey", "signingkey", "masterkey", | |
| 369 | + "credential", "credentials", "creds", "authorization", "bearer", "dsn", | |
| 370 | + "connectionstring", "connstr", "connstring", | |
| 371 | +)) | |
| 372 | +# Joined spellings: PGPASSWORD, DBPASSWORD, CLIENTSECRET, GHTOKEN. | |
| 373 | +_SECRET_KEY_SUFFIXES = ("password", "passwd", "passphrase", "secret", "token", "apikey") | |
| 374 | +_SECRET_KEY_PAIRS = frozenset(( | |
| 375 | + ("api", "key"), ("private", "key"), ("secret", "key"), ("access", "key"), | |
| 376 | + ("signing", "key"), ("master", "key"), ("encryption", "key"), | |
| 377 | + ("client", "secret"), ("auth", "token"), ("auth", "key"), | |
| 378 | + ("conn", "str"), ("connection", "string"), | |
| 379 | +)) | |
| 380 | +# ...unless another segment says the value is metadata about a credential. | |
| 381 | +_NOT_SECRET_KEY_WORDS = frozenset(( | |
| 382 | + "timeout", "expiry", "expires", "expire", "expiration", "ttl", "age", | |
| 383 | + "length", "len", "max", "min", "size", "count", "limit", "attempts", | |
| 384 | + "url", "uri", "endpoint", "host", "path", "file", "filename", "dir", | |
| 385 | + "name", "names", "id", "ids", "type", "field", "fields", "header", "param", | |
| 386 | + "hash", "hashed", "hasher", "hashers", "digest", "alg", "algorithm", | |
| 387 | + "reset", "csrf", "xsrf", "enabled", "disabled", "required", "use", | |
| 388 | + "policy", "rotation", "validator", "validators", "regex", "pattern", | |
| 389 | +)) | |
| 390 | +# A dotted "key" ending in one of these is a file name (`secrets.yml:12`). | |
| 391 | +_FILE_EXTENSIONS = frozenset(( | |
| 392 | + "yml", "yaml", "json", "toml", "ini", "cfg", "conf", "env", "properties", | |
| 393 | + "xml", "txt", "md", "py", "js", "ts", "jsx", "tsx", "go", "rb", "java", | |
| 394 | + "kt", "rs", "ex", "exs", "php", "cs", "sh", "swift", "scala", "c", "h", | |
| 395 | + "cpp", "tf", "tfvars", "sql", "lock", "pem", "key", | |
| 396 | +)) | |
| 397 | +_BARE_VALUE_SKIP = frozenset(( | |
| 398 | + "true", "false", "null", "none", "nil", "yes", "no", "required", | |
| 399 | + "optional", "string", "str", "redacted", "await", "new", | |
| 400 | +)) | |
| 401 | +_CAMEL_RE = re.compile(r"(?<=[a-z0-9])(?=[A-Z])") | |
| 402 | +_SEGMENT_SPLIT_RE = re.compile(r"[_\-.$\s]+") | |
| 403 | +# Assignment/comparison operator. The key before it is found by walking back | |
| 404 | +# over identifier characters, which keeps the scan linear on hostile input | |
| 405 | +# (the quoted code comes from the repository under review). | |
| 406 | +_ASSIGN_OP_RE = re.compile(r"(?:[!=]?==|!=|=>|:=|[:=])[ \t]*") | |
| 407 | +_KEY_CHARS = frozenset("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_$.-") | |
| 408 | +# `password: str = "..."`, `apiKey: string = '...'`, `password: &str = "..."`. | |
| 409 | +_TYPE_ANNOTATION_RE = re.compile(r"[&*]?[A-Za-z_][\w.<>\[\]&|?]*(?:[ \t]+=(?!=)[ \t]*|=(?=[\"'bfru@]))") | |
| 410 | +_STRING_PREFIX_RE = re.compile(r"(?:[bBfFrRuU]{1,2}|@)(?=[\"'])") | |
| 411 | +_AUTH_SCHEME_RE = re.compile(r"(?i)(?:basic|bearer|token|digest|apikey|negotiate)[ \t]+") | |
| 412 | +_BARE_TOKEN_RE = re.compile(r"[^\s,;&)}\]\"'`]+") | |
| 413 | +_DOTTED_REFERENCE_RE = re.compile(r"[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)+$") | |
| 414 | +_CONSTANT_NAME_RE = re.compile(r"[A-Z][A-Z0-9_]*$") | |
| 415 | +# What may precede a key that opens a config line: indentation, a diff or | |
| 416 | +# list marker, a quote, a `file:line:` prefix, `export`/`ENV`/`set`. | |
| 417 | +_LINE_LEAD_RE = re.compile( | |
| 418 | + r"[\s+>\"'-]*(?:[\w./\\-]+:\d+:[ \t]*)?(?:(?:export|ENV|ARG|set|SET|setx)[ \t]+)?$" | |
| 419 | +) | |
| 420 | +# Dockerfile `ENV DB_PASSWORD value` (the `=`-less form). | |
| 421 | +_DOCKER_ENV_RE = re.compile(r"(?m)^([ \t+-]*(?:ENV|ARG)[ \t]+([A-Za-z_]\w*)[ \t]+)([^\s=\"']+)$") | |
| 422 | +# <password>value</password> style markup. | |
| 423 | +_XML_VALUE_RE = re.compile(r"(<([A-Za-z_][\w.-]*)[^>]*>)([^<]{3,})(</)") | |
| 424 | +# curl -u user:pass / --user 'user:pass' | |
| 425 | +_CURL_USER_RE = re.compile(r"((?:\s-u[ \t]*|--user[= \t]+)[\"']?[^\s:'\"]*:)([^\s'\"]+)") | |
| 426 | +# --password VALUE / --token VALUE / --api-key VALUE | |
| 427 | +_CLI_FLAG_RE = re.compile( | |
| 428 | + r"(?i)((?<![\w-])--?(?:password|passwd|passphrase|token|secret|api[_-]?key|access[_-]?key" | |
| 429 | + r"|secret[_-]?key|client[_-]?secret|auth[_-]?token)[ \t]+)([^\s\"'$<{-][^\s\"']*)" | |
| 430 | +) | |
| 431 | +# Bearer <token> outside an Authorization assignment (prose, logs). | |
| 432 | +_BEARER_RE = re.compile(r"(?i)(\bbearer\s+)((?=[A-Za-z0-9._~+/=-]*\d)[A-Za-z0-9._~+/=-]{8,})") | |
| 433 | +# scheme://user:password@host and scheme://:password@host | |
| 434 | +_URL_CRED_RE = re.compile( | |
| 435 | + r"((?<![A-Za-z0-9+.-])[A-Za-z][A-Za-z0-9+.-]*://[^\s/:@]*:)([^\s/]{3,})(@)(?=[^\s/@]*(?:[/?#]|$))" | |
| 436 | +) | |
| 437 | +# Well-known token shapes, wherever they appear. | |
| 438 | +_TOKEN_SHAPES_RE = re.compile( | |
| 439 | + r"\b(?:sk-ant-[\w-]{10,}|sk_(?:live|test)_\w{8,}|sk-[\w-]{20,}" | |
| 440 | + r"|AKIA[0-9A-Z]{16}|ASIA[0-9A-Z]{16}|gh[pousr]_\w{20,}|github_pat_\w{20,}" | |
| 441 | + r"|xox[abprs]-[\w-]{10,}|glpat-[\w-]{20,}|AIza[\w-]{35}" | |
| 442 | + r"|eyJ[\w-]{10,}\.[\w-]{10,}\.[\w-]{10,})\b" | |
| 443 | +) | |
| 444 | +_PEM_BODY_RE = re.compile( | |
| 445 | + r"(-----BEGIN [A-Z ]*PRIVATE KEY[A-Z ]*-----)[\s\S]*?(-----END [A-Z ]*PRIVATE KEY[A-Z ]*-----|$)" | |
| 446 | +) | |
| 447 | +# A finding the reviewer itself classed as an exposed credential also gets | |
| 448 | +# the broad masks: quoted literals and long opaque runs anywhere in the text, | |
| 449 | +# since the value may sit in a positional argument or in the fix prose. | |
| 450 | +_SECRET_CATEGORY_RE = re.compile( | |
| 451 | + r"(?i)secret|credential|passw|passphrase|api[ _-]?key|private[ _-]?key" | |
| 452 | + r"|access[ _-]?key|token|key material|cwe-?(?:259|798|321|312|313)" | |
| 453 | +) | |
| 454 | +_NOT_SECRET_CATEGORY_RE = re.compile( | |
| 455 | + r"(?i)csrf|xsrf|reset|expir|invalidat|missing|validat|verif|hash|length" | |
| 456 | + r"|strength|polic|logging|logged|fixation|predictab|entropy|jwt alg" | |
| 457 | +) | |
| 458 | +_QUOTED_RE = re.compile(r"([\"'`])([^\"'`\s]{8,})(\1)") | |
| 459 | +# Quoted things that are not values: names, file names, templates. | |
| 460 | +_QUOTED_KEEP_RE = re.compile(r"[A-Z][A-Z0-9_]*$|[A-Za-z][A-Za-z_-]*[-_][A-Za-z]+$|[${<%(~.]") | |
| 461 | +_LABEL_RE = re.compile(r"[A-Za-z][A-Za-z_ .-]{0,30}[=:][ \t]*$") # "password=" log labels | |
| 462 | +_ATTRIBUTE_NAME_RE = re.compile(r"(?i)(?:key|name|id|for|class|type)=$") | |
| 463 | +_PATH_SEGMENT_RE = re.compile(r"[a-z0-9_.@+~-]+$") | |
| 464 | +_FILE_EXT_RE = re.compile(r"\w\.[A-Za-z][A-Za-z0-9]{0,4}$") | |
| 465 | +_OPAQUE_RUN_RE = re.compile(r"(?<![\w/.*+-])[A-Za-z0-9_+/-]{16,}={0,2}(?![\w*=]|\.[A-Za-z])") | |
| 466 | + | |
| 467 | + | |
| 468 | +def _mask(value: str) -> str: | |
| 469 | + return "****" + value[-2:] if len(value) >= 8 else "****" | |
| 470 | + | |
| 471 | + | |
| 472 | +def _looks_like_path(text: str) -> bool: | |
| 473 | + if text.startswith(("/", "./", "../", "~/")) or "://" in text: | |
| 474 | + return True | |
| 475 | + segments = [seg for seg in re.split(r"[/\\]", text) if seg] | |
| 476 | + return len(segments) > 1 and ( | |
| 477 | + all(_PATH_SEGMENT_RE.match(seg) for seg in segments) | |
| 478 | + or any(_FILE_EXT_RE.search(seg) for seg in segments) | |
| 479 | + ) | |
| 480 | + | |
| 481 | + | |
| 482 | +def _keep_assigned(value: str) -> bool: | |
| 483 | + """A quoted right-hand side is a literal; keep it only when it is a | |
| 484 | + template/reference placeholder or a path, never for its shape.""" | |
| 485 | + return bool( | |
| 486 | + value.startswith(("****", "$", "%(", "<", "{{", "~/")) | |
| 487 | + or ("{" in value and "}" in value) or "%s" in value or "$(" in value | |
| 488 | + or ("/" in value and _looks_like_path(value)) | |
| 489 | + or value.lower() in _BARE_VALUE_SKIP | |
| 490 | + ) | |
| 491 | + | |
| 492 | + | |
| 493 | +def _keep_quoted(inner: str) -> bool: | |
| 494 | + """True for a quoted string that reads as a name, reference, path, URL | |
| 495 | + or template rather than a value: those carry the finding's location.""" | |
| 496 | + return bool( | |
| 497 | + inner.startswith("****") | |
| 498 | + or _LABEL_RE.match(inner) | |
| 499 | + or ("{" in inner and "}" in inner) or "%s" in inner or "$(" in inner | |
| 500 | + or _QUOTED_KEEP_RE.match(inner) | |
| 501 | + or _DOTTED_REFERENCE_RE.match(inner) | |
| 502 | + or _FILE_EXT_RE.search(inner) | |
| 503 | + or (("/" in inner or "\\" in inner) and _looks_like_path(inner)) | |
| 504 | + ) | |
| 505 | + | |
| 506 | + | |
| 507 | +def _is_secret_key(key: str) -> bool: | |
| 508 | + segments = [ | |
| 509 | + seg for seg in _SEGMENT_SPLIT_RE.split(_CAMEL_RE.sub("_", key).lower()) if seg | |
| 510 | + ] | |
| 511 | + if not segments or any(seg in _NOT_SECRET_KEY_WORDS for seg in segments): | |
| 512 | + return False | |
| 513 | + if "." in key and key.rsplit(".", 1)[-1].lower() in _FILE_EXTENSIONS: | |
| 514 | + return False | |
| 515 | + for seg in segments: | |
| 516 | + if seg in _SECRET_KEY_WORDS: | |
| 517 | + return True | |
| 518 | + if not seg.startswith(("csrf", "xsrf")) and seg.endswith(_SECRET_KEY_SUFFIXES) \ | |
| 519 | + and any(0 < len(seg) - len(w) <= 6 for w in _SECRET_KEY_SUFFIXES if seg.endswith(w)): | |
| 520 | + return True | |
| 521 | + return any(pair in _SECRET_KEY_PAIRS for pair in zip(segments, segments[1:])) | |
| 522 | + | |
| 523 | + | |
| 524 | +def _is_secret_category(category: str) -> bool: | |
| 525 | + return bool(_SECRET_CATEGORY_RE.search(category)) and not _NOT_SECRET_CATEGORY_RE.search(category) | |
| 526 | + | |
| 527 | + | |
| 528 | +def _mask_assignments(s: str) -> str: | |
| 529 | + """`key = value`, `key: value`, `"key": "value"`, `cfg['key'] = 'value'`, | |
| 530 | + `key: Type = "value"`, `key == 'value'` where the key names a credential. | |
| 531 | + A quoted value is masked whole (spaces, punctuation and all); a bare | |
| 532 | + value is masked as one token, or to end of line when the key opens the | |
| 533 | + line (YAML, .env, ini, a quoted `+` diff line).""" | |
| 534 | + out: list[str] = [] | |
| 535 | + pos = 0 | |
| 536 | + length = len(s) | |
| 537 | + while True: | |
| 538 | + m = _ASSIGN_OP_RE.search(s, pos) | |
| 539 | + if not m: | |
| 540 | + out.append(s[pos:]) | |
| 541 | + return "".join(out) | |
| 542 | + i = m.end() | |
| 543 | + out.append(s[pos:i]) | |
| 544 | + pos = i | |
| 545 | + # Walk back: spaces, an optional quote/bracket closing a subscript or | |
| 546 | + # JSON key, then the identifier itself. | |
| 547 | + j = m.start() | |
| 548 | + while j > 0 and s[j - 1] in " \t": | |
| 549 | + j -= 1 | |
| 550 | + key_quote = "" | |
| 551 | + if j > 0 and s[j - 1] == "]": | |
| 552 | + j -= 1 | |
| 553 | + if j > 0 and s[j - 1] in "\"'": | |
| 554 | + key_quote = s[j - 1] | |
| 555 | + j -= 1 | |
| 556 | + key_end = key_start = j | |
| 557 | + while key_start > 0 and s[key_start - 1] in _KEY_CHARS: | |
| 558 | + key_start -= 1 | |
| 559 | + while key_start < key_end and s[key_start] in "-.$0123456789": | |
| 560 | + key_start += 1 | |
| 561 | + key = s[key_start:key_end] | |
| 562 | + if i >= length or not key or not _is_secret_key(key): | |
| 563 | + continue | |
| 564 | + ch = s[i] | |
| 565 | + line_start = s.rfind("\n", 0, key_start) + 1 | |
| 566 | + # `log("password=" + pw)`, `"...?api_key=" + key`: an odd number of | |
| 567 | + # quotes before this one means it closes the literal the key sits in, | |
| 568 | + # and what follows is code. | |
| 569 | + if not key_quote and ch in "\"'" and s.count(ch, line_start, i) % 2 == 1: | |
| 570 | + continue | |
| 571 | + if m.group(0).rstrip() == ":": | |
| 572 | + typed = _TYPE_ANNOTATION_RE.match(s, i) | |
| 573 | + if typed: | |
| 574 | + out.append(typed.group(0)) | |
| 575 | + pos = i = typed.end() | |
| 576 | + if i >= length: | |
| 577 | + continue | |
| 578 | + ch = s[i] | |
| 579 | + prefix_m = _STRING_PREFIX_RE.match(s, i) | |
| 580 | + if prefix_m: | |
| 581 | + out.append(prefix_m.group(0)) | |
| 582 | + pos = i = prefix_m.end() | |
| 583 | + ch = s[i] | |
| 584 | + if ch in "\"'`": | |
| 585 | + close = s.find(ch, i + 1) | |
| 586 | + newline = s.find("\n", i + 1) | |
| 587 | + if close == -1 or (newline != -1 and newline < close): | |
| 588 | + close = newline if newline != -1 else length | |
| 589 | + inner = s[i + 1:close] | |
| 590 | + scheme = _AUTH_SCHEME_RE.match(inner) | |
| 591 | + prefix = scheme.group(0) if scheme else "" | |
| 592 | + value = inner[len(prefix):] | |
| 593 | + if value and not _keep_assigned(value): | |
| 594 | + value = _mask(value) | |
| 595 | + out.append(ch + prefix + value) | |
| 596 | + pos = close | |
| 597 | + continue | |
| 598 | + scheme = _AUTH_SCHEME_RE.match(s, i) | |
| 599 | + if scheme: | |
| 600 | + out.append(scheme.group(0)) | |
| 601 | + pos = i = scheme.end() | |
| 602 | + token_m = _BARE_TOKEN_RE.match(s, i) | |
| 603 | + if not token_m: | |
| 604 | + continue | |
| 605 | + token = token_m.group(0) | |
| 606 | + end = token_m.end() | |
| 607 | + if token.startswith(("****", "$", "{", "<", "%", "&", "*", "@")) or "(" in token \ | |
| 608 | + or "[" in token or token.lower() in _BARE_VALUE_SKIP \ | |
| 609 | + or _DOTTED_REFERENCE_RE.match(token): | |
| 610 | + continue | |
| 611 | + opens_line = bool(_LINE_LEAD_RE.match(s[line_start:key_start])) | |
| 612 | + # `-H 'X-Api-Key: value'`: the key opens a quoted string, the value ends it. | |
| 613 | + opens_string = not key_quote and key_start > line_start and s[key_start - 1] in "\"'" | |
| 614 | + has_digit = any(c.isdigit() for c in token) | |
| 615 | + # In code a bare right-hand side is a reference; bare literals live in | |
| 616 | + # config lines (YAML, .env, ini, Dockerfile), where the key opens the | |
| 617 | + # line or a quoted string. Mid-line, only a token carrying digits | |
| 618 | + # reads as a value, and an ALL_CAPS one reads as a constant's name. | |
| 619 | + if not (opens_line or opens_string) and (_CONSTANT_NAME_RE.match(token) or not has_digit): | |
| 620 | + continue | |
| 621 | + # `key: v a l u e` runs on to the end of the line (or of the string); | |
| 622 | + # `KEY=value cmd`, a shell prefix assignment, is one token. | |
| 623 | + if (opens_line or opens_string) and (m.group(0)[-1] in " \t" or m.group(0).startswith(":")): | |
| 624 | + line_end = s.find("\n", end) | |
| 625 | + line_end = length if line_end == -1 else line_end | |
| 626 | + if opens_string: | |
| 627 | + close = s.find(s[key_start - 1], end, line_end) | |
| 628 | + line_end = close if close != -1 else line_end | |
| 629 | + rest = s[end:line_end] | |
| 630 | + comment = rest.find(" #") | |
| 631 | + if comment != -1: | |
| 632 | + rest = rest[:comment] | |
| 633 | + if len(rest.split()) > 3 and not opens_string: | |
| 634 | + if not has_digit: | |
| 635 | + continue # more than a few words after `Key:` is a sentence | |
| 636 | + elif "(" not in rest and "[" not in rest: | |
| 637 | + end += len(rest.rstrip()) | |
| 638 | + token = s[i:end] | |
| 639 | + out.append(_mask(token)) | |
| 640 | + pos = end | |
| 641 | + | |
| 642 | + | |
| 643 | +def _mask_docker_env(m: "re.Match[str]") -> str: | |
| 644 | + value = m.group(3) | |
| 645 | + if not _is_secret_key(m.group(2)) or value.startswith(("$", "{", "<", "****")): | |
| 646 | + return m.group(0) | |
| 647 | + return m.group(1) + _mask(value) | |
| 648 | + | |
| 649 | + | |
| 650 | +def _mask_xml(m: "re.Match[str]") -> str: | |
| 651 | + value = m.group(3).strip() | |
| 652 | + if not _is_secret_key(m.group(2)) or value.lower() in _BARE_VALUE_SKIP \ | |
| 653 | + or value.startswith(("$", "{", "%", "<", "****")): | |
| 654 | + return m.group(0) | |
| 655 | + return m.group(1) + _mask(value) + m.group(4) | |
| 656 | + | |
| 657 | + | |
| 658 | +def _mask_quoted(m: "re.Match[str]") -> str: | |
| 659 | + inner, text, start, end = m.group(2), m.string, m.start(), m.end() | |
| 660 | + after = text[end:end + 4].lstrip(" \t") | |
| 661 | + is_key = after[:1] in (":", "=") and after[1:2] not in ("=", ">", ":") | |
| 662 | + is_subscript = text[max(0, start - 1):start] == "[" and text[end:end + 1] == "]" | |
| 663 | + is_attribute_name = _ATTRIBUTE_NAME_RE.search(text[max(0, start - 8):start]) | |
| 664 | + if is_key or is_subscript or is_attribute_name or _keep_quoted(inner): | |
| 665 | + return m.group(0) | |
| 666 | + return m.group(1) + _mask(inner) + m.group(3) | |
| 667 | + | |
| 668 | + | |
| 669 | +def _mask_opaque(m: "re.Match[str]") -> str: | |
| 670 | + run, text = m.group(0), m.string | |
| 671 | + if text[m.end():m.end() + 1] == "(" or text[max(0, m.start() - 7):m.start()].lower() in ("commit ", "sha "): | |
| 672 | + return run # a call site / a commit reference, not a value | |
| 673 | + digits = sum(ch.isdigit() for ch in run) | |
| 674 | + letters = sum(ch.isalpha() for ch in run) | |
| 675 | + mixed_case = run.lower() != run and run.upper() != run | |
| 676 | + opaque = letters >= 2 and (digits >= 2 or (digits and mixed_case and len(run) >= 32)) | |
| 677 | + if not opaque or ("/" in run and _looks_like_path(run)): | |
| 678 | + return run | |
| 679 | + return _mask(run) | |
| 680 | + | |
| 681 | + | |
| 682 | +def redact_secret_values(text: Any, *, category: Any = "") -> str: | |
| 683 | + """Mask credential-shaped values in a finding field before it is echoed | |
| 684 | + into the main conversation or stored. ``category`` is the finding's own | |
| 685 | + category; one that names an exposed credential turns on the broad masks.""" | |
| 686 | + s = str(text or "") | |
| 687 | + if not s: | |
| 688 | + return s | |
| 689 | + s = _PEM_BODY_RE.sub(lambda m: m.group(1) + " **** " + m.group(2), s) | |
| 690 | + s = _TOKEN_SHAPES_RE.sub(lambda m: _mask(m.group(0)), s) | |
| 691 | + s = _URL_CRED_RE.sub(lambda m: m.group(1) + _mask(m.group(2)) + m.group(3), s) | |
| 692 | + s = _CURL_USER_RE.sub(lambda m: m.group(1) + _mask(m.group(2)), s) | |
| 693 | + s = _CLI_FLAG_RE.sub(lambda m: m.group(1) + _mask(m.group(2)), s) | |
| 694 | + s = _BEARER_RE.sub(lambda m: m.group(1) + _mask(m.group(2)), s) | |
| 695 | + s = _mask_assignments(s) | |
| 696 | + s = _DOCKER_ENV_RE.sub(_mask_docker_env, s) | |
| 697 | + s = _XML_VALUE_RE.sub(_mask_xml, s) | |
| 698 | + if _is_secret_category(str(category or "")): | |
| 699 | + s = _QUOTED_RE.sub(_mask_quoted, s) | |
| 700 | + s = _OPAQUE_RUN_RE.sub(_mask_opaque, s) | |
| 701 | + return s | |
| 702 | + | |
| 703 | + | |
| 347 | 704 | _SEVERITY_ORDER = {"critical": 0, "high": 1, "medium": 2, "low": 3} |
| 348 | 705 | |
| 349 | 706 | |
| @@ -366,7 +723,9 @@ | ||
| 366 | 723 | |
| 367 | 724 | |
| 368 | 725 | def format_findings(findings: list[dict[str, Any]]) -> str: |
| 369 | - """Render findings as the same text block the CC plugin emits to Claude.""" | |
| 726 | + """Render findings, most severe first, as the text block the CC plugin | |
| 727 | + emits to Claude. Quoted code goes through ``redact_secret_values``.""" | |
| 728 | + findings = sorted(findings, key=lambda v: _SEVERITY_ORDER.get(v.get("severity", "medium"), 2)) | |
| 370 | 729 | by_file: dict[str, list[dict[str, Any]]] = {} |
| 371 | 730 | for v in findings: |
| 372 | 731 | by_file.setdefault(v.get("filePath", "unknown"), []).append(v) |
| @@ -388,11 +747,14 @@ | ||
| 388 | 747 | lines.append(f" {fp}:") |
| 389 | 748 | for v in vs: |
| 390 | 749 | sev = (v.get("severity") or "medium").upper() |
| 750 | + cat = v.get("category", "Unknown") | |
| 391 | 751 | lines.append( |
| 392 | - f" {n}. [{sev}] [{v.get('category', 'Unknown')}] " | |
| 393 | - f"{v.get('vulnerableCode', 'N/A')}" | |
| 752 | + f" {n}. [{sev}] [{cat}] " | |
| 753 | + f"{redact_secret_values(v.get('vulnerableCode', 'N/A'), category=cat)}" | |
| 394 | 754 | ) |
| 395 | - lines.append(f" Suggested fix: {v.get('fix', 'N/A')}") | |
| 755 | + lines.append( | |
| 756 | + f" Suggested fix: {redact_secret_values(v.get('fix', 'N/A'), category=cat)}" | |
| 757 | + ) | |
| 396 | 758 | lines.append("") |
| 397 | 759 | n += 1 |
| 398 | 760 | return "\n".join(lines) |
| @@ -830,6 +830,20 @@ | ||
| 830 | 830 | |
| 831 | 831 | COMMIT_REVIEW_ENABLED = is_commit_review_enabled() |
| 832 | 832 | |
| 833 | + | |
| 834 | +def _finding_snapshot(v): | |
| 835 | + """What previous_findings keeps of a finding. Stop and commit review | |
| 836 | + dedupe on (filePath, category); vulnerableCode only feeds the reviewer's | |
| 837 | + do-not-re-flag list, so it is stored masked — the state file must not | |
| 838 | + become a copy of a credential that lived in the working tree.""" | |
| 839 | + category = v.get("category", "Unknown") | |
| 840 | + return { | |
| 841 | + "filePath": v.get("filePath", ""), | |
| 842 | + "category": category, | |
| 843 | + "vulnerableCode": review_api.redact_secret_values(v.get("vulnerableCode", ""), category=category), | |
| 844 | + } | |
| 845 | + | |
| 846 | + | |
| 833 | 847 | def _agentic_review_with_race( |
| 834 | 848 | repo_root: str, |
| 835 | 849 | diff_files: List[Tuple[str, str]], |
| @@ -1335,14 +1349,7 @@ | ||
| 1335 | 1349 | # Record new findings into shared state. Key on (filePath, category) — |
| 1336 | 1350 | # vulnerableCode bytes drift between fires (diff context lines shift) so |
| 1337 | 1351 | # matching on it under-dedupes; this aligns with Stop's _record_fire. |
| 1338 | - finding_snapshots = [ | |
| 1339 | - { | |
| 1340 | - "filePath": v.get("filePath", ""), | |
| 1341 | - "category": v.get("category", "Unknown"), | |
| 1342 | - "vulnerableCode": v.get("vulnerableCode", ""), | |
| 1343 | - } | |
| 1344 | - for v in new_vulns | |
| 1345 | - ] | |
| 1352 | + finding_snapshots = [_finding_snapshot(v) for v in new_vulns] | |
| 1346 | 1353 | |
| 1347 | 1354 | def _record_findings(state): |
| 1348 | 1355 | existing = [f for f in state.get("previous_findings", []) if isinstance(f, dict)] |
| @@ -1663,12 +1670,7 @@ | ||
| 1663 | 1670 | # push-sweep itself won't re-find them; leaving them out of |
| 1664 | 1671 | # previous_findings keeps the door open for the per-commit hook to |
| 1665 | 1672 | # surface them later if the code is touched again. |
| 1666 | - snapshots = [ | |
| 1667 | - {"filePath": v.get("filePath", ""), | |
| 1668 | - "category": v.get("category", "Unknown"), | |
| 1669 | - "vulnerableCode": v.get("vulnerableCode", "")} | |
| 1670 | - for v in reported | |
| 1671 | - ] | |
| 1673 | + snapshots = [_finding_snapshot(v) for v in reported] | |
| 1672 | 1674 | def _record(state): |
| 1673 | 1675 | existing = [f for f in state.get("previous_findings", []) |
| 1674 | 1676 | if isinstance(f, dict)] |
| @@ -1882,14 +1884,7 @@ | ||
| 1882 | 1884 | concrete_guidance = _format_vulns_guidance(vulns) |
| 1883 | 1885 | |
| 1884 | 1886 | if concrete_guidance: |
| 1885 | - finding_snapshots = [ | |
| 1886 | - { | |
| 1887 | - "filePath": v.get("filePath", ""), | |
| 1888 | - "category": v.get("category", "Unknown"), | |
| 1889 | - "vulnerableCode": v.get("vulnerableCode", ""), | |
| 1890 | - } | |
| 1891 | - for v in vulns | |
| 1892 | - ] | |
| 1887 | + finding_snapshots = [_finding_snapshot(v) for v in vulns] | |
| 1893 | 1888 | # Update baseline so next stop hook iteration only sees new changes |
| 1894 | 1889 | new_sha = capture_git_baseline(cwd) |
| 1895 | 1890 | new_untracked_baseline = _list_untracked(cwd) if new_sha else None |
| @@ -1,0 +1,301 @@ | ||
| 1 | +"""Findings are fed back into the main conversation and kept in session | |
| 2 | +state; a hardcoded-secret finding quotes the secret itself. These tests pin | |
| 3 | +that credential-shaped values are masked at every formatting and storage | |
| 4 | +boundary while code quotes and location strings survive | |
| 5 | +(anthropics/claude-code#96276, the reporter's Stop-hook case). | |
| 6 | + | |
| 7 | +Run: python3 -m unittest discover -s plugins/security-guidance/tests -v | |
| 8 | +""" | |
| 9 | + | |
| 10 | +import os | |
| 11 | +import sys | |
| 12 | +import tempfile | |
| 13 | +import time | |
| 14 | +import unittest | |
| 15 | +from unittest import mock | |
| 16 | + | |
| 17 | +HOOKS_DIR = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "hooks") | |
| 18 | +sys.path.insert(0, HOOKS_DIR) | |
| 19 | + | |
| 20 | +_tmp_state = tempfile.mkdtemp(prefix="sg-test-state-") | |
| 21 | +os.environ.setdefault("SECURITY_WARNINGS_STATE_DIR", _tmp_state) | |
| 22 | +os.environ.setdefault("SECURITY_GUIDANCE_DEBUG_LOG", os.path.join(_tmp_state, "log.txt")) | |
| 23 | + | |
| 24 | +import llm # noqa: E402 | |
| 25 | +import review_api # noqa: E402 | |
| 26 | +import security_reminder_hook # noqa: E402 | |
| 27 | + | |
| 28 | +MARKER = "SG_FAKE_MARKER_A2_7f3a91" | |
| 29 | +SECRET_FINDING = { | |
| 30 | + "filePath": "secrets.yml", | |
| 31 | + "category": "Hardcoded Secrets", | |
| 32 | + "vulnerableCode": f"password: {MARKER}", | |
| 33 | + "explanation": "A plaintext credential is committed to the working tree.", | |
| 34 | + "fix": f"Move {MARKER} into a secret manager and read it from the environment.", | |
| 35 | + "severity": "high", | |
| 36 | +} | |
| 37 | +SQLI_FINDING = { | |
| 38 | + "filePath": "app.py", | |
| 39 | + "category": "SQL Injection", | |
| 40 | + "vulnerableCode": 'cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")', | |
| 41 | + "explanation": "user_id flows from the request into the query string.", | |
| 42 | + "fix": "Use a parameterized query: cursor.execute(sql, (user_id,)).", | |
| 43 | + "severity": "high", | |
| 44 | +} | |
| 45 | + | |
| 46 | + | |
| 47 | +def R(text, category="Hardcoded Secrets"): | |
| 48 | + return review_api.redact_secret_values(text, category=category) | |
| 49 | + | |
| 50 | + | |
| 51 | +class MaskedValuesTests(unittest.TestCase): | |
| 52 | + def test_reporter_case_keeps_key_and_masks_value(self): | |
| 53 | + self.assertEqual(R(f"password: {MARKER}"), "password: ****91") | |
| 54 | + self.assertEqual(R(f"db:\n host: db.example.invalid\n password: {MARKER}"), | |
| 55 | + "db:\n host: db.example.invalid\n password: ****91") | |
| 56 | + | |
| 57 | + def test_bare_marker_in_fix_prose_is_masked_for_secret_findings(self): | |
| 58 | + out = R(f"Move {MARKER} out of secrets.yml") | |
| 59 | + self.assertNotIn(MARKER, out) | |
| 60 | + self.assertEqual(out, "Move ****91 out of secrets.yml") | |
| 61 | + | |
| 62 | + def test_quoted_value_is_masked_whole_whatever_it_contains(self): | |
| 63 | + # Punctuation, spaces and brackets inside the quotes must not cut the mask short. | |
| 64 | + self.assertEqual(R("SECRET_KEY = 'django-insecure-)x7@k!p#q2wz&8'"), "SECRET_KEY = '****&8'") | |
| 65 | + self.assertEqual(R("password: 'my secret pass phrase'"), "password: '****se'") | |
| 66 | + self.assertEqual(R('api_key = "abcd1234<efgh5678>ijkl"', "SSRF"), 'api_key = "****kl"') | |
| 67 | + | |
| 68 | + def test_config_line_bare_value_is_masked_to_end_of_line(self): | |
| 69 | + for text, want in ( | |
| 70 | + ("password: correct horse battery staple", "password: ****le"), | |
| 71 | + (" - password: correct horse # dev only", " - password: ****se # dev only"), | |
| 72 | + ("+ password: prodpassword", "+ password: ****rd"), # quoted `+` diff line | |
| 73 | + ("secrets.yml:3: password: prodpassword", "secrets.yml:3: password: ****rd"), | |
| 74 | + ("export API_TOKEN=abcdefgh1", "export API_TOKEN=****h1"), | |
| 75 | + ("DB_PASSWORD=SUPERSECRET99", "DB_PASSWORD=****99"), | |
| 76 | + ("PGPASSWORD=s3cretvalue psql -h db", "PGPASSWORD=****ue psql -h db"), | |
| 77 | + ("PASSWORD=hunter2\nUSER=bob", "PASSWORD=****\nUSER=bob"), | |
| 78 | + ): | |
| 79 | + with self.subTest(text=text): | |
| 80 | + self.assertEqual(R(text, "X"), want) | |
| 81 | + | |
| 82 | + def test_key_forms(self): | |
| 83 | + for text, want in ( | |
| 84 | + ("app.config['SECRET_KEY'] = 'p9Zxnotsosecret'", "app.config['SECRET_KEY'] = '****et'"), | |
| 85 | + ("$config['db']['password'] = 'Pr0duction!';", "$config['db']['password'] = '****n!';"), | |
| 86 | + ("password => 'perlish123'", "password => '****23'"), | |
| 87 | + ('{"password": "hunter2", "user": "bob"}', '{"password": "****", "user": "bob"}'), | |
| 88 | + ("<password>hunter22x</password>", "<password>****2x</password>"), | |
| 89 | + ("X-Api-Key: abcd1234efgh", "X-Api-Key: ****gh"), | |
| 90 | + ("mysql --password=hunter2 -u root", "mysql --password=**** -u root"), | |
| 91 | + ("if token == 'abc123secret':", "if token == '****et':"), | |
| 92 | + ('if (token === "abc123") {', 'if (token === "****") {'), | |
| 93 | + ('if password != "S3cr3tAdm1n":', 'if password != "****1n":'), | |
| 94 | + ('password: str = "hunter2"', 'password: str = "****"'), | |
| 95 | + ('const apiKey: string = "abcd1234efgh5678";', 'const apiKey: string = "****78";'), | |
| 96 | + ('key = b"0123456789abcdef"', 'key = b"0123456789abcdef"'), # `key` alone is not a credential name | |
| 97 | + ('password = b"hunter2xyz"', 'password = b"****yz"'), | |
| 98 | + ("app.secret_key = 'super secret key'", "app.secret_key = '****ey'"), | |
| 99 | + ("self.api_key = 'abcdefgh1234'", "self.api_key = '****34'"), | |
| 100 | + ("password: cGFzc3dvcmQ=", "password: ****Q="), # base64, not a type annotation | |
| 101 | + ('password = "ABCDEF123456"', 'password = "****56"'), # a quoted RHS is a literal | |
| 102 | + ('token = "Zq9.Lm3xTv8pQw2n"', 'token = "****2n"'), | |
| 103 | + ("--token abcdef123456 --verbose", "--token ****56 --verbose"), | |
| 104 | + ("-H 'X-Api-Key: abcdefghijklmnop'", "-H 'X-Api-Key: ****op'"), | |
| 105 | + ("ENV DB_PASSWORD s3cret", "ENV DB_PASSWORD ****"), | |
| 106 | + ("POSTGRES_PASSWORD: postgrespass", "POSTGRES_PASSWORD: ****ss"), | |
| 107 | + ("db.password=changeitplease", "db.password=****se"), | |
| 108 | + ): | |
| 109 | + with self.subTest(text=text): | |
| 110 | + self.assertEqual(R(text, "Insecure Configuration"), want) | |
| 111 | + | |
| 112 | + def test_known_token_shapes_masked_in_any_category(self): | |
| 113 | + for value in ( | |
| 114 | + "sk_live_DUMMYENVMARKER111abc", | |
| 115 | + "AKIAABCDEFGHIJKLMNOP", | |
| 116 | + "ghp_abcdefghijklmnopqrstuvwx0123456789", | |
| 117 | + "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.abcdefghijklmnop", | |
| 118 | + ): | |
| 119 | + with self.subTest(value=value): | |
| 120 | + out = R(f"requests.get(url + '{value}')", "SSRF") | |
| 121 | + self.assertNotIn(value, out) | |
| 122 | + self.assertIn("****" + value[-2:], out) | |
| 123 | + | |
| 124 | + def test_http_credentials(self): | |
| 125 | + self.assertEqual(R('connect("postgres://admin:s3cr3tpass@db/prod")', "SQL Injection"), | |
| 126 | + 'connect("postgres://admin:****ss@db/prod")') | |
| 127 | + self.assertEqual(R("redis://:sup3rs3cretlongvalue99@cache.internal:6379/0", "SSRF"), | |
| 128 | + "redis://:****99@cache.internal:6379/0") | |
| 129 | + self.assertEqual(R("mongodb://root:P@ssw0rd!2024@mongo:27017/db", "SSRF"), | |
| 130 | + "mongodb://root:****24@mongo:27017/db") | |
| 131 | + self.assertEqual(R('{"Authorization": "Bearer abcdEFGH1234ijkl"}', "SSRF"), | |
| 132 | + '{"Authorization": "Bearer ****kl"}') | |
| 133 | + self.assertEqual(R('{"Authorization": "Basic YWRtaW46cGFzc3dvcmQ="}', "SSRF"), | |
| 134 | + '{"Authorization": "Basic ****Q="}') | |
| 135 | + self.assertEqual(R("Authorization: Token 9944b09199c62bcf9418ad846dd0e4bbdfc6ee4b", "IDOR"), | |
| 136 | + "Authorization: Token ****4b") | |
| 137 | + self.assertEqual(R("curl -u admin:Passw0rd! https://host/api", "Command Injection"), | |
| 138 | + "curl -u admin:****d! https://host/api") | |
| 139 | + self.assertEqual(R("curl --user 'admin:Passw0rd!' https://host/api", "Command Injection"), | |
| 140 | + "curl --user 'admin:****d!' https://host/api") | |
| 141 | + | |
| 142 | + def test_private_key_bodies_are_dropped(self): | |
| 143 | + for kind in ("RSA PRIVATE KEY", "PRIVATE KEY", "PGP PRIVATE KEY BLOCK"): | |
| 144 | + pem = f"-----BEGIN {kind}-----\nlQOYBGRk3xkBCAC7n0v9Zq1mX2pL8sT4uY\nabcd\n-----END {kind}-----" | |
| 145 | + with self.subTest(kind=kind): | |
| 146 | + out = R(pem, "Key management") | |
| 147 | + self.assertNotIn("lQOYBGRk", out) | |
| 148 | + self.assertIn(f"-----BEGIN {kind}-----", out) | |
| 149 | + | |
| 150 | + def test_positional_secret_in_secret_category(self): | |
| 151 | + out = R("client = Client('AKIAABCDEFGHIJKLMNOP', 'wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY')", | |
| 152 | + "Hardcoded AWS credentials") | |
| 153 | + self.assertEqual(out, "client = Client('****OP', '****EY')") | |
| 154 | + self.assertEqual(R("jwt.decode(tok, 'mysupersecretkey', algorithms=['HS256'])", "Hardcoded JWT secret"), | |
| 155 | + "jwt.decode(tok, '****ey', algorithms=['HS256'])") | |
| 156 | + self.assertEqual(R('API_KEYS = ["key1abcdx", "key2efghx"]'), 'API_KEYS = ["****dx", "****hx"]') | |
| 157 | + self.assertEqual(R('if ("Adm1nP@ssw0rd!" == pw) {'), 'if ("****d!" == pw) {') | |
| 158 | + self.assertEqual(R("Rotate wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY now"), "Rotate ****EY now") | |
| 159 | + | |
| 160 | + | |
| 161 | +class UnmaskedTextTests(unittest.TestCase): | |
| 162 | + """The finding must still say where the problem is.""" | |
| 163 | + | |
| 164 | + def test_code_quotes_survive(self): | |
| 165 | + for code, category in ( | |
| 166 | + (SQLI_FINDING["vulnerableCode"], "SQL Injection"), | |
| 167 | + ("subprocess.run(cmd, shell=True)", "Command Injection"), | |
| 168 | + ("password_hash = bcrypt.hashpw(pw, salt)", "Weak crypto"), | |
| 169 | + ("password = derive_key(salt)", "Hardcoded Secrets"), | |
| 170 | + ("token = await getToken()", "Hardcoded Secrets"), | |
| 171 | + ("auth_token=request.args['t']", "IDOR"), | |
| 172 | + ("token = request.headers['X-Token']", "Hardcoded Secrets"), | |
| 173 | + ("if token == expected:", "Timing attack"), | |
| 174 | + ("password: ${DB_PASS}", "Hardcoded Secrets"), | |
| 175 | + ("<password>${DB_PASS}</password>", "Hardcoded Secrets"), | |
| 176 | + ('logger.debug("password=" + password)', "Secrets in logs"), | |
| 177 | + ('pw = getpass.getpass("Password: ")', "Hardcoded Secrets"), | |
| 178 | + ): | |
| 179 | + with self.subTest(code=code): | |
| 180 | + self.assertEqual(R(code, category), code) | |
| 181 | + | |
| 182 | + def test_fix_advice_survives(self): | |
| 183 | + for text in ( | |
| 184 | + "Replace with password = os.environ['DB_PASSWORD']", | |
| 185 | + "const apiKey = process.env.API_KEY;", | |
| 186 | + "os.environ.get('DB_PASSWORD')", | |
| 187 | + "Rotate the password: anyone with repo access already has it", | |
| 188 | + "Credentials: move the database password into the environment and rotate it.", | |
| 189 | + "Token: see line 12 of auth.py", | |
| 190 | + "const t = fetchOAuth2AccessTokenForUser(req)", | |
| 191 | + "see commit 9944b09199c62bcf9418ad846dd0e4bbdfc6ee4b", | |
| 192 | + 'password_file = "/run/secrets/db"', | |
| 193 | + "--password $DB_PASS", | |
| 194 | + "Move the value out of `config/database.yml` (use `bin/rails credentials:edit`)", | |
| 195 | + "client.get_secret_value(SecretId='prod/db')['SecretString']", | |
| 196 | + "Replace with `process.env.STRIPE_SECRET_KEY` from `.env.production`", | |
| 197 | + 'headers = {"Content-Type": ctype, "X-Api-Key": key}', | |
| 198 | + ): | |
| 199 | + with self.subTest(text=text): | |
| 200 | + self.assertEqual(R(text, "Hardcoded Secrets"), text) | |
| 201 | + self.assertEqual(R("os.environ.get('DB_PASSWORD', 'changeme123')"), | |
| 202 | + "os.environ.get('DB_PASSWORD', '****23')") | |
| 203 | + | |
| 204 | + def test_locations_survive_in_secret_findings(self): | |
| 205 | + for text in ( | |
| 206 | + "config/settings_v2.py, /api/v1/auth/login endpoint", | |
| 207 | + "src/server/routes/auth2fa.ts — POST /api/v2/sessions", | |
| 208 | + "deploy/k8s/prod-secrets-2024.yaml", | |
| 209 | + "see 'src/Components/App.tsx' and '/etc/app/conf'", | |
| 210 | + "config/secrets.yml:12", | |
| 211 | + "src/auth/token.py:45 — GET /api/v2/users/12345/tokens", | |
| 212 | + "`docker-compose.prod.yml`", | |
| 213 | + ): | |
| 214 | + with self.subTest(text=text): | |
| 215 | + self.assertEqual(R(text, "Hardcoded credential"), text) | |
| 216 | + | |
| 217 | + def test_credential_metadata_keys_are_not_values(self): | |
| 218 | + for text, category in ( | |
| 219 | + ("OAUTH_CALLBACK = 'https://attacker.example.net/cb'", "Open Redirect"), | |
| 220 | + ("auth_url = 'http://sso.example.com/login'", "Cleartext Transmission"), | |
| 221 | + ("author_id = 12345", "IDOR"), | |
| 222 | + ("PASSWORD_RESET_TIMEOUT = 259200", "Auth"), | |
| 223 | + ("MAX_TOKENS = 4096", "LLM"), | |
| 224 | + ("Require bearer authentication on this route", "Missing Authentication"), | |
| 225 | + ): | |
| 226 | + with self.subTest(text=text): | |
| 227 | + self.assertEqual(R(text, category), text) | |
| 228 | + | |
| 229 | + def test_token_named_categories_that_are_not_about_secret_values(self): | |
| 230 | + for text, category in ( | |
| 231 | + ('<form action="/transfer/confirm2" method="POST">', "CSRF token missing"), | |
| 232 | + ("fetch('/api/v1/users/delete', {method: 'POST'})", "Missing CSRF token"), | |
| 233 | + ("headers['X-CSRF-Token'] = getCookie('csrftoken')", "CSRF Token"), | |
| 234 | + ("user = User.objects.get(reset_token=request.GET['t'])", "Password reset token not invalidated"), | |
| 235 | + ): | |
| 236 | + with self.subTest(text=text): | |
| 237 | + self.assertEqual(R(text, category), text) | |
| 238 | + | |
| 239 | + def test_non_string_fields_do_not_raise(self): | |
| 240 | + self.assertEqual(R(None), "") | |
| 241 | + self.assertEqual(R(12345, ["Secrets"]), "12345") | |
| 242 | + out = review_api.format_findings([{"filePath": "a", "category": ["Secrets"], "vulnerableCode": 7, | |
| 243 | + "fix": None, "severity": "high"}]) | |
| 244 | + self.assertIn("[['Secrets']] 7", out) | |
| 245 | + | |
| 246 | + def test_hostile_input_stays_linear(self): | |
| 247 | + # The quoted code comes from the repository under review, so no input | |
| 248 | + # shape may make the scan quadratic: 4x the input stays well under | |
| 249 | + # 16x the time. | |
| 250 | + def timed(probe): | |
| 251 | + started = time.perf_counter() | |
| 252 | + R(probe, "Hardcoded Secrets") | |
| 253 | + return time.perf_counter() - started | |
| 254 | + | |
| 255 | + for unit in ("auth.", "token-", "a", "'b", "a:", " ", "\t", "=", "x.]", '"'): | |
| 256 | + with self.subTest(unit=unit): | |
| 257 | + small = min(timed(unit * 2500) for _ in range(3)) | |
| 258 | + large = min(timed(unit * 10000) for _ in range(3)) | |
| 259 | + self.assertLess(large, small * 10 + 0.02) | |
| 260 | + | |
| 261 | + | |
| 262 | +class BoundaryTests(unittest.TestCase): | |
| 263 | + """Every block that reaches the main conversation, and the state file, | |
| 264 | + goes through the mask.""" | |
| 265 | + | |
| 266 | + def test_guidance_block_masks_secret_but_keeps_location(self): | |
| 267 | + out = llm._format_vulns_guidance([SQLI_FINDING, SECRET_FINDING]) | |
| 268 | + self.assertNotIn(MARKER, out) | |
| 269 | + self.assertIn("secrets.yml:", out) | |
| 270 | + self.assertIn("[Hardcoded Secrets] password: ****91", out) | |
| 271 | + self.assertIn("Suggested fix: Move ****91 into a secret manager", out) | |
| 272 | + self.assertIn(SQLI_FINDING["vulnerableCode"], out) # code quote untouched | |
| 273 | + self.assertEqual(out, review_api.format_findings([SQLI_FINDING, SECRET_FINDING])) | |
| 274 | + | |
| 275 | + def test_concerns_review_masks_evidence_line(self): | |
| 276 | + analysis = { | |
| 277 | + "hasConcerns": True, | |
| 278 | + "concerns": [{ | |
| 279 | + "category": "Hardcoded credential", | |
| 280 | + "area": "secrets.yml", | |
| 281 | + "concern": f"Confirm whether {MARKER} is a live credential.", | |
| 282 | + "evidenceLine": f" password: {MARKER}", | |
| 283 | + "severity": "high", | |
| 284 | + }], | |
| 285 | + } | |
| 286 | + with mock.patch.object(llm, "HAS_API_CREDENTIALS", True), \ | |
| 287 | + mock.patch.object(llm, "_call_claude_dual_or", return_value=analysis): | |
| 288 | + out = llm.analyze_security_concerns([("secrets.yml", f"+ password: {MARKER}\n")], is_diff=True) | |
| 289 | + self.assertIsNotNone(out) | |
| 290 | + self.assertNotIn(MARKER, out) | |
| 291 | + self.assertIn("secrets.yml", out) | |
| 292 | + self.assertIn("Evidence: password: ****91", out) | |
| 293 | + | |
| 294 | + def test_state_snapshot_stores_masked_code(self): | |
| 295 | + snap = security_reminder_hook._finding_snapshot(SECRET_FINDING) | |
| 296 | + self.assertEqual(snap, {"filePath": "secrets.yml", "category": "Hardcoded Secrets", | |
| 297 | + "vulnerableCode": "password: ****91"}) | |
| 298 | + | |
| 299 | + | |
| 300 | +if __name__ == "__main__": | |
| 301 | + unittest.main() |