Skip to content

[Bug] TOML string outputs are not properly escaped #6000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eric-forte-elastic wants to merge 12 commits into
base: main
Choose a base branch
from
Open

[Bug] TOML string outputs are not properly escaped #6000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f4d204e
Fix quote support
eric-forte-elastic Apr 27, 2026
1450037
preserve_all_strings
eric-forte-elastic Apr 29, 2026
fefd114
Reduce complexity
eric-forte-elastic Apr 29, 2026
3aa971e
Merge branch 'main' into 5182-bug-toml-string-outputs-are-not-properl…
eric-forte-elastic Apr 29, 2026
f65dd24
patch bump
eric-forte-elastic Apr 29, 2026
0e034aa
Fix max string wrap
eric-forte-elastic May 4, 2026
9fa8d81
Merge branch 'main' into 5182-bug-toml-string-outputs-are-not-properl…
eric-forte-elastic May 4, 2026
c5daa10
patch bump
eric-forte-elastic May 4, 2026
bbda2eb
Merge branch '5182-bug-toml-string-outputs-are-not-properly-escaped' …
eric-forte-elastic May 4, 2026
e2d945a
Add noqa
eric-forte-elastic May 4, 2026
fb49fb8
Revert rule formatter changes, handled in a different PR
eric-forte-elastic May 4, 2026
5f08e5b
fix bump
eric-forte-elastic May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions detection_rules/etc/test_toml.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,5 +170,15 @@
}
]
}
},
{
"metadata": {
"creation_date": "2020/02/26",
"maturity": "development",
"updated_date": "2020/02/26"
},
"rule": {
"query": "file.path: \"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\" file.path: Hello\\:World"
}
}
]
9 changes: 6 additions & 3 deletions detection_rules/rule_formatter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,12 @@ def dump_str(self, v: str | NonformattedField) -> str:
raw = (multiline or (DQ in v and SQ not in v)) and TRIPLE_DQ not in v

if multiline:
if raw:
return "".join([TRIPLE_DQ, *initial_newline, *lines, TRIPLE_DQ])
return "\n".join([TRIPLE_SQ] + [json.dumps(line)[1:-1] for line in lines] + [TRIPLE_SQ])
# Triple-double-quoted basic strings allow literal newlines and literal ``"``
# (as long as ``"""`` doesn't appear, which is guarded above via ``TRIPLE_DQ not in v``),
# but backslashes must be escaped so that e.g. ``Hello\:World`` is serialized as
# ``Hello\\:World`` -- otherwise ``\:`` is an invalid TOML escape sequence (issue #5182).
escaped_lines = [line.replace("\\", "\\\\") for line in lines]
Copy link
Copy Markdown
Contributor

@wingiti wingiti May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break the fields "note", "setup", "description", "osquery" if they are multiline as they are somehow additionally escaped below in lines 253-274.

Same issue occurred in PR #6074

Query needs to be handled in another way or, from my perspective preferred, multiline should be escaped in general (like in this PR or #6074) but lines 253-274 needs to be removed. As I don't know what impact this might have I will wait for some further feedback for now.

Copy link
Copy Markdown
Contributor Author

@eric-forte-elastic eric-forte-elastic May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing lines 253-274 without performing a fairly broad refactor of our toml_write process would cause a number of issues in our current pipeline.

The fields you mentioned will always be additionally escaped when written to TOML to counteract the escape invocation that is currently present in toml_write. Having json.dumps creates an issue as you described in #6074; however, adding an additional escape does not necessarily break these other fields. Their values change, but it does not inherently cause them to not function correctly (as the json dumps does). This is only of particular importance to os_query, as it is a functional field rather than a descriptive one. However, given that in the current bug is that os_query is broken when it goes to multi-line (sufficient length), this change is intended as it can address the issue.

Copy link
Copy Markdown
Contributor

@wingiti wingiti May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the os_query stuff, haven't had such an example so far but you are correct this might be the only field with impact on functionality.

Nevertheless if you use json dumps or your approach above (currently I don't see a difference in things I tested) you will add more escape characters than needed and which will be removed.
e.g. if the input of note field (on system 1) is: "some \ example" you will end up with a note field (on system 2) of: "some \ example"

This distorts the result (even it has no impact on functionality) and will drive you nuts if you compare two rules on two systems.

Copy link
Copy Markdown
Contributor Author

@eric-forte-elastic eric-forte-elastic May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% agree, generally speaking I am going to close this PR and put together a refactor proposal which should resolve this issue in light of:
#6000 (comment)

return "".join([TRIPLE_DQ, *initial_newline, *escaped_lines, TRIPLE_DQ])
if raw:
return f"'{lines[0]:s}'"
# In the toml library there is a magic replace for \\\\x -> u00 that we wish to avoid until #4979 is resolved
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[project]
name = "detection_rules"
version = "1.6.33"
version = "1.6.34"
description = "Detection Rules is the home for rules used by Elastic Security. This repository is used for the development, maintenance, testing, validation, and release of rules for Elastic Security’s Detection Engine."
readme = "README.md"
requires-python = ">=3.12"
Expand Down
Loading