diff --git a/api/core/dify_builder/handlers_fix.py b/api/core/dify_builder/handlers_fix.py index 5c943a00ca0..0e1518976dd 100644 --- a/api/core/dify_builder/handlers_fix.py +++ b/api/core/dify_builder/handlers_fix.py @@ -466,14 +466,16 @@ _ENDPOINT_TEMPLATE_RE = re.compile(r"\{\{#([^.#{}]+)\.([^.#{}]+)#\}\}") def endpoint_variable_names(graph: Graph) -> set[str]: - """Names of the start variables an http-request node reads its URL from. + """Names of the start variables an http-request node reads its URL, + headers, params, or API-key authorization from. - Nothing can invent a real endpoint any more than it can invent an upload - (ESQ1-302 / build.py's ``_ground_placeholder_endpoints``), 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 waiting on the user. + 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. """ start_id = "" for node in graph.get("nodes", []): @@ -484,14 +486,23 @@ 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 - url = str(data.get("url") or "") - for node_id, var_name in _ENDPOINT_TEMPLATE_RE.findall(url): - if node_id == start_id: - names.add(var_name) + _collect(str(data.get("url") or "")) + _collect(str(data.get("headers") or "")) + _collect(str(data.get("params") or "")) + 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 "")) return names diff --git a/api/services/dify_builder/agent/build.py b/api/services/dify_builder/agent/build.py index 1273d3e0a82..c1242297831 100644 --- a/api/services/dify_builder/agent/build.py +++ b/api/services/dify_builder/agent/build.py @@ -664,6 +664,10 @@ def build_nodes( _ground(intents, grounding_mc, tenant_id, plan_items) for node_id in _ground_placeholder_endpoints(intents, trusted_text=trusted_text): logger.info("Dify Builder: http-request %s had a placeholder URL; now read from a start variable", node_id) + for node_id in _ground_placeholder_credentials(intents, trusted_text=trusted_text): + logger.info( + "Dify Builder: http-request %s had a placeholder credential; now read from a start variable", node_id + ) applicable, rejected = graph_ops.filter_applicable({"nodes": [], "edges": []}, intents, _ALLOWED_NODE_TYPES) if not applicable: reason = rejected[0][1] if rejected else "no applicable node intents" @@ -761,6 +765,24 @@ def _is_placeholder_endpoint(url: str) -> bool: return _PLACEHOLDER_URL_RE.search(text) is not None or _is_localhost_host(text) +def _add_required_start_variable(variables: list[dict], var_name: str, label: str) -> None: + """Append a REQUIRED text-input start variable (max 2048 chars) named + ``var_name`` to ``variables`` if not already present -- the write shared + by every grounding pass that turns an invented literal (URL, credential) + into a start-node input, so it is not duplicated per pass.""" + if any(isinstance(v, dict) and v.get("variable") == var_name for v in variables): + return + variables.append( + { + "variable": var_name, + "label": label, + "type": "text-input", + "required": True, + "max_length": 2048, + } + ) + + def _ground_placeholder_endpoints(intents: list[MutationIntent], *, trusted_text: str = "") -> list[str]: """Replace every http-request URL the user didn't supply with a REQUIRED start-node variable and a template reference to it. @@ -801,16 +823,7 @@ def _ground_placeholder_endpoints(intents: list[MutationIntent], *, trusted_text continue node_id = str(intent.args.get("node_id") or "") var_name = f"{node_id}_url" - if not any(v.get("variable") == var_name for v in variables if isinstance(v, dict)): - variables.append( - { - "variable": var_name, - "label": f"{node_config.get('title') or node_id} URL", - "type": "text-input", - "required": True, - "max_length": 2048, - } - ) + _add_required_start_variable(variables, var_name, f"{node_config.get('title') or node_id} URL") node_config["url"] = f"{{{{#{start_id}.{var_name}#}}}}" intent.args["config"] = node_config grounded.append(node_id) @@ -820,6 +833,146 @@ def _ground_placeholder_endpoints(intents: list[MutationIntent], *, trusted_text return grounded +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. + """ + stripped_value = value.strip() + remainder = user_supplied.strip_auth_scheme(stripped_value) + if not remainder: + return False + 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) + + +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") + leading_ws = value[: len(value) - len(lstripped)] + core = lstripped.rstrip(" \t") + trailing_ws = lstripped[len(core) :] + remainder = user_supplied.strip_auth_scheme(core) + 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]: + """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)``. + """ + if not text: + return text, False + lines = text.split("\n") + changed = False + new_lines: list[str] = [] + for line in lines: + if ":" not in line: + 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): + new_lines.append(line) + continue + new_lines.append(f"{key}:{_replace_credential_value(value, template_ref)}") + changed = True + return "\n".join(new_lines), changed + + +def _ground_placeholder_credentials(intents: list[MutationIntent], *, trusted_text: str = "") -> list[str]: + """Replace every http-request credential literal the user didn't supply + -- a header/param whose key names a credential, or an ``api-key`` + authorization's ``config.api_key`` -- with a REQUIRED start-node variable + and a template reference to it. Sibling to ``_ground_placeholder_endpoints``, + called right after it in ``build_nodes`` and sharing its start-variable + insertion write (``_add_required_start_variable``); reads the start + node's config fresh, so a variable that pass already added (e.g. + ``_url``) is seen and not clobbered. + + S5b/F2: requirements analysis invented an ``Authorization: Bearer + YOUR_API_KEY`` header for a goal that named no credential. Turning it + into an input makes the test-data gate ask the user for the real key -- + or, when mocked, drop it (``without_endpoint_values`` -> now + ``endpoint_variable_names``, extended to also scan headers/params/ + authorization) so the run fails on a missing required input instead of + an invented secret the target API rejects. + + A single ``_api_key`` variable covers every credential grounded on + that node, however many lines/fields triggered it. Returns the ids of the + http-request nodes it re-pointed. + """ + start = next((i for i in intents if i.op == "create_node" and i.args.get("node_type") == "start"), None) + if start is None: + return [] + start_id = str(start.args.get("node_id") or "") + if not start_id: + return [] + config = dict(start.args.get("config") or {}) + variables = list(config.get("variables") or []) + grounded: list[str] = [] + for intent in intents: + if intent.op != "create_node" or intent.args.get("node_type") != "http-request": + continue + node_config = dict(intent.args.get("config") or {}) + node_id = str(intent.args.get("node_id") or "") + var_name = f"{node_id}_api_key" + template_ref = f"{{{{#{start_id}.{var_name}#}}}}" + changed = False + + new_headers, headers_changed = _ground_credential_text( + str(node_config.get("headers") or ""), template_ref, trusted_text=trusted_text + ) + 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 + ) + if params_changed: + node_config["params"] = new_params + changed = True + + authorization = node_config.get("authorization") + auth_config = authorization.get("config") if isinstance(authorization, dict) else None + if ( + isinstance(authorization, dict) + and authorization.get("type") == "api-key" + and isinstance(auth_config, dict) + and _is_invented_credential(str(auth_config.get("api_key") or ""), trusted_text) + ): + new_auth_config = dict(auth_config) + new_auth_config["api_key"] = template_ref + node_config["authorization"] = {**authorization, "config": new_auth_config} + changed = True + + if not changed: + continue + _add_required_start_variable(variables, var_name, f"{node_config.get('title') or node_id} API key") + intent.args["config"] = node_config + grounded.append(node_id) + if grounded: + config["variables"] = variables + start.args["config"] = config + return grounded + + def _ground(intents: list[MutationIntent], mc: ModelConfig, tenant_id: str, plan_items: list[str]) -> None: mode = mc.mode.value if hasattr(mc.mode, "value") else str(mc.mode) datasets = resources.list_tenant_resources(tenant_id).datasets diff --git a/api/tests/unit_tests/core/dify_builder/test_handlers_build.py b/api/tests/unit_tests/core/dify_builder/test_handlers_build.py index 574f7d47769..399ce03b562 100644 --- a/api/tests/unit_tests/core/dify_builder/test_handlers_build.py +++ b/api/tests/unit_tests/core/dify_builder/test_handlers_build.py @@ -1639,6 +1639,56 @@ def test_run_test_prefill_omits_a_placeholder_endpoint_but_keeps_other_fields(): assert [f["type"] for f in form.payload["fields"]] == ["text-input", "text-input"] # both still asked for +def test_run_test_prefill_omits_a_placeholder_credential_but_keeps_other_fields(): + """The credential counterpart of the placeholder-endpoint prefill guard + (Task 3): a mocked API key would let the run launch with an invented + secret and fail deep inside the http-request node instead of at input + validation -- so, like an invented URL, it must stay empty while an + ordinary text field is still pre-filled.""" + from core.dify_builder.handlers_build import handle_execution + from tests.unit_tests.core.dify_builder.fakes import FakeBuildDifyPort + + env, _ = _new_env() + env.dify = FakeBuildDifyPort() + env.dify.graph = { + "nodes": [ + { + "id": "s", + "data": { + "type": "start", + "variables": [ + {"variable": "topic", "type": "text-input"}, + {"variable": "h_api_key", "type": "text-input", "required": True, "max_length": 2048}, + ], + }, + }, + { + "id": "h", + "data": { + "type": "http-request", + "title": "Call PPT API", + "url": "https://api.acme.com/v1/render", + "headers": "Authorization: Bearer {{#s.h_api_key#}}", + "params": "", + "authorization": {"type": "no-auth", "config": None}, + }, + }, + ], + "edges": [], + } + env.agent.generate_mock_inputs = lambda _schema, _prior: { + "topic": "quarterly report", + "h_api_key": "sk-mock-invented", + } + s = _session(entry_mode=EntryMode.BUILD, current_state=PcState.BUILD_EXECUTION) + fc = DifyBuilderContext(test_input_ref="") + result = handle_execution(env, Turn(actor=_actor(), action=Action(kind="run_test")), s, fc) + assert result.next == PcState.BUILD_AWAIT_TESTDATA + form = next(i for i in result.items if i.kind == "form") + assert form.payload["values"] == {"topic": "quarterly report"} # h_api_key dropped, topic kept + assert [f["type"] for f in form.payload["fields"]] == ["text-input", "text-input"] # both still asked for + + def test_run_test_still_asks_when_a_file_is_declared_among_other_variables(): """Mixed schema: text can be mocked, but the file variable cannot -- the gate must still fire so a human can supply the upload.""" 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 9df06d7b65a..776391e7267 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 @@ -825,6 +825,75 @@ def test_endpoint_variable_names_empty_when_there_is_no_http_request_node(): assert endpoint_variable_names(graph) == set() +def test_endpoint_variable_names_collects_headers_too(): + """A node whose url and header both read the start node counts both; a + header reading another node's output is an ordinary wired value, not a + credential waiting on the user.""" + from core.dify_builder.handlers_fix import endpoint_variable_names + + graph = { + "nodes": [ + { + "id": "s", + "data": { + "type": "start", + "variables": [ + {"variable": "h_url", "type": "text-input"}, + {"variable": "h_api_key", "type": "text-input"}, + ], + }, + }, + {"id": "llm1", "data": {"type": "llm"}}, + { + "id": "h", + "data": { + "type": "http-request", + "url": "{{#s.h_url#}}", + "headers": "Authorization: Bearer {{#s.h_api_key#}}\nX-Trace: {{#llm1.text#}}", + "params": "", + "authorization": {"type": "no-auth", "config": None}, + }, + }, + ], + "edges": [], + } + assert endpoint_variable_names(graph) == {"h_url", "h_api_key"} + + +def test_endpoint_variable_names_also_collects_params_and_authorization_api_key(): + from core.dify_builder.handlers_fix import endpoint_variable_names + + graph = { + "nodes": [ + { + "id": "s", + "data": { + "type": "start", + "variables": [ + {"variable": "p_key", "type": "text-input"}, + {"variable": "auth_key", "type": "text-input"}, + ], + }, + }, + { + "id": "h", + "data": { + "type": "http-request", + "url": "https://api.acme.com/v1", + "headers": "", + "params": "api_key: {{#s.p_key#}}", + "authorization": { + "type": "api-key", + "config": {"type": "bearer", "api_key": "{{#s.auth_key#}}"}, + }, + }, + }, + ], + "edges": [], + } + assert endpoint_variable_names(graph) == {"p_key", "auth_key"} + + # ---- without_endpoint_values ------------------------------------------------- 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 9f97e47620a..1bd09016132 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 @@ -583,7 +583,15 @@ def test_build_nodes_grounds_a_placeholder_endpoint_into_a_required_start_variab assert "Call PPT API" in var["label"] -def _http_node(node_id: str, title: str, url: str) -> dict: +def _http_node( + node_id: str, + title: str, + url: str, + *, + headers: str = "", + params: str = "", + authorization: dict | None = None, +) -> dict: return { "id": node_id, "type": "custom", @@ -592,14 +600,35 @@ def _http_node(node_id: str, title: str, url: str) -> dict: "title": title, "method": "post", "url": url, - "authorization": {"type": "no-auth", "config": None}, - "headers": "", - "params": "", + "authorization": authorization or {"type": "no-auth", "config": None}, + "headers": headers, + "params": params, "body": _HTTP_BODY, }, } +def _gen_graph_with_http_full( + url: str, + *, + headers: str = "", + params: str = "", + authorization: dict | None = None, +) -> dict: + return { + "graph": { + "nodes": [ + {"id": "s", "type": "custom", "data": {"type": "start", "title": "Start", "variables": []}}, + _http_node("h", "Call PPT API", url, headers=headers, params=params, authorization=authorization), + {"id": "e", "type": "custom", "data": {"type": "end", "title": "End", "outputs": []}}, + ], + "edges": [{"id": "e1", "source": "s", "target": "h"}, {"id": "e2", "source": "h", "target": "e"}], + }, + "error": "", + "errors": [], + } + + def test_build_nodes_grounds_each_placeholder_endpoint_into_its_own_start_variable(): gen_graph = _gen_graph_with_http("https://api.example.com/ppt/generate") nodes = gen_graph["graph"]["nodes"] @@ -685,3 +714,78 @@ def test_is_placeholder_endpoint_recognises_the_usual_inventions(): assert not build._is_placeholder_endpoint( "https://abc123.localhost.run/webhook" ) # a real tunnel domain, not the localhost host + + +# ---- Task 3: invented credentials become an input --------------------------- + + +def test_build_nodes_grounds_an_invented_credential_header_into_a_required_start_variable(): + """S5b/F2: requirements analysis invented an ``Authorization: Bearer + YOUR_API_KEY`` header the goal never supplied. Grounding it into a start + variable means the test-data gate asks the user for the real key instead + of the workflow shipping a header the run can never satisfy. The + Content-Type line is not a credential and must pass through byte-identical.""" + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + headers="Content-Type: application/json\nAuthorization: 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"]["headers"] == ("Content-Type: application/json\nAuthorization: Bearer {{#s.h_api_key#}}") + var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_api_key") + assert var["type"] == "text-input" + assert var["required"] is True + assert var["max_length"] == 2048 + assert "Call PPT API" in var["label"] + + +def test_build_nodes_leaves_a_header_credential_alone_when_the_user_actually_supplied_it(): + """The mirror case: the goal literally names the secret, so it is not a + fabrication and must be left alone.""" + goal = "Call https://api.acme.com/v1/render with Authorization: Bearer sk-live-abc123." + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + headers="Authorization: Bearer sk-live-abc123", + ), + 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 sk-live-abc123" + assert start.args["config"]["variables"] == [] + + +def test_build_nodes_grounds_an_invented_api_key_authorization_config(): + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + authorization={"type": "api-key", "config": {"type": "bearer", "api_key": "YOUR_TOKEN"}}, + ) + ) + + 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#}}" + assert http.args["config"]["authorization"]["config"]["type"] == "bearer" # untouched + 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_invented_api_key_param(): + intents = _build_with( + _gen_graph_with_http_full( + "https://api.acme.com/v1/render", + params="api_key: xxxxxxxx", + ) + ) + + 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"] == "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