From 1de47b56edc6290eebe9bfd16e3bf3cddaa326af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=8E=8B=E4=B8=80=E5=B8=86?= Date: Wed, 7 Oct 2026 17:10:45 +0800 Subject: [PATCH] fix(skillscan): detect modern sk API tokens (#6394) * fix(skillscan): detect modern sk API tokens * fix(skillscan): skip placeholders and align notification redaction --- README.md | 2 + backend/app/scheduler/notification_text.py | 2 +- .../harness/deerflow/skills/AGENTS.md | 2 + .../deerflow/skills/skillscan/orchestrator.py | 10 ++- .../test_notification_delivery_worker.py | 22 ++++++ backend/tests/test_skillscan_native.py | 77 +++++++++++++++++++ 6 files changed, 111 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 4a5e235b35..cc9d79ed3b 100644 --- a/README.md +++ b/README.md @@ -1343,6 +1343,8 @@ and cloud-metadata hostname detection is case-insensitive. For Python credential mappings, SkillScan checks literal values while treating ordinary dictionary keys as labels. A mapping such as `tokens = {"access_token": os.getenv("ACCESS_TOKEN")}` does not report a hardcoded credential. Keys matching a recognized cloud or API token format are still checked as embedded credentials. +SkillScan blocks embedded legacy and modern `sk-` API tokens, including OpenAI project, service-account, and admin keys and Anthropic keys, even in Bearer headers without a secret assignment. Token bodies may contain `-` and `_`; finding evidence is fully redacted. Placeholder examples do not suppress detection of later credentials in the same file. Scheduled-task notification summaries also redact these key formats before sending them to external IM platforms. + DeerFlow also ships with **skill-reviewer**, a public skill for read-only skill quality review. It uses the built-in `review_skill_package` tool to inspect installed skills, local packages, archives, or pasted `SKILL.md` content without activating the target skill, binding its secrets, executing its scripts, or installing it. The tool returns a compact, tag-neutralized JSON payload to the model context and keeps the full raw review payload in the tool artifact for programmatic consumers. The deterministic review core reuses DeerFlow parsing and SkillScan facts, emits versioned JSON contracts under `contracts/skill_review/`, and can be run from the backend CLI: ```bash diff --git a/backend/app/scheduler/notification_text.py b/backend/app/scheduler/notification_text.py index f450011d52..83c0e44d30 100644 --- a/backend/app/scheduler/notification_text.py +++ b/backend/app/scheduler/notification_text.py @@ -49,7 +49,7 @@ _EGRESS_SECRET_PATTERNS: tuple[re.Pattern[str], ...] = ( re.compile(r"\b(?:AKIA|ASIA)[A-Z0-9]{16}\b"), re.compile(r"\bgh[pousr]_[A-Za-z0-9_]{20,}\b"), re.compile(r"\bxox[baprs]-[A-Za-z0-9-]{20,}\b"), - re.compile(r"\bsk-[A-Za-z0-9]{20,}\b"), + re.compile(r"\bsk-[A-Za-z0-9_-]{20,}\b"), re.compile(r"(?im)\b(token|password|passwd|api[_-]?key|secret|credential)s?\b\s*[:=]\s*[\"']?([^\"'\s#]+)"), ) diff --git a/backend/packages/harness/deerflow/skills/AGENTS.md b/backend/packages/harness/deerflow/skills/AGENTS.md index 30370f4afc..fcaa73ae39 100644 --- a/backend/packages/harness/deerflow/skills/AGENTS.md +++ b/backend/packages/harness/deerflow/skills/AGENTS.md @@ -1,5 +1,7 @@ ### Skills System (`packages/harness/deerflow/skills/`) +- **SkillScan API tokens**: `secret-cloud-token` accepts `-` and `_` in the `sk-` family, matching the character class used by PII redaction. Legacy, OpenAI project/service-account/admin, and Anthropic keys must trigger `CRITICAL` even in a Bearer header without a secret assignment. Skip placeholder matches until a real credential is found, retaining at most one cloud-token finding per file. Keep evidence fully redacted and preserve the minimum length and word boundaries; synthetic regressions live in `tests/test_skillscan_native.py`. Keep the mirrored `sk-` pattern in `app/scheduler/notification_text.py` aligned so scheduled-run summaries scrub the same key formats before IM delivery. + - **SkillScan shell pipes**: the `shell-curl-pipe-shell` warning covers sudo, common shell interpreter paths/names, and real backslash-newline continuations, including between `sudo`, its flags, and the interpreter. Keep repeated regex alternatives disjoint; failed matches on literal backslash text must not stall scanning. Value-bearing sudo flags (`sudo -u root bash`) are intentionally out of scope for this warning-level heuristic. Tests in `tests/test_skillscan_native.py` include a subprocess timeout regression and non-shell pipe controls. - **Integration package boundaries**: recurse through provider and namespace directories, but stop at each package's `SKILL.md`. Nested fixtures must neither become catalog entries nor shadow real public skills; regression coverage lives in `tests/test_user_scoped_skill_storage.py`. diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index 03920f51ba..c526dbcacc 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -131,7 +131,7 @@ _SECRET_TOKEN_PATTERNS = tuple( r"\b(?:AKIA|ASIA)[A-Z0-9]{16}\b", r"\bgh[pousr]_[A-Za-z0-9_]{20,}\b", r"\bxox[baprs]-[A-Za-z0-9-]{20,}\b", - r"\bsk-[A-Za-z0-9]{20,}\b", + r"\bsk-[A-Za-z0-9_-]{20,}\b", ) ) _SENSITIVE_PATH_RE = re.compile(r"(~/.ssh|/etc/passwd|/etc/shadow|/var/run/docker\.sock|docker\.sock|169\.254\.169\.254)") @@ -361,10 +361,14 @@ def _scan_secrets(rel_path: str, text: str) -> list[SecurityFinding]: findings.append(_finding_from_match("secret-private-key", rel_path, text, private_key)) for pattern in _SECRET_TOKEN_PATTERNS: - match = pattern.search(text) - if match and not _looks_like_placeholder(match.group(0)): + for match in pattern.finditer(text): + if _looks_like_placeholder(match.group(0)): + continue findings.append(_finding_from_match("secret-cloud-token", rel_path, text, match)) break + else: + continue + break if _is_python_path(rel_path, text): findings.extend(_scan_python_secret_assignments(rel_path, text)) diff --git a/backend/tests/test_notification_delivery_worker.py b/backend/tests/test_notification_delivery_worker.py index 485dbabe52..fd7c28d399 100644 --- a/backend/tests/test_notification_delivery_worker.py +++ b/backend/tests/test_notification_delivery_worker.py @@ -16,6 +16,7 @@ from app.channels.base import ChannelUnavailable from app.channels.message_bus import MessageBus from app.channels.wecom import WeComChannel from app.scheduler.notification_delivery import NotificationDeliveryWorker, render_notification_text +from app.scheduler.notification_text import redact_egress_text class FakeDeliveryRepo: @@ -156,6 +157,27 @@ def test_render_truncates_unbounded_error_text(): assert "Error:" not in text +@pytest.mark.parametrize("prefix", ["sk-", "sk-proj-", "sk-svcacct-", "sk-admin-", "sk-ant-api03-"]) +@pytest.mark.asyncio +async def test_delivery_scrubs_sk_bearer_tokens(prefix): + # Synthetic credentials, with no secret assignment to provide a fallback. + secret = prefix + "A1b2_C3d4-E5f6_G7h8-I9j0" * 3 + summary = f"Authorization: Bearer {secret}; Repeated: {secret}" + safe_summary = "Authorization: Bearer [redacted]; Repeated: [redacted]" + assert redact_egress_text(summary) == safe_summary + + repo = FakeDeliveryRepo([_delivery_row()]) + channel = FakeChannel() + worker = _make_worker(repo, channel, resolve_run_summary=lambda *_: summary) + + await worker.run_once(now=datetime.now(UTC)) + + assert repo.sent == ["delivery-1"] + assert len(channel.sent) == 1 + assert safe_summary in channel.sent[0][1] + assert secret not in channel.sent[0][1] + + @pytest.mark.asyncio async def test_run_once_delivers_claimed_row_and_marks_sent(): repo = FakeDeliveryRepo([_delivery_row()]) diff --git a/backend/tests/test_skillscan_native.py b/backend/tests/test_skillscan_native.py index 76159e001a..fd0b6d05dc 100644 --- a/backend/tests/test_skillscan_native.py +++ b/backend/tests/test_skillscan_native.py @@ -95,6 +95,83 @@ def test_secret_evidence_is_redacted_everywhere(tmp_path: Path) -> None: assert all(token not in (blocked_finding["evidence"] or "") for blocked_finding in excinfo.value.findings) +@pytest.mark.parametrize("prefix", ["sk-", "sk-proj-", "sk-svcacct-", "sk-admin-", "sk-ant-api03-"]) +@pytest.mark.parametrize("body", ["A1b2C3d4E5f6G7h8I9j0" * 3, "A1b2_C3d4-E5f6_G7h8-I9j0" * 3]) +def test_sk_bearer_tokens_block_without_assignment(tmp_path: Path, prefix: str, body: str) -> None: + """Synthetic credentials must be detected without an API_KEY/TOKEN assignment.""" + token = prefix + body + skill_dir = tmp_path / "demo-skill" + _write_skill(skill_dir) + scripts_dir = skill_dir / "scripts" + scripts_dir.mkdir() + (scripts_dir / "run.sh").write_text( + f'#!/bin/sh\ncurl -H "Authorization: Bearer {token}" https://api.example.invalid/v1/messages\n', + encoding="utf-8", + ) + + result = scan_skill_dir(skill_dir) + + finding = _finding_by_rule(result["findings"], "secret-cloud-token") + assert finding["severity"] == "CRITICAL" + assert finding["file"] == "scripts/run.sh" + assert finding["line"] == 2 + assert finding["evidence"] == "[redacted]" + assert result["blocked"] is True + assert result["scanner_errors"] == [] + assert not any(item["rule_id"] == "secret-env-assignment" for item in result["findings"]) + assert body not in repr(result) + + with pytest.raises(StaticScanBlockedError) as excinfo: + enforce_static_scan(skill_dir, skill_name="demo-skill", app_config=SimpleNamespace(skill_scan=SimpleNamespace(enabled=True))) + + assert body not in str(excinfo.value) + assert body not in repr(excinfo.value.findings) + + +@pytest.mark.parametrize( + "text", + [ + "sk-" + "A1b2C3d4E5f6G7h8I9j", # Below the existing 20-character minimum. + "sk-proj-short", + "sk-svcacct-short", + "sk-admin-short", + "sk-ant-api03-short", + "sk-proj-your_api_key_goes_here", + "sk-ant-api03-example_api_key_value", + "prefixsk-" + "A1b2C3d4E5f6G7h8I9j0" * 3, + "prefix_sk-proj-" + "A1b2_C3d4-E5f6_G7h8-I9j0" * 3, + ], +) +def test_sk_token_non_credentials_stay_unblocked(tmp_path: Path, text: str) -> None: + skill_dir = tmp_path / "demo-skill" + _write_skill(skill_dir, f"Authorization: Bearer {text}\n") + + result = scan_skill_dir(skill_dir) + + assert not any(finding["rule_id"] == "secret-cloud-token" for finding in result["findings"]) + assert result["blocked"] is False + + +@pytest.mark.parametrize("prefix", ["sk-", "sk-proj-", "sk-svcacct-", "sk-admin-", "sk-ant-api03-"]) +def test_sk_placeholder_does_not_hide_later_credential(tmp_path: Path, prefix: str) -> None: + token = prefix + "A1b2C3d4E5f6G7h8I9j0" * 3 + skill_dir = tmp_path / "demo-skill" + _write_skill( + skill_dir, + f"Authorization: Bearer sk-proj-your_api_key_goes_here\nAuthorization: Bearer sk-ant-api03-example_api_key_value\nAuthorization: Bearer {token}\nAuthorization: Bearer {token}\n", + ) + + result = scan_skill_dir(skill_dir) + + findings = [finding for finding in result["findings"] if finding["rule_id"] == "secret-cloud-token"] + assert len(findings) == 1 # Preserve the first-real-token finding policy. + assert findings[0]["severity"] == "CRITICAL" + assert findings[0]["line"] == 8 + assert findings[0]["evidence"] == "[redacted]" + assert result["blocked"] is True + assert token not in repr(result) + + def test_dedup_keeps_distinct_lines_for_repeated_pattern(tmp_path: Path) -> None: skill_dir = tmp_path / "demo-skill" _write_skill(skill_dir)