Skip to content

fix: replace _ws_ space encoding with escaped spaces in SIGMA query generation - #1804

Draft
thecodingshrimp wants to merge 1 commit into
opensearch-project:mainfrom
thecodingshrimp:fix/sigma-whitespace-query-encoding
Draft

fix: replace _ws_ space encoding with escaped spaces in SIGMA query generation#1804
thecodingshrimp wants to merge 1 commit into
opensearch-project:mainfrom
thecodingshrimp:fix/sigma-whitespace-query-encoding

Conversation

@thecodingshrimp

@thecodingshrimp thecodingshrimp commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

SIGMA detection rules whose field values contain spaces never match because spaces are encoded as literal _ws_ tokens that Lucene cannot decode in all query contexts. This fix replaces the _ws_ scheme with backslash-escaped spaces (\ ) emitted via the existing backend escape loop.

Two failure modes fixed:

  1. Wildcard path (contains/startswith/endswith): query_string wildcard terms bypass the field analyzer, so _ws_ is matched literally against ingest data that contains real spaces — detections never fire.
  2. Regex path: SigmaRegularExpression encoded spaces as _ws_, but Lucene regex syntax treats space as a literal character and does not apply the rule_ws_filter char_filter — detections never fire.

Additional correctness fix:

  1. SigmaWindowsDashModifier hard-coded a .replace("_ws_", " ") decode followed by .replace(" ", "_ws_") re-encode. This conflicted with the new escaping on the wildcard path and has been removed.

Changes:

  • OSQueryBackend.java: add " " (space) to addEscaped so the escape loop in SigmaString.convert() emits \ via backend config rather than hardcoded string replace.
  • SigmaString.java: remove hardcoded replace(" ", "_ws_") from convert() and toString() — escaping now flows through the backend-config escape loop.
  • SigmaRegularExpression.java: remove replace(" ", "_ws_") — Lucene regex treats space as a literal, no encoding needed.
  • SigmaWindowsDashModifier.java: remove _ws_ decode/re-encode round-trip — values are stored with literal spaces throughout.
  • detector-settings.json / DetectorMonitorConfig.java: rule_ws_filter and rule_analyzer are kept during this transition window; the _ws_ token no longer appears in generated queries so the filter is dormant but harmless. Retirement requires a re-index migration and is deferred to a follow-up.

Related Issues

Resolves #1024

Check List

  • New functionality includes testing.
    • Unit tests: QueryBackendTests — whitespace wildcard path (*This\ is\ an\ example*), whitespace quoted path, regex path, windash path; all passing (BUILD SUCCESSFUL).
    • Integration tests: DetectorRestApiIT.testDetectorWithWhitespaceRuleContainsPath and testDetectorWithWhitespaceRuleQuotedPath added (compile-verified; require a live OpenSearch cluster to run).
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions

github-actions Bot commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit c764d9b)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Uninitialized variable

bucketLevelQuery is declared and used inside .query(QueryBuilders.queryStringQuery(bucketLevelQuery)), but the diff shows the variable extracted from rule.getQueries().get(0).getValue() — verify the previous inline usage was fully replaced. If rule.getQueries() is empty this now throws IndexOutOfBoundsException before the null check on line 1023 can help. The null-check for _ws_ is only useful if the value can be null; consider whether the code path expects at least one query.

 
String bucketLevelQuery = rule.getQueries().get(0).getValue();
// Legacy '_ws_'-encoded query: runs against ingest index (no rule_ws_filter), matches nothing.
// Re-save the detector to recompile the rule with the corrected encoding (PR #1789).
if (bucketLevelQuery != null && bucketLevelQuery.contains("_ws_")) {
    log.warn("Rule '{}' contains legacy _ws_ encoding in its query string; re-save the detector to recompile the rule", rule.getId());
}

SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder()
        .seqNoAndPrimaryTerm(true)
        .version(true)
        // Build query string filter
        .query(QueryBuilders.queryStringQuery(bucketLevelQuery))
Dead code branch

In convertConditionFieldEqValStr, convertedExpr is used in the applyDeMorgans branch (line 346) before it is initialized on line 344; but more importantly the non-DeMorgans code path relies on convertedExpr being set. Looking at the flow: convertedExpr is assigned on line 344 with the non-DeMorgans template, then unconditionally overwritten on line 346 when DeMorgans applies. That is correct, but the diff also removes an earlier initialization that existed in the old code inside an if/else. Verify that no code path returns convertedExpr before it is assigned — otherwise a compile error or NPE occurs. Also confirm there is a proper return convertedExpr; after the block (not visible in diff).

 
String expr = "%s" + this.eqToken + " " + (containsWildcard? this.reQuote: this.strQuote) + "%s" + (containsWildcard? this.reQuote: this.strQuote);
String exprWithDeMorgansApplied = this.notToken + " " + "%s" + this.eqToken + " " + (containsWildcard? this.reQuote: this.strQuote) + "%s" + (containsWildcard? this.reQuote: this.strQuote);

