From a73c443ecdacb734f2e8141d5ff4d45b0fb397c7 Mon Sep 17 00:00:00 2001 From: Charles Yao Date: Tue, 22 Sep 2026 08:52:37 -0700 Subject: [PATCH] fix(dify-builder): scrub only real credential keys and exact URL hosts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keys are credentials by their last segment (api_key, access_token, Authorization), not by containing "token" or "password", so password_policy / token_limit defaults survive. URL hosts stop at the first non-URL character and drop trailing punctuation, so a goal that names a host at the end of a sentence or next to CJK text is recognised. Placeholder tokens like <…> only count under a credential key or an auth scheme. Co-Authored-By: Claude Opus 5 (1M context) --- api/services/dify_builder/agent/build.py | 37 +++++----- .../dify_builder/agent/user_supplied.py | 70 +++++++++++++++++-- .../dify_builder/test_build_cognition.py | 43 ++++++++++++ .../dify_builder/test_user_supplied.py | 56 +++++++++++++++ 4 files changed, 180 insertions(+), 26 deletions(-) diff --git a/api/services/dify_builder/agent/build.py b/api/services/dify_builder/agent/build.py index 1cc16510e6c..01715d39145 100644 --- a/api/services/dify_builder/agent/build.py +++ b/api/services/dify_builder/agent/build.py @@ -59,22 +59,22 @@ def analyze_goal( return {"fields": form_schema.reconcile_form_fields(fields, scrubbed_values), "values": scrubbed_values} -# A dict key naming a credential -- checked against ``is_user_supplied_secret`` -# rather than the plain placeholder regex, so a realistic-looking but -# never-stated secret (e.g. a made-up ``sk-live-...`` string) is still caught -# even though it doesn't match any of CREDENTIAL_PLACEHOLDER_RE's shapes. -_CREDENTIAL_KEY_RE = re.compile(r"(?i)(authorization|api[_-]?key|token|secret|password)") - -# Mirrors the prefix ``user_supplied.is_user_supplied_secret`` strips, so a -# literal is checked against CREDENTIAL_PLACEHOLDER_RE the same way regardless -# of which of the two credential checks below is doing the looking. -_AUTH_SCHEME_PREFIX_RE = re.compile(r"^(?:bearer|basic|token)\s+", re.IGNORECASE) - - def _is_invented_literal(value: Any, key: str | None, goal_text: str, trusted_hosts: set[str]) -> bool: """True when ``value`` (found under ``key``, possibly nested inside a field's dict/list default) is a URL or credential the LLM invented - rather than one the goal actually states.""" + rather than one the goal actually states. + + An off-goal URL blanks the field regardless of key. A credential-shaped + placeholder (``<...>``, ``YOUR_API_KEY``, ...) only blanks it when the + value is under a credential-named key (``user_supplied.is_credential_key``, + decided by the key's LAST segment -- not by containing "token" or + "password" anywhere) or the value itself starts with an auth scheme + (``Bearer ``/``Basic ``/``Token ``); otherwise an ordinary default like + ``"Dear ,"`` would be blanked for no reason. A + credential-keyed value that isn't placeholder-shaped but also never + appears in the goal (e.g. an invented ``sk-live-...`` string) is still + caught via ``is_user_supplied_secret``. + """ if isinstance(value, dict): return any(_is_invented_literal(v, k, goal_text, trusted_hosts) for k, v in value.items()) if isinstance(value, list): @@ -83,14 +83,11 @@ def _is_invented_literal(value: Any, key: str | None, goal_text: str, trusted_ho return False if user_supplied.url_hosts(value) - trusted_hosts: return True - stripped = _AUTH_SCHEME_PREFIX_RE.sub("", value, count=1) - if user_supplied.CREDENTIAL_PLACEHOLDER_RE.search(stripped): + credential_key = key is not None and user_supplied.is_credential_key(key) + stripped = user_supplied.strip_auth_scheme(value) + if (credential_key or stripped != value) and user_supplied.CREDENTIAL_PLACEHOLDER_RE.search(stripped): return True - if ( - key is not None - and _CREDENTIAL_KEY_RE.search(key) - and not user_supplied.is_user_supplied_secret(value, goal_text) - ): + if credential_key and not user_supplied.is_user_supplied_secret(value, goal_text): return True return False diff --git a/api/services/dify_builder/agent/user_supplied.py b/api/services/dify_builder/agent/user_supplied.py index b9f51c610b7..ff9ef47298f 100644 --- a/api/services/dify_builder/agent/user_supplied.py +++ b/api/services/dify_builder/agent/user_supplied.py @@ -19,10 +19,17 @@ from collections.abc import Mapping from typing import Any from urllib.parse import urlsplit -# A URL literal as it appears embedded in freeform prose -- stops at -# whitespace or a quoting/bracketing character so "https://a.com/x." at the -# end of a sentence doesn't pull in trailing punctuation. -_URL_RE = re.compile(r"https?://[^\s\"'<>]+", re.IGNORECASE) +# A URL literal as it appears embedded in freeform prose. Restricted to the +# ASCII characters RFC 3986 allows in a URI, so it stops before CJK or other +# non-URL text glued onto the URL with no separating space (e.g. a Chinese +# sentence with no space before or after the URL). Trailing sentence +# punctuation this still picks up (a period ending a sentence, say) is +# stripped separately below, since ``.`` is itself a valid URI character. +_URL_RE = re.compile(r"https?://[A-Za-z0-9._~:/?#\[\]@!$&'()*+,;=%-]+", re.IGNORECASE) + +# Punctuation that ends a sentence or closes a bracket/quote rather than +# belonging to the URL itself, stripped from the end of each regex match. +_URL_TRAILING_PUNCTUATION = ".,;:!?)]'\"" # A Dify workflow template reference, e.g. ``{{#node1.url#}}``. It names a # variable, not a literal value, so it can never be "invented" the way a @@ -44,6 +51,21 @@ CREDENTIAL_PLACEHOLDER_RE = re.compile( re.IGNORECASE, ) +# A dict/field key's last word-segment that, by itself, names a credential +# regardless of what comes before it (so "access_token" and "auth" both +# qualify, but "token_limit" does not -- "limit" is the last segment). +_CREDENTIAL_LAST_SEGMENTS = {"authorization", "auth", "token", "secret", "password", "passwd"} + +# A last segment of "key" only counts as a credential when the segment (or +# merged word) before it names what kind of key it is -- otherwise ordinary +# fields like "key_points" would match on "key" alone. +_CREDENTIAL_KEY_PREFIXES = {"api", "access", "secret", "private", "auth"} + +# Splits an identifier into camelCase words: an uppercase run immediately +# followed by "Xy" (e.g. the "API" in "APIKey"), or an optional leading +# capital plus a run of lowercase/digits (e.g. "api", "Key", "3"). +_CAMEL_SEGMENT_RE = re.compile(r"[A-Z]+(?=[A-Z][a-z])|[A-Z]?[a-z0-9]+|[A-Z]+|[0-9]+") + def url_hosts(text: str) -> set[str]: """Lowercase hostnames of every ``http(s)://…`` URL literally present in @@ -51,8 +73,9 @@ def url_hosts(text: str) -> set[str]: text actually names.""" hosts: set[str] = set() for raw_url in _URL_RE.findall(text or ""): + url = raw_url.rstrip(_URL_TRAILING_PUNCTUATION) try: - host = urlsplit(raw_url).hostname + host = urlsplit(url).hostname except ValueError: continue if host: @@ -80,12 +103,19 @@ def is_user_supplied_url(url: str, trusted_text: str) -> bool: return host.lower() in url_hosts(trusted_text) +def strip_auth_scheme(value: str) -> str: + """Strip a leading ``Bearer ``/``Basic ``/``Token `` auth-scheme prefix + (case-insensitive) from ``value``, if present; otherwise return it + unchanged. Shared so every credential check strips the same way.""" + return _AUTH_SCHEME_PREFIX_RE.sub("", value or "", count=1) + + def is_user_supplied_secret(value: str, trusted_text: str) -> bool: """True iff ``value`` -- after stripping a leading ``Bearer ``/``Basic ``/``Token `` auth-scheme prefix -- is a literal the user actually wrote, rather than an LLM's placeholder for a secret it does not have. """ - remainder = _AUTH_SCHEME_PREFIX_RE.sub("", value or "", count=1) + remainder = strip_auth_scheme(value) if not remainder: return False if _TEMPLATE_MARKER in remainder: @@ -95,6 +125,34 @@ def is_user_supplied_secret(value: str, trusted_text: str) -> bool: return remainder in (trusted_text or "") +def _key_segments(key: str) -> list[str]: + """Lowercase word-segments of a dict/field key, split on ``_``, ``-``, + ``.``, whitespace, and camelCase boundaries -- so ``x-api-key``, + ``apiKey`` and ``api_key`` all segment to ``["api", "key"]``.""" + segments: list[str] = [] + for part in re.split(r"[_\-.\s]+", key or ""): + if not part: + continue + segments.extend(match.lower() for match in _CAMEL_SEGMENT_RE.findall(part)) + return segments + + +def is_credential_key(key: str) -> bool: + """True iff ``key`` names a credential, decided by its LAST segment -- + not by containing a credential-ish substring anywhere. So + ``password_policy``, ``token_limit``, ``session_token_expiry`` and + ``max_tokens`` are NOT credential keys, but ``api_key``, ``apiKey``, + ``x-api-key``, ``access_token`` and ``Authorization`` are. + """ + segments = _key_segments(key) + if not segments: + return False + last = segments[-1] + if last in _CREDENTIAL_LAST_SEGMENTS or last == "apikey": + return True + return last == "key" and len(segments) >= 2 and segments[-2] in _CREDENTIAL_KEY_PREFIXES + + def trusted_text_for(goal_text: str, requirements: Mapping[str, Any]) -> str: """The text a candidate URL/credential is checked against: what the user actually submitted -- the goal prose plus the requirements form they diff --git a/api/tests/unit_tests/services/dify_builder/test_build_cognition.py b/api/tests/unit_tests/services/dify_builder/test_build_cognition.py index d545074f8a2..a15a962e372 100644 --- a/api/tests/unit_tests/services/dify_builder/test_build_cognition.py +++ b/api/tests/unit_tests/services/dify_builder/test_build_cognition.py @@ -143,6 +143,49 @@ def test_scrub_invented_defaults_blanks_a_credential_the_goal_does_not_state(): assert out["api_key"] == "" +def test_scrub_invented_defaults_keeps_lookalike_keys_that_are_not_credentials(): + # ESQ1-302 fix round 1: an unanchored credential-key regex blanked these + # (token_limit contains "token", password_policy contains "password", + # session_token_expiry contains "token") even though none of them holds + # a secret. Credential-ness is decided by the key's LAST segment. + values = { + "password_policy": "strong", + "token_limit": "unlimited", + "session_token_expiry": "30 minutes", + } + + out = build._scrub_invented_defaults(values, _S5B_GOAL) + + assert out == values + + +def test_scrub_invented_defaults_keeps_a_non_credential_placeholder_looking_string(): + # ESQ1-302 fix round 1: CREDENTIAL_PLACEHOLDER_RE's "<...>" shape must + # not blank an ordinary templated-greeting default that happens to use + # angle brackets. + values = {"greeting_template": "Dear ,"} + + out = build._scrub_invented_defaults(values, _S5B_GOAL) + + assert out == values + + +def test_scrub_invented_defaults_blanks_a_bearer_placeholder_under_a_non_credential_key(): + values = {"x": "Bearer YOUR_API_KEY"} + + out = build._scrub_invented_defaults(values, _S5B_GOAL) + + assert out["x"] == "" + + +def test_scrub_invented_defaults_blanks_a_placeholder_under_a_credential_key(): + values = {"api_key": ""} + + out = build._scrub_invented_defaults(values, _S5B_GOAL) + + assert out["api_key"] == "" + + def test_analyze_goal_scrubs_invented_defaults_end_to_end(monkeypatch): captured: dict[str, str] = {} raw = { diff --git a/api/tests/unit_tests/services/dify_builder/test_user_supplied.py b/api/tests/unit_tests/services/dify_builder/test_user_supplied.py index af7de9dea99..7fb34ffc9e1 100644 --- a/api/tests/unit_tests/services/dify_builder/test_user_supplied.py +++ b/api/tests/unit_tests/services/dify_builder/test_user_supplied.py @@ -95,3 +95,59 @@ def test_trusted_text_for_combines_goal_and_requirements_json(): def test_trusted_text_for_empty_requirements(): assert us.trusted_text_for("goal only", {}) == "goal only\n{}" + + +# --- Fix round 1: is_credential_key decides on the key's LAST segment ----- + + +def test_is_credential_key_true_for_real_credential_keys(): + for key in ("api_key", "apiKey", "x-api-key", "access_token", "Authorization"): + assert us.is_credential_key(key), key + + +def test_is_credential_key_false_for_lookalike_keys(): + for key in ("password_policy", "token_limit", "session_token_expiry", "max_tokens", "key_points"): + assert not us.is_credential_key(key), key + + +def test_is_credential_key_false_for_empty_key(): + assert us.is_credential_key("") is False + + +# --- Fix round 1: url_hosts stops at the first non-URL character ---------- + + +def test_url_hosts_drops_trailing_sentence_punctuation(): + text = "… to https://render.acme.internal. Thanks." + assert us.url_hosts(text) == {"render.acme.internal"} + + +def test_url_hosts_stops_before_cjk_text(): + text = "请调用https://api.x.com接口完成任务" + assert us.url_hosts(text) == {"api.x.com"} + + +def test_is_user_supplied_url_true_with_url_at_end_of_sentence(): + trusted = "… to https://render.acme.internal. Thanks." + assert us.is_user_supplied_url("https://render.acme.internal/v2/pptx", trusted) is True + + +def test_url_hosts_lowercases_host_with_port(): + assert us.url_hosts("https://API.Example.com:8443/x") == {"api.example.com"} + + +# --- Fix round 1: strip_auth_scheme is a shared, exported helper ---------- + + +def test_strip_auth_scheme_strips_known_prefixes_case_insensitively(): + assert us.strip_auth_scheme("Bearer sk-live-abc123") == "sk-live-abc123" + assert us.strip_auth_scheme("basic sk-live-abc123") == "sk-live-abc123" + assert us.strip_auth_scheme("TOKEN sk-live-abc123") == "sk-live-abc123" + + +def test_strip_auth_scheme_leaves_a_plain_value_unchanged(): + assert us.strip_auth_scheme("sk-live-abc123") == "sk-live-abc123" + + +def test_strip_auth_scheme_handles_empty_value(): + assert us.strip_auth_scheme("") == ""