From e54a2cbd2ac4e4ed185e1309e8ebf3d73ac2c095 Mon Sep 17 00:00:00 2001 From: Charles Yao Date: Tue, 22 Sep 2026 09:43:00 -0700 Subject: [PATCH] fix(dify-builder): ground only credentials and host endpoints, never data inputs The test form and mock data skipped every start variable an http node read, so a search query or a language header was never mocked. Only a host-position URL template, credential-keyed header/param lines and an api-key auth now count, with one shared is_credential_key in core. A bare "key" param, an empty credential and a bare "Bearer" are grounded too, and an existing Dify template is never re-grounded. Co-Authored-By: Claude Opus 5 (1M context) --- api/core/dify_builder/credentials.py | 70 +++++++ api/core/dify_builder/handlers_fix.py | 79 ++++++-- api/services/dify_builder/agent/build.py | 86 +++++--- .../dify_builder/agent/user_supplied.py | 56 ++---- .../core/dify_builder/test_handlers_fix.py | 52 ++++- .../services/dify_builder/test_build_nodes.py | 189 ++++++++++++++++++ .../dify_builder/test_user_supplied.py | 29 +++ 7 files changed, 470 insertions(+), 91 deletions(-) create mode 100644 api/core/dify_builder/credentials.py diff --git a/api/core/dify_builder/credentials.py b/api/core/dify_builder/credentials.py new file mode 100644 index 00000000000..fcb5728a83b --- /dev/null +++ b/api/core/dify_builder/credentials.py @@ -0,0 +1,70 @@ +"""Whether a header/param/field key names a credential. + +Pure, no I/O, stdlib-only -- ``core/dify_builder`` must not import from +``services``, so this is the single shared home for ``is_credential_key`` +(moved here in the Task 3 review fix round) and its params-only sibling +``is_credential_param_key``. Both ``core/dify_builder/handlers_fix.py`` +(deciding which start variables a mocked/prefilled form must skip) and +``services/dify_builder/agent/build.py`` (deciding which literals to ground) +need the SAME rule, so there is one definition; ``user_supplied.py`` imports +and re-exports both so existing ``services``-side callers see no API change. +""" + +import re + +# 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 _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 is_credential_param_key(key: str) -> bool: + """``is_credential_key`` PLUS a bare ``key`` (case-insensitive, the WHOLE + key -- not merely its last segment): a raw ``?key=...`` query parameter is + a common way an API names its API key, which ``is_credential_key`` alone + misses (its last-segment rule requires a qualifying prefix like + ``api``/``access`` before a bare ``key``). Params-only -- a header simply + named ``key`` is not a realistic credential header the way a query param + is, so this rule is not applied to headers. ``cache_key``/``key_points`` + still segment to more than just ``key`` and stay non-credential, same as + under ``is_credential_key``. + """ + return is_credential_key(key) or (key or "").strip().lower() == "key" diff --git a/api/core/dify_builder/handlers_fix.py b/api/core/dify_builder/handlers_fix.py index 0e1518976dd..7f6efee3a44 100644 --- a/api/core/dify_builder/handlers_fix.py +++ b/api/core/dify_builder/handlers_fix.py @@ -30,6 +30,7 @@ import re import uuid from typing import Any +from core.dify_builder import credentials from core.dify_builder.changes import describe_changed_nodes from core.dify_builder.contract import ( AssistantTurnItem, @@ -460,22 +461,64 @@ def upload_variable_names(schema: StartSchema) -> set[str]: # ``{{#.#}}`` -- the Dify template reference syntax -- may sit -# anywhere inside a URL string (e.g. ``https://x.com/{{#s.path#}}``), so this -# searches the whole string rather than anchoring to it. +# anywhere inside a string (e.g. ``https://x.com/{{#s.path#}}``), so this +# searches the whole string rather than anchoring to it, except where a +# caller below deliberately anchors it with ``.match``. _ENDPOINT_TEMPLATE_RE = re.compile(r"\{\{#([^.#{}]+)\.([^.#{}]+)#\}\}") +def _start_var_names(text: str, start_id: str) -> set[str]: + """Every start-node template's variable name occurring ANYWHERE in + ``text`` -- used where the field IS the credential/endpoint itself (an + ``authorization.config.api_key``, or the value half of a credential-keyed + header/param line), so position within it doesn't matter.""" + return {var_name for node_id, var_name in _ENDPOINT_TEMPLATE_RE.findall(text or "") if node_id == start_id} + + +def _leading_start_var_name(text: str, start_id: str) -> str | None: + """The variable name of a start-node template occupying the HOST + position: ``text``, stripped of whitespace, STARTS with the template -- + not merely containing one later, e.g. in a path or query + (``https://wttr.in/{{#s.city#}}``'s ``city`` is an ordinary data input, + not an endpoint). Returns None when there is no such leading template. + """ + match = _ENDPOINT_TEMPLATE_RE.match((text or "").strip()) + if match and match.group(1) == start_id: + return match.group(2) + return None + + +def _credential_line_var_names(text: str, start_id: str, key_is_credential) -> set[str]: + """Start-variable templates in the VALUE half of each ``Key: Value`` line + of a ``headers``/``params`` text block whose KEY names a credential per + ``key_is_credential`` -- an ordinary data field read via a header/param + (a search query, an ``Accept-Language``) must stay mockable, so only a + credential-keyed line's value counts.""" + names: set[str] = set() + for line in (text or "").split("\n"): + if ":" not in line: + continue + key, _sep, value = line.partition(":") + if key_is_credential(key.strip()): + names |= _start_var_names(value, start_id) + return names + + def endpoint_variable_names(graph: Graph) -> set[str]: - """Names of the start variables an http-request node reads its URL, - headers, params, or API-key authorization from. + """Names of the start variables an http-request node reads a real + endpoint or credential from: the url's HOST position, a credential-keyed + ``headers``/``params`` line, or an ``api-key`` authorization's + ``config.api_key``. Nothing can invent a real endpoint or credential any more than it can invent an upload (ESQ1-302 / build.py's ``_ground_placeholder_endpoints`` and Task 3's credential grounding), so these are values a human still has - to supply even when every other field is mocked for them. Only a - reference to the graph's own START node counts -- a reference to some - other node's output is an ordinary wired value, not an endpoint/credential - waiting on the user. + to supply even when every other field is mocked for them. An ORDINARY + data input the workflow happens to read via the url path/query, or via a + non-credential header/param (a search query, an ``Accept-Language``), is + NOT collected -- it stays mockable, same as any other start variable. + Only a reference to the graph's own START node counts at all -- a + reference to some other node's output is an ordinary wired value. """ start_id = "" for node in graph.get("nodes", []): @@ -486,23 +529,21 @@ def endpoint_variable_names(graph: Graph) -> set[str]: if not start_id: return set() names: set[str] = set() - - def _collect(text: str) -> None: - for node_id, var_name in _ENDPOINT_TEMPLATE_RE.findall(text): - if node_id == start_id: - names.add(var_name) - for node in graph.get("nodes", []): data = node.get("data") or {} if data.get("type") != "http-request": continue - _collect(str(data.get("url") or "")) - _collect(str(data.get("headers") or "")) - _collect(str(data.get("params") or "")) + leading = _leading_start_var_name(str(data.get("url") or ""), start_id) + if leading: + names.add(leading) + names |= _credential_line_var_names(str(data.get("headers") or ""), start_id, credentials.is_credential_key) + names |= _credential_line_var_names( + str(data.get("params") or ""), start_id, credentials.is_credential_param_key + ) authorization = data.get("authorization") auth_config = authorization.get("config") if isinstance(authorization, dict) else None - if isinstance(auth_config, dict): - _collect(str(auth_config.get("api_key") or "")) + if isinstance(authorization, dict) and authorization.get("type") == "api-key" and isinstance(auth_config, dict): + names |= _start_var_names(str(auth_config.get("api_key") or ""), start_id) return names diff --git a/api/services/dify_builder/agent/build.py b/api/services/dify_builder/agent/build.py index c1242297831..c18235f0202 100644 --- a/api/services/dify_builder/agent/build.py +++ b/api/services/dify_builder/agent/build.py @@ -833,22 +833,44 @@ def _ground_placeholder_endpoints(intents: list[MutationIntent], *, trusted_text return grounded +# A value that IS one of these words and nothing else (no content after it) +# is a bare auth-scheme with no credential attached -- ``strip_auth_scheme`` +# alone does not catch this: its regex only strips a scheme word FOLLOWED BY +# whitespace, so a bare "Bearer" (nothing after it) comes back unstripped, +# not empty. +_BARE_SCHEME_WORDS = frozenset({"bearer", "basic", "token"}) + +# Mirrors user_supplied._TEMPLATE_MARKER: a Dify template reference names a +# variable, not a literal, so it is never "invented" and must never be +# re-grounded -- checked BEFORE the placeholder-shape check below, since a +# grounded variable's own NAME (e.g. "your_api_key") can coincidentally match +# CREDENTIAL_PLACEHOLDER_RE's "YOUR...KEY" pattern. +_TEMPLATE_MARKER = "{{#" + + def _is_invented_credential(value: str, trusted_text: str) -> bool: """Mirrors ``_ground_placeholder_endpoints``'s OR-of-two-checks shape, but for a credential value (a header/param 'Value' half, or an - ``authorization.config.api_key``): an unmistakable placeholder - (``CREDENTIAL_PLACEHOLDER_RE``, after stripping a leading ``Bearer - ``/``Basic ``/``Token `` scheme) grounds even in placeholder-only mode - (``trusted_text == ""``, mirroring how a shaped-like-invented URL grounds - with no goal text at all); otherwise, with ``trusted_text`` non-empty, a - value that never appears verbatim in it also grounds (S5b-style - plausible-looking secret the model invented). An empty value has nothing - to ground. + ``authorization.config.api_key``): + + 1. Already a Dify template reference (contains ``{{#``) -- a real + reference, never re-grounded, checked first (see ``_TEMPLATE_MARKER``). + 2. Empty, or nothing beyond a bare ``Bearer``/``Basic``/``Token`` scheme + word -- never a valid credential either way, so it grounds + unconditionally. + 3. An unmistakable placeholder (``CREDENTIAL_PLACEHOLDER_RE``) -- grounds + even in placeholder-only mode (``trusted_text == ""``, mirroring how a + shaped-like-invented URL grounds with no goal text at all). + 4. Otherwise, with ``trusted_text`` non-empty, a value that never appears + verbatim in it also grounds (S5b-style plausible-looking secret the + model invented). """ stripped_value = value.strip() remainder = user_supplied.strip_auth_scheme(stripped_value) - if not remainder: + if _TEMPLATE_MARKER in remainder: return False + if not remainder or remainder.lower() in _BARE_SCHEME_WORDS: + return True if user_supplied.CREDENTIAL_PLACEHOLDER_RE.search(remainder): return True return bool(trusted_text) and not user_supplied.is_user_supplied_secret(stripped_value, trusted_text) @@ -856,26 +878,34 @@ def _is_invented_credential(value: str, trusted_text: str) -> bool: def _replace_credential_value(value: str, template_ref: str) -> str: """Rebuild a header/param 'Value' half with its secret swapped for - ``template_ref``, preserving surrounding whitespace and a leading - ``Bearer ``/``Basic ``/``Token `` auth-scheme prefix exactly as written - (e.g. ``" Bearer YOUR_API_KEY"`` -> ``" Bearer {{#s.h_api_key#}}"``).""" - lstripped = value.lstrip(" \t") + ``template_ref``, preserving surrounding whitespace (including a ``\\r`` + left by a ``\\r\\n`` line ending) and a leading ``Bearer ``/``Basic ``/ + ``Token `` auth-scheme prefix exactly as written (e.g. ``" Bearer + YOUR_API_KEY"`` -> ``" Bearer {{#s.h_api_key#}}"``). A BARE scheme word + with nothing after it (``"Bearer"``) keeps the word, adding the + separating space the grounded value now needs.""" + lstripped = value.lstrip(" \t\r") leading_ws = value[: len(value) - len(lstripped)] - core = lstripped.rstrip(" \t") + core = lstripped.rstrip(" \t\r") trailing_ws = lstripped[len(core) :] remainder = user_supplied.strip_auth_scheme(core) - scheme_prefix = core[: len(core) - len(remainder)] + if remainder == core and core.lower() in _BARE_SCHEME_WORDS: + scheme_prefix = f"{core} " + else: + scheme_prefix = core[: len(core) - len(remainder)] return f"{leading_ws}{scheme_prefix}{template_ref}{trailing_ws}" -def _ground_credential_text(text: str, template_ref: str, *, trusted_text: str) -> tuple[str, bool]: +def _ground_credential_text(text: str, template_ref: str, *, trusted_text: str, key_is_credential) -> tuple[str, bool]: """Ground each ``Key: Value`` line of an http-request ``headers``/ - ``params`` text block whose key names a credential - (``user_supplied.is_credential_key``, decided by the key's LAST segment) - and whose value the user didn't supply (``_is_invented_credential``), - replacing only the secret part of that line with ``template_ref``. Every - other line -- including a credential line the user DID supply -- passes - through byte-identical. Returns ``(new_text, changed)``. + ``params`` text block whose key names a credential per + ``key_is_credential`` (``user_supplied.is_credential_key`` for headers; + ``user_supplied.is_credential_param_key`` for params, which also treats a + bare ``key`` as one) and whose value the user didn't supply + (``_is_invented_credential``), replacing only the secret part of that + line with ``template_ref``. Every other line -- including a credential + line the user DID supply -- passes through byte-identical. Returns + ``(new_text, changed)``. """ if not text: return text, False @@ -887,7 +917,7 @@ def _ground_credential_text(text: str, template_ref: str, *, trusted_text: str) new_lines.append(line) continue key, _sep, value = line.partition(":") - if not user_supplied.is_credential_key(key.strip()) or not _is_invented_credential(value, trusted_text): + if not key_is_credential(key.strip()) or not _is_invented_credential(value, trusted_text): new_lines.append(line) continue new_lines.append(f"{key}:{_replace_credential_value(value, template_ref)}") @@ -936,14 +966,20 @@ def _ground_placeholder_credentials(intents: list[MutationIntent], *, trusted_te changed = False new_headers, headers_changed = _ground_credential_text( - str(node_config.get("headers") or ""), template_ref, trusted_text=trusted_text + str(node_config.get("headers") or ""), + template_ref, + trusted_text=trusted_text, + key_is_credential=user_supplied.is_credential_key, ) if headers_changed: node_config["headers"] = new_headers changed = True new_params, params_changed = _ground_credential_text( - str(node_config.get("params") or ""), template_ref, trusted_text=trusted_text + str(node_config.get("params") or ""), + template_ref, + trusted_text=trusted_text, + key_is_credential=user_supplied.is_credential_param_key, ) if params_changed: node_config["params"] = new_params diff --git a/api/services/dify_builder/agent/user_supplied.py b/api/services/dify_builder/agent/user_supplied.py index ff9ef47298f..1b8ebb4c634 100644 --- a/api/services/dify_builder/agent/user_supplied.py +++ b/api/services/dify_builder/agent/user_supplied.py @@ -19,6 +19,19 @@ from collections.abc import Mapping from typing import Any from urllib.parse import urlsplit +from core.dify_builder.credentials import is_credential_key, is_credential_param_key + +__all__ = [ + "CREDENTIAL_PLACEHOLDER_RE", + "is_credential_key", + "is_credential_param_key", + "is_user_supplied_secret", + "is_user_supplied_url", + "strip_auth_scheme", + "trusted_text_for", + "url_hosts", +] + # 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 @@ -51,21 +64,6 @@ 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 @@ -125,34 +123,6 @@ 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/core/dify_builder/test_handlers_fix.py b/api/tests/unit_tests/core/dify_builder/test_handlers_fix.py index 776391e7267..d39341a4085 100644 --- a/api/tests/unit_tests/core/dify_builder/test_handlers_fix.py +++ b/api/tests/unit_tests/core/dify_builder/test_handlers_fix.py @@ -786,17 +786,61 @@ def test_endpoint_variable_names_finds_a_start_variable_referenced_as_the_whole_ assert endpoint_variable_names(graph) == {"h_url"} -def test_endpoint_variable_names_finds_a_start_variable_embedded_in_a_larger_url(): +def test_endpoint_variable_names_ignores_a_url_template_not_in_host_position(): + """Review fix round 1 (item 1): a start-variable template appearing LATER + in the url (path/query), not at its very start, is an ordinary data + input -- e.g. ``https://wttr.in/{{#s.city#}}`` -- not an endpoint the + user must supply, so mocking ``city`` must not be blocked. Only a + template occupying the HOST position (the url, stripped, STARTS with it) + counts; this intentionally narrows the old (Plan 3) behaviour, which + collected any embedded reference.""" from core.dify_builder.handlers_fix import endpoint_variable_names graph = { "nodes": [ - {"id": "s", "data": {"type": "start", "variables": [{"variable": "path", "type": "text-input"}]}}, - {"id": "h", "data": {"type": "http-request", "url": "https://x.com/{{#s.path#}}"}}, + {"id": "s", "data": {"type": "start", "variables": [{"variable": "city", "type": "text-input"}]}}, + {"id": "h", "data": {"type": "http-request", "url": "https://wttr.in/{{#s.city#}}"}}, ], "edges": [], } - assert endpoint_variable_names(graph) == {"path"} + assert endpoint_variable_names(graph) == set() + + +def test_endpoint_variable_names_ignores_non_credential_header_and_param_keys(): + """Review fix round 1 (item 1): an ordinary data input read via a header + or param -- not a secret -- must stay mockable. Before this fix, ANY + start-variable template anywhere in headers/params was collected, so a + search query or an Accept-Language header was silently dropped from + every prefill/mock, and "use mock data" failed for every API-calling + app. Only a CREDENTIAL-keyed line counts now.""" + from core.dify_builder.handlers_fix import endpoint_variable_names + + graph = { + "nodes": [ + { + "id": "s", + "data": { + "type": "start", + "variables": [ + {"variable": "query", "type": "text-input"}, + {"variable": "lang", "type": "text-input"}, + ], + }, + }, + { + "id": "h", + "data": { + "type": "http-request", + "url": "https://api.acme.com/v1/search", + "headers": "Accept-Language: {{#s.lang#}}", + "params": "q: {{#s.query#}}", + "authorization": {"type": "no-auth", "config": None}, + }, + }, + ], + "edges": [], + } + assert endpoint_variable_names(graph) == set() def test_endpoint_variable_names_ignores_a_reference_to_a_non_start_node(): diff --git a/api/tests/unit_tests/services/dify_builder/test_build_nodes.py b/api/tests/unit_tests/services/dify_builder/test_build_nodes.py index 1bd09016132..4f39bea6304 100644 --- a/api/tests/unit_tests/services/dify_builder/test_build_nodes.py +++ b/api/tests/unit_tests/services/dify_builder/test_build_nodes.py @@ -789,3 +789,192 @@ def test_build_nodes_grounds_an_invented_api_key_param(): assert http.args["config"]["params"] == "api_key: {{#s.h_api_key#}}" var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key") assert var["required"] is True + + +# ---- Task 3 review fix round 1 ------------------------------------------ + + +def test_build_nodes_grounds_a_bare_key_param(): + """Review item 2: a raw ``?key=...`` param is a common way an API names + its key; ``is_credential_key("key")`` alone is False (its last-segment + rule needs a qualifying prefix), so params get a params-only rule that + also treats a bare, whole ``key`` as a credential.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + params="key: YOUR_API_KEY", + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["params"] == "key: {{#s.h_api_key#}}" + var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key") + assert var["required"] is True + + +def test_build_nodes_leaves_cache_key_and_key_points_params_alone(): + """The params-only bare-``key`` rule must not fire on a key that merely + ENDS in "key" as part of a longer word/segment.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + params="cache_key: YOUR_API_KEY\nkey_points: YOUR_API_KEY", + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["params"] == "cache_key: YOUR_API_KEY\nkey_points: YOUR_API_KEY" + assert start.args["config"]["variables"] == [] + + +def test_build_nodes_grounds_an_empty_api_key_authorization_config(): + """Review item 3: an empty ``config.api_key`` is never a valid credential + either -- it must ground exactly like a placeholder one, not pass through + and fail every run.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + authorization={"type": "api-key", "config": {"type": "bearer", "api_key": ""}}, + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["authorization"]["config"]["api_key"] == "{{#s.h_api_key#}}" + var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key") + assert var["required"] is True + + +def test_build_nodes_grounds_an_empty_credential_header_line(): + """Review item 3, header/param side: ``Authorization:`` with nothing + after the colon is an empty credential, not a real one -- it must + ground too.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + headers="Authorization:", + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["headers"] == "Authorization:{{#s.h_api_key#}}" + var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key") + assert var["required"] is True + + +def test_build_nodes_leaves_an_already_templated_credential_header_alone(): + """Review item 4: a header that already reads a start variable (e.g. + from an earlier grounding pass, or one the model wrote itself) must + never be re-grounded -- even though the variable NAME can itself look + placeholder-shaped (``your_api_key`` matches CREDENTIAL_PLACEHOLDER_RE's + ``YOUR...KEY`` pattern). The template check must run BEFORE the + placeholder-shape check.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + headers="Authorization: Bearer {{#s.your_api_key#}}", + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["headers"] == "Authorization: Bearer {{#s.your_api_key#}}" + assert start.args["config"]["variables"] == [] + + +def test_build_nodes_grounds_a_bare_scheme_word_keeping_it(): + """Review minor (b): a value that is EXACTLY a scheme word (nothing + after it -- ``strip_auth_scheme`` only strips a scheme FOLLOWED by + whitespace, so a bare word isn't recognised as empty by that helper + alone) is grounded while keeping the scheme word.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + headers="Authorization: Bearer", + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["headers"] == "Authorization: Bearer {{#s.h_api_key#}}" + var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key") + assert var["required"] is True + + +def test_build_nodes_grounds_a_credential_header_preserving_crlf_line_endings(): + """Review minor (a): ``_replace_credential_value`` used to strip only + `` \\t``, so a ``\\r\\n``-terminated line's trailing ``\\r`` was treated + as part of the secret and silently dropped instead of preserved as a + line-ending character.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + headers="Content-Type: application/json\r\nAuthorization: Bearer YOUR_API_KEY\r\n", + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + assert http.args["config"]["headers"] == ( + "Content-Type: application/json\r\nAuthorization: Bearer {{#s.h_api_key#}}\r\n" + ) + + +def test_build_nodes_grounds_an_invented_secret_absent_from_trusted_text(): + """Review minor (c), test 1: in trusted mode, a non-placeholder-shaped + but still invented secret (absent from what the user actually typed) + must ground -- not just an obviously placeholder-shaped one.""" + goal = "Call https://api.acme.com/v1/render to render a slide deck." + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + headers="Authorization: Bearer sk-invented", + ), + trusted_text=goal, + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["headers"] == "Authorization: Bearer {{#s.h_api_key#}}" + var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key") + assert var["required"] is True + + +def test_build_nodes_leaves_a_non_placeholder_secret_alone_in_placeholder_only_mode(): + """Review minor (c), test 2: with no trusted_text at all, only an + unmistakably placeholder-shaped credential grounds -- a real-looking + secret is left alone rather than treated as invented just because there + is no goal text to check it against (mirrors the URL grounding's own + placeholder-only-mode behaviour).""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + headers="Authorization: Bearer sk-live-abc123", + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["headers"] == "Authorization: Bearer sk-live-abc123" + assert start.args["config"]["variables"] == [] + + +def test_build_nodes_grounds_both_an_invented_url_and_an_invented_header_on_the_same_node(): + """Review minor (c), test 3: a node with both an invented URL and an + invented credential header gets BOTH a ``_url`` and a + ``_api_key`` start variable -- the two grounding passes share the + start-variable insertion write without clobbering each other.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.example.com/ppt/generate", + headers="Authorization: Bearer YOUR_API_KEY", + ) + ) + + http = next(i for i in intents if i.args.get("node_type") == "http-request") + start = next(i for i in intents if i.args.get("node_type") == "start") + assert http.args["config"]["url"] == "{{#s.h_url#}}" + assert http.args["config"]["headers"] == "Authorization: Bearer {{#s.h_api_key#}}" + assert {v["variable"] for v in start.args["config"]["variables"]} == {"h_url", "h_api_key"} 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 7fb34ffc9e1..ddde57b488e 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 @@ -114,6 +114,35 @@ def test_is_credential_key_false_for_empty_key(): assert us.is_credential_key("") is False +# --- Task 3 review fix round 1: is_credential_key moved into core, plus a +# params-only sibling that also treats a bare "key" as a credential -------- + + +def test_is_credential_key_still_importable_after_moving_into_core(): + """The pure logic now lives in core.dify_builder.credentials (core must + not import services); user_supplied re-exports it so this module's own + API is unchanged.""" + from core.dify_builder.credentials import is_credential_key as core_is_credential_key + + assert us.is_credential_key is core_is_credential_key + + +def test_is_credential_param_key_also_treats_a_bare_key_as_a_credential(): + assert us.is_credential_param_key("key") + assert us.is_credential_param_key("KEY") + assert us.is_credential_param_key(" key ") + + +def test_is_credential_param_key_still_excludes_lookalikes(): + for key in ("cache_key", "key_points", "password_policy", "token_limit"): + assert not us.is_credential_param_key(key), key + + +def test_is_credential_param_key_still_true_for_ordinary_credential_keys(): + for key in ("api_key", "apiKey", "x-api-key", "access_token", "Authorization"): + assert us.is_credential_param_key(key), key + + # --- Fix round 1: url_hosts stops at the first non-URL character ----------