// For wildcard values that spacedPhraseShape did not route (e.g. an interior '?' or extra '*'
// alongside a space, such as 'hello? world'), the converted value carries a raw space. A raw
// space in an unquoted query_string wildcard term splits it into multiple terms and breaks
// matching against the single keyword-analyzed token, so escape interior spaces here too.
String convertedValue = this.convertValueStr(value);
if (containsWildcard) {
    convertedValue = convertedValue.replace(" ", "\\ ");
}

String convertedExpr = String.format(Locale.getDefault(), expr, field, convertedValue);
if (applyDeMorgans) {
    convertedExpr = String.format(Locale.getDefault(), exprWithDeMorgansApplied, field, convertedValue);
}
return convertedExpr;
Unquoted wildcard term semantics

buildSpacedValueQuery emits e.g. field: *escaped\ text* as an unquoted query_string term. Because query_string splits on whitespace at the parser level before applying escapes in some versions, verify that backslash-escaped spaces survive parsing to the wildcard analyzer. If any target field is not configured as keyword / rule_analyzer, the escaped wildcard will still not substring-match analyzed tokens — the fix assumes all mapped fields use rule_analyzer, which the PR description acknowledges as a limitation for plain spaced values but not for the wildcard path.

 
private String buildSpacedValueQuery(String field, String text, boolean leadingWildcard, boolean trailingWildcard) {
    String escaped = escapeLiteralText(text).replace(" ", "\\ ");
    String lead  = leadingWildcard  ? this.wildcardMulti : "";
    String trail = trailingWildcard ? this.wildcardMulti : "";
    return field + this.eqToken + " " + lead + escaped + trail;
}

@github-actions

github-actions Bot commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to c764d9b

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Escape literal wildcard chars in spaced text

escapeLiteralText only escapes characters in addEscaped but does not escape the
wildcardMulti () and wildcardSingle (?) characters as literals. If a spaced value's
literal segment contains a bare or ? (as claimed in the Javadoc), they will pass
through unescaped and be interpreted as wildcards by query_string, contradicting the
documented intent.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [616-625]

 private String escapeLiteralText(String text) {
     String result = text.replace(escapeChar, escapeChar + escapeChar); // backslash first
     for (char c : addEscaped.toCharArray()) {
         String cs = String.valueOf(c);
         if (!cs.equals(escapeChar)) {
             result = result.replace(cs, escapeChar + cs);
         }
     }
+    if (wildcardMulti != null) result = result.replace(wildcardMulti, escapeChar + wildcardMulti);
+    if (wildcardSingle != null) result = result.replace(wildcardSingle, escapeChar + wildcardSingle);
     return result;
 }
Suggestion importance[1-10]: 6

__

Why: The Javadoc explicitly states bare */? should be escaped as literals, but the implementation only escapes addEscaped characters. However, in practice spacedPhraseShape only routes values whose literal segment has no wildcards, so this is more of a defensive/correctness gap than an active bug.

Low
Avoid double-escaping already-escaped spaces

Replacing every space with \ unconditionally will double-escape spaces that
convertValueStr already escaped (since addEscaped may include space), producing \\
in the output. Check for spaces not already preceded by a backslash before escaping,
e.g. using a regex like (?<!\\) to avoid corrupting already-escaped spaces.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [339-342]

 String convertedValue = this.convertValueStr(value);
 if (containsWildcard) {
-    convertedValue = convertedValue.replace(" ", "\\ ");
+    convertedValue = convertedValue.replaceAll("(?<!\\\\) ", "\\\\ ");
 }
Suggestion importance[1-10]: 3

__

Why: Space is not in the default addEscaped set for this backend, so convertValueStr does not pre-escape spaces, making the unconditional replace safe. The concern is largely hypothetical and the suggestion adds complexity without a demonstrated issue.

Low
Detect only unescaped spaces for quoting decision

The check !convertedValue.contains(" ") looks at the fully-converted string, which
may contain spaces that are already escaped (\ ). An escaped space is still safe
inside an unquoted wildcard term, so this will incorrectly force quoting for values
whose spaces have already been escaped. Test for a raw (unescaped) space instead.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [440-443]

 String convertedValue = this.convertValueStr(value);
 // A '*' next to whitespace can't act as a query_string wildcard on the keyword-analyzed single
 // token and a bare '*' is rejected; quote such values (literal '*'), keep *Wfuzz* unquoted.
