fix: json body corruption when string variables contain special characters - #32317
fix: json body corruption when string variables contain special characters#32317Mr-Neutr0n wants to merge 1 commit into
Conversation
…ters the http request node was doing raw string substitution for template variables in json bodies, then relying on repair_json to fix the resulting broken json. this corrupted data when string values contained quotes, backslashes, newlines, or other json-special characters. now string variable values are properly json-encoded before substitution using json.dumps, which correctly escapes special characters. also handles the case where template authors pre-wrap variables in quotes. fixes langgenius#31927
dosubot
Bot
added
the
size:L
Summary of ChangesHello @Mr-Neutr0n, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the HTTP Request node where JSON bodies would become corrupted if string variables contained special characters. The previous raw string substitution method failed to properly escape these characters, leading to invalid JSON. The solution involves a new, JSON-aware templating function that correctly encodes string variable values, ensuring the integrity of the generated JSON body and preventing data corruption. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to resolve JSON body corruption when HTTP request nodes use string variables with special characters, introducing _convert_template_for_json for proper escaping and repair_json for backward compatibility, along with comprehensive regression tests. However, a critical vulnerability exists: the logic for detecting quoted variables in _convert_template_for_json is flawed, potentially leading to invalid JSON. Furthermore, error handling for JSON parsing failures exposes sensitive variables in exception messages, posing an information exposure risk. Additionally, consider refactoring the new method for improved readability.
| if preceding.rstrip().endswith('"') and following.lstrip().startswith('"'): | ||
| # Already quoted — just escape special chars without adding quotes | ||
| escaped = json.dumps(variable.value, ensure_ascii=False) | ||
| # Strip the outer quotes that json.dumps adds | ||
| result.append(escaped[1:-1]) | ||
| else: | ||
| # Not quoted — json.dumps adds quotes and escapes | ||
| result.append(json.dumps(variable.value, ensure_ascii=False)) |
There was a problem hiding this comment.
The logic for detecting "already quoted" variables is flawed. It only works correctly if the variable is the sole content of the quotes in the template. If a variable is part of a larger string (e.g., ""prefix-{{#var#}}""), preceding.rstrip().endswith('"') will be false, and json.dumps() will be called on the variable value, adding extra quotes (e.g., ""prefix-"val"""). This results in invalid JSON, which breaks functionality and can be used to trigger the RequestBodyError mentioned above, potentially leaking other sensitive variables in the JSON body.
| repaired = repair_json(json_string) | ||
| json_object = json.loads(repaired, strict=False) | ||
| except json.JSONDecodeError as e: | ||
| raise RequestBodyError(f"Failed to parse JSON: {json_string}") from e |
There was a problem hiding this comment.
The RequestBodyError exception includes the full json_string in its message. This string contains resolved variable values, which may include sensitive information such as API keys, secrets, or PII. Since this error message is often returned to the user or logged, it can lead to unauthorized disclosure of sensitive data. It is recommended to avoid including the full json_string in the error message and instead provide only the specific parsing error details.
| raise RequestBodyError(f"Failed to parse JSON: {json_string}") from e | |
| raise RequestBodyError(f"Failed to parse JSON: {e}") from e |
|
Why not just remove this JSON "repairing" and modification? It's non-obvious behaviour (hidden changes that the user cannot know about or turn off) and definitely an anti-pattern. If a user creates JSON, they want to send it to the endpoint as-is. If the JSON is invalid, it's the user's problem and the user fixes it. Period. Or at least there should be an option to disable this behaviour. |
|
@MezentsevIlya you're right in direction, and I owe you a proper answer — sorry it took this long. But I think the disagreement dissolves once we're precise about who produces the invalid JSON, because it isn't the user. The user's JSON is valid. Dify breaks it.The template from #31927 is well-formed: {"model": "pro", "messages": [{"role": "user", "content": {{#node.result#}}}]}The node then pastes the raw variable value into a JSON string position with no encoding. Reproduced against the real code path: So And note what "repairing" actually did: not a hidden modification, a silent total data loss. The request went out with Which makes your position and this patch the same positionEscaping at substitution time means well-formed templates never produce invalid JSON, so I kept it as a fallback only for templates that were malformed before substitution, since some existing workflows likely lean on that and dropping it is a breaking change. If you'd rather it be removed outright or put behind a switch, I agree that's the better end state, and it's a clean follow-up once the cause is fixed. I'd just rather not bundle a breaking change with a data-loss fix. This PR can't merge, though — the code movedWhile this sat, The bug is verbatim intact in graphon 0.7.0 ( Closing this one in favour of that. Thanks for pushing on the design — the rewrite is better for it. |
2 participants
When the HTTP Request node builds a JSON body from a template like
{"content": {{#node.result#}}}, it was doing raw string substitution viaconvert_template().textand then runningrepair_jsonon the result. If the variable value contains quotes, backslashes, newlines, or pipe characters (common in markdown), the substitution breaks the JSON structure andrepair_jsoncorrupts the data rather than fixing it.The root cause is that string values weren't being JSON-escaped before substitution. For example, a value like
hello "world"would produce{"content": hello "world"}— broken JSON thatrepair_jsoncan't reliably reconstruct.Fixed by adding
_convert_template_for_json()which usesjson.dumps()to properly encode string variable values before they land in the JSON structure. Numbers, objects, and arrays continue to use their existing.textrepresentation which already produces valid JSON tokens. Also handles the edge case where template authors already wrap variables in quotes (e.g."{{#var#}}").Added two regression tests covering strings with quotes/backslashes/newlines and markdown table content.
Closes #31927