-boolean useWildcardExpr = value.containsWildcard() && !convertedValue.contains(" ");
+boolean useWildcardExpr = value.containsWildcard()
+        && !convertedValue.matches(".*(?<!\\\\) .*");
Suggestion importance[1-10]: 3

__

Why: Similar to suggestion 1, space is not in the backend's addEscaped, so convertValueStr output won't contain pre-escaped spaces. The suggestion addresses a theoretical concern rather than an actual bug.

Low

Previous suggestions

Suggestions up to commit b26e56c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Escape quotes/backslashes in phrase value

The mixed-separator fallback emits the raw text inside double quotes without
escaping embedded quote or backslash characters. If the Sigma value contains a " or
</code>, the generated query_string will be malformed or unsafe. Escape backslashes and
double quotes before wrapping in quotes.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [577-623]

     private String buildSpacedValueQuery(String field, String text, boolean leadingWildcard, boolean trailingWildcard) {
         // Collect alphanumeric tokens into phrase groups; flush on any non-space separator.
-        // Lowercase tokens when adding (matching the rule_analyzer), but iterate over original text.
         List<List<String>> groups = new ArrayList<>();
         List<String> currentGroup = new ArrayList<>();
         int len = text.length();
         int i = 0;
         while (i < len) {
             int tokenStart = i;
             while (i < len && Character.isLetterOrDigit(text.charAt(i))) {
                 i++;
             }
             if (i > tokenStart) {
                 currentGroup.add(text.substring(tokenStart, i).toLowerCase(Locale.ROOT));
             }
 ...
-        // Mixed-separator / non-alphanumeric fallback: emit one quoted phrase (escaped spaces in a
-        // bare wildcard term are invalid query_string syntax; the keyword analyzer indexes the value
-        // as a single token, so a phrase matches it exactly). Trailing * gives startswith prefix.
         if (groups.size() != 1) {
             String phraseSuffix = (trailingWildcard && !leadingWildcard) ? "*" : "";
-            return field + this.eqToken + " \"" + text + "\"" + phraseSuffix;
+            String escaped = text.replace("\\", "\\\\").replace("\"", "\\\"");
+            return field + this.eqToken + " \"" + escaped + "\"" + phraseSuffix;
         }
Suggestion importance[1-10]: 7

__

Why: Valid concern: if the input text contains unescaped " or \, the emitted quoted phrase would produce a malformed query_string. This is a legitimate hardening for the mixed-separator fallback path.

Medium
Contains-with-spaces loses wildcard semantics

The contains branch passes leadingWildcard=true, trailingWildcard=true, but in
buildSpacedValueQuery the phrase suffix * is only appended when trailingWildcard &&
!leadingWildcard. So for contains-with-spaces the query is emitted as a bare phrase
with no wildcards, which is a stricter/exact match than contains semantics. Verify
semantics or add proper prefix/suffix wildcards for the contains shape.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [308-320]

         switch (spacedShape) {
             case "contains":
                 text = parts.get(1).getLeft();
+                // Ensure contains semantics are preserved (not just phrase-equality).
                 phraseExpr = buildSpacedValueQuery(field, text, true, true);
                 break;
-            case "startswith":
-                text = parts.get(0).getLeft();
-                phraseExpr = buildSpacedValueQuery(field, text, false, true);
-                break;
-            case "endswith":
-                text = parts.get(1).getLeft();
-                phraseExpr = buildSpacedValueQuery(field, text, true, false);
-                break;
Suggestion importance[1-10]: 4

__

Why: The observation is technically correct that no * is appended for contains, but the tests confirm this is the intended behavior since phrase matches against analyzed fields already match anywhere. Impact is limited.

Low
General
Handle other wildcard shapes with spaces

spacedPhraseShape only detects specific 2- or 3-element wildcard shapes with
plain-text spaces, but SigmaString values with escaped spaces or additional
wildcards inside the text may still contain spaces yet fall through to the default
wildcard path, producing terms like foo\ bar that fail at query_string parse time
(the original bug). Consider also handling the case where any wildcard SigmaString
value has whitespace in a text segment, or add a fallback that escapes/quotes spaces
on the default path.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [303-305]

         String spacedShape = spacedPhraseShape(value);
         if (spacedShape != null) {
             var parts = value.getsOpt();
+            // TODO: also handle multi-wildcard patterns with embedded spaces to avoid
+            // emitting bare wildcard terms containing whitespace.
Suggestion importance[1-10]: 5

__

Why: Raises a valid edge case that non-standard wildcard patterns with embedded spaces could still fall through to a broken wildcard path, but only proposes a TODO comment without a concrete fix.

Low
Suggestions up to commit 557b485
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix inconsistent De Morgan handling for spaced values

When spacedShape returns a non-null value, the spaced-phrase branch ignores
applyDeMorgans for computing the underlying comparison but only prepends notToken,
so it never emits the exists conjunction that the non-spaced De Morgan expression
includes elsewhere. This produces asymmetric semantics between spaced and non-spaced
negated values and can change match results for negated rules; ensure the De Morgan
branch generates equivalent structure (including any exists check) for spaced
values.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [303-328]

+String spacedShape = spacedPhraseShape(value);
+if (spacedShape != null) {
+    var parts = value.getsOpt();
+    String text;
+    String phraseExpr;
+    switch (spacedShape) {
+        case "contains":
+            text = parts.get(1).getLeft();
 
-
Suggestion importance[1-10]: 8

__

Why: The test testConvertNotComplicatedExpression shows negated non-spaced values produce NOT ... AND _exists_: field structure, but the spaced-phrase branch only prepends notToken without the _exists_ conjunction, creating asymmetric semantics for negated spaced values. This is a valid correctness concern.

Medium
Preserve original casing in phrase tokens

Lowercasing the token inside the phrase-clause path unconditionally will cause the
emitted query to mismatch when the target field is not analyzed with a lowercase
filter, and it also alters the user-facing casing for plain (non-wildcard) values
(as seen in testConvertValueStrWithWhitespaceQuoted where the expected output
preserves original case "This is an example"). Preserve the original casing here and
rely on the analyzer at query time for case normalization.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [589-591]

 if (i > tokenStart) {
-    currentGroup.add(text.substring(tokenStart, i).toLowerCase(Locale.ROOT));
+    currentGroup.add(text.substring(tokenStart, i));
 }
Suggestion importance[1-10]: 7

__

Why: The test testConvertValueStrWithWhitespaceQuoted expects "This is an example" with original casing preserved, but the phrase-clause path only triggers for wildcards (contains/startswith/endswith). Still, for the contains test with mixed casing input, lowercasing may unnecessarily alter user-facing output. Valid concern about consistency.

Medium
Suggestions up to commit 6fc2d9a
CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid broadening endswith to contains-phrase

For the endswith case (leadingWildcard=true, trailingWildcard=false) the phrase is
emitted with no wildcards, which turns an endswith into a phrase-anywhere match — a
semantic broadening that yields false positives (documents that contain the phrase
but do not end with it will match). Since query_string does not support a
leading-wildcard phrase prefix, either fall back to the escaped-wildcard branch for
endswith, or add explicit position anchoring; do not silently degrade endswith to
"contains phrase".

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [642-648]

+// endswith cannot be expressed as an analyzed phrase with leading wildcard;
+// fall back to the escaped wildcard form to preserve semantics.
+if (leadingWildcard && !trailingWildcard) {
+    String escaped = escapeWildcardText(text);
+    return field + this.eqToken + " " + this.wildcardMulti + escaped;
+}
 String clause = clauses.get(0);
 String phraseSuffix = (trailingWildcard && !leadingWildcard) ? "*" : "";
 if (clause.startsWith("\"")) {
     return field + this.eqToken + " " + clause + phraseSuffix;
 } else {
     return field + this.eqToken + " \"" + clause + "\"" + phraseSuffix;
 }
Suggestion importance[1-10]: 7

__

Why: Legitimate semantic concern: endswith with a leading wildcard is emitted as a phrase without position anchoring, which broadens matches to "contains phrase". The test testConvertValueStrEndsWithWhitespace confirms this behavior is intentional but it does represent a semantic broadening that could cause false positives.

Medium
Preserve non-space separators as distinct groups

The separator loop can produce an empty sep when i == len was already handled, but
if the token loop advanced zero characters at a non-alnum start and the separator
loop also captures the same char, whiteSpaceOnly is computed but there is no else
branch to flush the group when the separator IS whitespace-only followed by
end-of-string. More importantly, after processing a trailing whitespace-only
separator, currentGroup is never flushed inside the loop, which is fine, but a lone
leading non-alnum char (e.g. " /s ") yields tokens ["s"] while the leading / is
silently absorbed — losing semantic content and causing over-matching. Preserve
fidelity by falling back to the escaped-wildcard path when any non-space separator
is encountered anywhere.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [598-612]

 int sepStart = i;
 while (i < len && !Character.isLetterOrDigit(lower.charAt(i))) {
     i++;
 }
 String sep = lower.substring(sepStart, i);
 
 boolean whiteSpaceOnly = sep.trim().isEmpty() && !sep.isEmpty();
 
 if (!whiteSpaceOnly) {
-    // Non-space separator breaks the phrase group; flush it.
+    // Non-space separator anywhere → treat whole value as mixed-separator.
     if (!currentGroup.isEmpty()) {
         groups.add(new ArrayList<>(currentGroup));
         currentGroup.clear();
     }
+    groups.add(new ArrayList<>()); // force groups.size() > 1 path
 }
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a valid concern about semantic fidelity for mixed-separator values, and the test case ' /s ' in the existing test appears to lose the / character. However, the current code does flush the group on non-space separator, and the improvement's effect depends on precise semantics; it's a reasonable but not critical improvement.

Low
General
Honor wildcard flags in punctuation fallback

The all-punctuation fallback emits a phrase without applying the leading/trailing
wildcard flags, so contains/startswith/endswith semantics are lost for
punctuation-only values. Also, the emitted phrase for a contains on pure punctuation
will not match analyzed text that contains that punctuation, because the
keyword-based analyzer will not tokenize it identically. Honor the wildcard flags
here as well.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [627-631]

 if (groups.isEmpty()) {
-    // All-punctuation fallback: emit as a phrase (escaping backslashes/quotes).
-    String escapedLower = text.toLowerCase(Locale.ROOT).replace("\\", "\\\\").replace("\"", "\\\"");
-    return field + this.eqToken + " \"" + escapedLower + "\"";
+    // All-punctuation fallback: use escaped wildcard so contains/startswith/endswith are preserved.
+    String escaped = escapeWildcardText(text);
+    String leading  = leadingWildcard  ? this.wildcardMulti : "";
+    String trailing = trailingWildcard ? this.wildcardMulti : "";
+    return field + this.eqToken + " " + leading + escaped + trailing;
 }
Suggestion importance[1-10]: 6

__

Why: Valid point: the all-punctuation fallback emits a phrase without honoring the leading/trailing wildcard flags, which loses contains/startswith/endswith semantics for punctuation-only values. This is an edge case but a legitimate correctness issue.

Low
Replace stringly-typed shape with enum

Using string literals ("contains"/"startswith"/"endswith") as a control-flow
contract between two private methods is fragile — a typo in either side silently
falls through the default and throws IllegalStateException at runtime for what
should be compile-time-safe branching. Replace with an enum so the compiler enforces
exhaustiveness and the default branch becomes unreachable.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [303-323]

-String spacedShape = spacedPhraseShape(value);
+SpacedShape spacedShape = spacedPhraseShape(value);
 if (spacedShape != null) {
     var parts = value.getsOpt();
     String text;
     String phraseExpr;
     switch (spacedShape) {
-        case "contains":
+        case CONTAINS:
             text = parts.get(1).getLeft();
Suggestion importance[1-10]: 3

__

Why: Minor code quality improvement replacing string constants with an enum for compile-time safety. Reasonable refactor but not impactful to correctness or functionality.

Low
Suggestions up to commit 8af723d
CategorySuggestion                                                                                                                                    Impact
General
Preserve startswith semantics with multiple clauses

For a startswith with multiple clauses (trailingWildcard=true), the trailing
wildcard is silently dropped, which changes the semantics from "starts with X" to
"contains all tokens of X" — this can produce false positives. Consider applying the
wildcard to the last clause (e.g. phrase-prefix on the trailing phrase, or term on
the trailing term) to preserve prefix semantics.*

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [668-678]

 if (clauses.size() == 1) {
     String clause = clauses.get(0);
-    // A trailing wildcard only makes sense after a quoted phrase (phrase-prefix query).
-    // For a bare term we also support it, but it would be unusual.
     String suffix = trailingWildcard ? "*" : "";
     if (clause.startsWith("\"")) {
         return field + this.eqToken + " " + clause + suffix;
     } else {
         return field + this.eqToken + " \"" + clause + "\"" + suffix;
     }
 }
+// Note: for multi-clause startswith, trailing wildcard cannot be applied to a group;
+// consider appending '*' to the last clause to preserve prefix semantics.
Suggestion importance[1-10]: 6

__

Why: Correctly identifies a semantic issue where multi-clause startswith drops the trailing wildcard, potentially producing false positives; however, the improved_code doesn't actually fix the multi-clause case, only leaves a note.

Low
Escape more reserved chars inside phrase

When emitting a phrase inside query_string, only backslash and double-quote are
escaped, but query_string also treats other characters (e.g. /, +, -, :, (, ), {, },
[, ], ~, ^, !, ?, *, |, &) as reserved even inside a phrase in some cases (notably /
triggers regexp parsing). Consider escaping these to avoid parse failures on values
containing such characters.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [646-652]

 if (groups.isEmpty()) {
     // Fallback: emit the raw text as a phrase.
-    // Escape backslashes and double-quotes so the query_string remains syntactically valid.
-    String escapedLower = lower.replace("\\", "\\\\").replace("\"", "\\\"");
+    // Escape query_string reserved characters that can break phrase parsing.
+    String escapedLower = lower
+            .replace("\\", "\\\\")
+            .replace("\"", "\\\"")
+            .replace("/", "\\/");
     String suffix = trailingWildcard ? "*" : "";
     return field + this.eqToken + " \"" + escapedLower + "\"" + suffix;
 }
Suggestion importance[1-10]: 4

__

Why: Escaping additional reserved chars like / inside a phrase could avoid parse errors on some values, but this fallback path is rarely hit and the suggestion is speculative without evidence of failing cases.

Low
Extract text by type rather than fragile index

The endswith branch uses parts.get(1), but per spacedPhraseShape, an endswith value
has shape [WILDCARD_MULTI, Left], so the text is at index 1 — that is correct.
However, contains shape is [WILDCARD_MULTI, Left, WILDCARD_MULTI] so parts.get(1) is
right too. Double-check: the code is correct, but consider extracting text via a
helper to avoid index confusion and future breakage.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [307-322]

 switch (spacedShape) {
     case "contains":
-        text = parts.get(1).getLeft();
+    case "endswith":
+        text = parts.stream().filter(p -> p.isLeft()).findFirst().get().getLeft();
         phraseExpr = buildSpacedValueQuery(field, text, false);
         break;
     case "startswith":
-        text = parts.get(0).getLeft();
+        text = parts.stream().filter(p -> p.isLeft()).findFirst().get().getLeft();
         phraseExpr = buildSpacedValueQuery(field, text, true);
         break;
-    case "endswith":
-        text = parts.get(1).getLeft();
-        phraseExpr = buildSpacedValueQuery(field, text, false);
-        break;
Suggestion importance[1-10]: 3

__

Why: The suggestion itself acknowledges the current code is correct; the refactor to filter by type is a minor readability/robustness change with limited impact.

Low
Suggestions up to commit 4a1d335
CategorySuggestion                                                                                                                                    Impact
Possible issue
Preserve exists-guard on negated phrase branch

When applyDeMorgans is true the returned expression is NOT field: "...", but callers
wrap the result in the standard field-existence template only for the non-DeMorgan
path — here the returned string bypasses the exists guard used elsewhere,
producing semantics inconsistent with the wildcard branch (e.g. NOT CommandLine: "s"
AND exists: CommandLine). Ensure the DeMorgan-negated phrase branch also emits the
AND exists: field clause to match the existing convention and preserve correctness
for missing fields.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [302-327]

 String spacedShape = spacedPhraseShape(value);
 if (spacedShape != null) {
     var parts = value.getsOpt();
     String text;
     String phraseExpr;
     switch (spacedShape) {
         case "contains":
             text = parts.get(1).getLeft();
             phraseExpr = buildSpacedValueQuery(field, text, false);
             break;
         case "startswith":
             text = parts.get(0).getLeft();
             phraseExpr = buildSpacedValueQuery(field, text, true);
             break;
         case "endswith":
             text = parts.get(1).getLeft();
             phraseExpr = buildSpacedValueQuery(field, text, false);
             break;
         default:
             throw new IllegalStateException("Unexpected spaced phrase shape: " + spacedShape);
     }
     if (applyDeMorgans) {
-        return this.notToken + " " + phraseExpr;
+        return this.notToken + " " + phraseExpr + " AND " + this.existsToken + this.eqToken + " " + field;
     }
     return phraseExpr;
 }
Suggestion importance[1-10]: 7

__

Why: The observation is consistent with the test expectation NOT CommandLine: "s" AND _exists_: CommandLine, where the _exists_ clause appears in the wildcard branch. However, the _exists_ clause is added by an outer wrapper rather than inside convertConditionFieldEqValStr, so the fix as proposed may double-emit or be incorrect. The concern is legitimate but the exact fix needs verification.

Medium
Avoid lossy toString round-trip

val.getLeft().toString() collapses the original SigmaString (which may already
contain placeholders or wildcards) into a plain string and then reparses it via new
SigmaString(...), losing any existing special-char structure such as pre-existing
wildcards from other modifiers. Previously the ws round-trip was masking this, but
now any wildcard in the source value becomes literal text before
replaceWithPlaceholder runs. Operate on the original SigmaString directly instead of
round-tripping through toString().

src/main/java/org/opensearch/securityanalytics/rules/modifiers/SigmaWindowsDashModifier.java [42-43]

-return Either.left(new SigmaExpansion(new SigmaString(val.getLeft().toString()).replaceWithPlaceholder(Pattern.compile("\\B[-/]\\b"), "_windash")
-        .replacePlaceholders(callback).stream().map(s -> new SigmaString(s.toString())).collect(Collectors.toList())));
+SigmaString src = (SigmaString) val.getLeft();
+return Either.left(new SigmaExpansion(src.replaceWithPlaceholder(Pattern.compile("\\B[-/]\\b"), "_windash")
+        .replacePlaceholders(callback)));
Suggestion importance[1-10]: 6

__

Why: Valid concern about losing SigmaString structure via toString round-trip, though this pattern predates the PR (the old code also did toString). The proposed improved_code changes the return type signature and may not compile as-is.

Low
General
Avoid forced lowercasing of query text

Lowercasing the user-supplied text before emitting it as a phrase silently changes
case-sensitive rule semantics: a rule matching Admin will also match admin. This is
not equivalent to letting the analyzer normalize both sides, because the same text
is also emitted into a query_string that runs against fields whose analyzer may not
lowercase (e.g., keyword sub-fields or fields with a non-default analyzer). Emit the
original text and rely on the target field's analyzer for normalization.

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [603-611]

-String lower = text.toLowerCase(Locale.ROOT);
-int len = lower.length();
+String len_src = text;
+int len = len_src.length();
 int i = 0;
 while (i < len) {
     // Collect an alphanumeric token.
     int tokenStart = i;
-    while (i < len && Character.isLetterOrDigit(lower.charAt(i))) {
+    while (i < len && Character.isLetterOrDigit(len_src.charAt(i))) {
         i++;
     }
Suggestion importance[1-10]: 5

__

Why: Lowercasing may alter semantics for case-sensitive fields, but the tests explicitly assert lowercase output. The improved_code also introduces confusing variable renames without clearly addressing the concern.

Low
Broaden whitespace detection beyond space

The shape detector treats a value as "spaced" only if the text segment contains a
literal space, but Sigma also supports tab and other whitespace which the analyzer
likewise tokenizes. More importantly, values that contain a space embedded with
other wildcards (e.g. foo * bar, or a bc) fall through to the wildcard path and
re-introduce the bug of matching against an analyzed field with wildcards spanning
tokens. Consider using Character.isWhitespace and either broadening the shape
detection or documenting the unsupported cases.*

src/main/java/org/opensearch/securityanalytics/rules/backend/OSQueryBackend.java [549-553]

 if (parts.size() == 3
         && parts.get(0).isMiddle() && parts.get(0).getMiddle() == SigmaString.SpecialChars.WILDCARD_MULTI
-        && parts.get(1).isLeft() && parts.get(1).getLeft().contains(" ")
+        && parts.get(1).isLeft() && containsWhitespace(parts.get(1).getLeft())
         && parts.get(2).isMiddle() && parts.get(2).getMiddle() == SigmaString.SpecialChars.WILDCARD_MULTI) {
     return "contains";
 }
Suggestion importance[1-10]: 4

__

Why: The point about tabs/other whitespace is valid but likely minor since Sigma rules commonly use spaces. Edge cases with embedded wildcards are a real gap but the suggestion doesn't fully solve them.

Low

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c7ffeb9

@thecodingshrimp

thecodingshrimp commented Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review. Addressing both flagged areas:

Quoted string escaping (addEscaped + space)

The concern is valid to raise, but \ inside a Lucene query_string quoted phrase resolves correctly. The Lucene query parser treats backslash as the escape character in all contexts — both wildcard terms and quoted phrases — so "This\ is\ an\ example" is parsed as the phrase This is an example and matches ingest data containing plain spaces. This is the same escaping used by other Lucene-based backends (e.g. Elasticsearch/OpenSearch query DSL). The unit test testConvertValueStrWithWhitespaceQuoted asserting mappedA: "This\ is\ an\ example" reflects the correct emitted form.

Null regexp on error path

The assignment ordering is preserved (this.regexp = regexp before compile()). Regarding null input: Pattern.compile(null) throws NullPointerException, which is identical behavior to the previous code where regexp.replace(" ", "_ws_") would also throw NPE on null. The null contract is unchanged.

cudos: claude

@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch from c7ffeb9 to 6651c1a Compare August 28, 2026 23:44
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6651c1a

@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch from 6651c1a to b280664 Compare September 1, 2026 12:33
@github-actions

github-actions Bot commented Sep 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 4a1d335.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

PathLineSeverityDescription
src/test/java/org/opensearch/securityanalytics/rules/backend/QueryBackendWhitespaceITests.java51mediumHardcoded fallback password 'A3nonuser_' for the admin user is embedded as a literal string. Even in test code, committing a real-looking credential risks developers adopting it as their actual local-cluster password, and the value becomes permanently visible in git history.
src/test/java/org/opensearch/securityanalytics/rules/backend/QueryBackendWhitespaceITests.java120lowbuildPermissiveSslContext() installs a trust-all TrustManager and NoopHostnameVerifier, disabling TLS certificate validation entirely. Although the test targets localhost, this pattern is often copied into non-test contexts and could mask MITM attacks if the cluster URL is changed.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b280664

@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch 2 times, most recently from 2a525cc to dfe6919 Compare September 1, 2026 12:51
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit dfe6919

1 similar comment
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit dfe6919

@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch from dfe6919 to 4a1d335 Compare September 1, 2026 15:25
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4a1d335

@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch from 4a1d335 to 8af723d Compare September 2, 2026 09:24
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 8af723d

@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch from 8af723d to 6fc2d9a Compare September 2, 2026 11:46
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6fc2d9a

@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch from 6fc2d9a to 557b485 Compare September 2, 2026 14:27
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 557b485

@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch from 557b485 to b26e56c Compare September 3, 2026 11:53
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b26e56c

@thecodingshrimp
fix: fix SIGMA spaced-value queries for path-style and phrase-style v…
c764d9b 
…alues

Removing the legacy _ws_ whitespace encoding exposed several ways a space in a
SIGMA value produced query_string that was either rejected at percolator index
time or silently stopped matching, breaking integration tests that assert a fixed
number of indexed/matched rules (MapperRestApiIT.testWAFMappings, AlertsIT and
CorrelationEngineRestApiIT match-count assertions).

The target fields use rule_analyzer (keyword tokenizer), so a field value indexes
as a single token. All fixes target OSQueryBackend:

1) Field-bound spaced values (buildSpacedValueQuery): emit the value as a quoted
phrase wrapped in wildcards (*"text"* for contains, "text"* for startswith,
*"text" for endswith). A bare escaped-space wildcard (e.g. *C:\Program\ Files*)
fails to parse on path values, while a bare quoted phrase cannot substring-match
the single keyword-analyzed token. Wrapping the quoted phrase in wildcards both
parses reliably and restores contains/startswith/endswith matching. Separator
detection is decoupled from token lowercasing by iterating over the original text.

2) Unbound wildcard values adjacent to whitespace (convertConditionValStr): values
such as the SQL-injection rule's 'select * ' emitted a bare '*' via the unquoted
unboundWildcardExpression, which query_string rejects ("no field mapping for [*]").
A '*' next to whitespace cannot act as a query_string wildcard against the
keyword-analyzed single token anyway, so quote such values (literal '*' matches the
token); genuine whitespace-free wildcards like *Wfuzz* stay unquoted.

Add/adjust QueryBackendTests coverage for the wrapped contains/startswith/endswith
forms (including mixed space/dash and path-style values) and for the unbound
wildcard-with-whitespace case.

Signed-off-by: thecodingshrimp <leonard.stutzer@sap.com>
@thecodingshrimp
thecodingshrimp force-pushed the fix/sigma-whitespace-query-encoding branch from b26e56c to c764d9b Compare September 4, 2026 08:15
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c764d9b

@thecodingshrimp
thecodingshrimp marked this pull request as draft September 4, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amsiglan amsiglan Awaiting requested review from amsiglan amsiglan is a code owner
@AWSHurneyt AWSHurneyt Awaiting requested review from AWSHurneyt AWSHurneyt is a code owner
@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02 getsaurabh02 is a code owner
@lezzago lezzago Awaiting requested review from lezzago lezzago is a code owner
@eirsep eirsep Awaiting requested review from eirsep eirsep is a code owner
@engechas engechas Awaiting requested review from engechas engechas is a code owner
@goyamegh goyamegh Awaiting requested review from goyamegh goyamegh is a code owner
@riysaxen-amzn riysaxen-amzn Awaiting requested review from riysaxen-amzn riysaxen-amzn is a code owner
@toepkerd toepkerd Awaiting requested review from toepkerd toepkerd is a code owner
@vikhy-aws vikhy-aws Awaiting requested review from vikhy-aws vikhy-aws is a code owner
@manaswini1920 manaswini1920 Awaiting requested review from manaswini1920 manaswini1920 is a code owner
@nishtham-amazon nishtham-amazon Awaiting requested review from nishtham-amazon nishtham-amazon is a code owner
@KashKondaka KashKondaka Awaiting requested review from KashKondaka KashKondaka is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SIGMA rule translation -> lucene query replaces spaces " " with "_ws_" which lucene doesnt understand.

1 participant

@thecodingshrimp