Skip to content
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

Handle unicode characters in filters #707

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Handle unicode characters in filters #707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 10 additions & 5 deletions scim-spec/scim-spec-schema/src/main/antlr4/imports/Json.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ PLUS: '+';
ZERO: '0';

// Json String

STRING: '"' (CHAR)* '"';
fragment CHAR: UNESCAPED | ESCAPED;
fragment UNESCAPED : (' ' .. '!') | ('#' .. '[') | (']' .. '~');
fragment ESCAPED: '\\' ('"' | '\\' | '/' | 'b' | 'f' | 'n' | 'r' | 't');
STRING
: '"' (ESC | SAFECODEPOINT)* '"';
fragment ESC
: '\\' (["\\/bfnrt] | UNICODE);
fragment UNICODE
: 'u' HEX HEX HEX HEX;
fragment HEX
: [0-9a-fA-F];
fragment SAFECODEPOINT
: ~ ["\\\u0000-\u001F];
11 changes: 1 addition & 10 deletions ...ma/src/main/java/org/apache/directory/scim/spec/filter/AttributeComparisonExpression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@
import java.time.format.DateTimeFormatter;
import java.util.Date;

import org.apache.commons.lang3.StringEscapeUtils;

import org.apache.directory.scim.spec.filter.attribute.AttributeReference;
import lombok.Value;

Expand Down Expand Up @@ -88,14 +86,7 @@ private String createCompareValueString() {
if (this.compareValue == null) {
compareValueString = "null";
} else if (this.compareValue instanceof String) {
// TODO change this to escapeJson() when dependencies get upgraded
String escaped = StringEscapeUtils.escapeEcmaScript((String) this.compareValue)
// StringEscapeUtils follows the outdated JSON spec requiring "/" to be escaped, this could subtly break things
.replaceAll("\\\\/", "/")
// We don't want single-quotes escaped, this will be unnecessary with escapeJson()
.replaceAll("\\\\'", "'");
Comment on lines -91 to -96
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, just now giving this a quick look, @bdemers. Can we note for posterity, even if just in this PR convo, where escapeJson() is now being used?


compareValueString = QUOTE + escaped + QUOTE;
compareValueString = QUOTE + this.compareValue + QUOTE;
} else if (this.compareValue instanceof Date) {
compareValueString = QUOTE + toDateTimeString((Date) this.compareValue) + QUOTE;
} else if (this.compareValue instanceof LocalDate) {
Expand Down
20 changes: 3 additions & 17 deletions ...chema/src/main/java/org/apache/directory/scim/spec/filter/ExpressionBuildingListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@
import java.util.Deque;
import java.util.Locale;

import org.apache.commons.lang3.StringEscapeUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.apache.directory.scim.spec.filter.FilterParser.AttributeCompareExpressionContext;
import org.apache.directory.scim.spec.filter.FilterParser.AttributeGroupExpressionContext;
import org.apache.directory.scim.spec.filter.FilterParser.AttributeLogicExpressionContext;
Expand All @@ -41,8 +37,6 @@

public class ExpressionBuildingListener extends FilterBaseListener {

private static final Logger LOG = LoggerFactory.getLogger(ExpressionBuildingListener.class);

protected Deque<FilterExpression> expressionStack = new ArrayDeque<>();

@Override
Expand Down Expand Up @@ -160,14 +154,8 @@ public FilterExpression getFilterExpression() {

private static Object parseJsonType(String jsonValue) {
if (jsonValue.startsWith("\"")) {
String doubleEscaped = jsonValue.substring(1, jsonValue.length() - 1)
// StringEscapeUtils follows the outdated JSON spec requiring "/" to be escaped, this could subtly break things
.replaceAll("\\\\/", "\\\\\\\\/")
// Just in case someone needs a single-quote with a backslash in front of it, this will be unnecessary with escapeJson()
.replaceAll("\\\\'", "\\\\\\\\'");

// TODO change this to escapeJson() when dependencies get upgraded
return StringEscapeUtils.unescapeEcmaScript(doubleEscaped);
Comment on lines -163 to -170
Copy link
Contributor

Choose a reason for hiding this comment

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

@bdemers Can we note for posterity, even if just in this PR convo, where escapeJson() is now being used?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was hoping @chrisharm would chime in 🤞
My take is that the escape would happen at a different level:

  • filters passed in as a query param would be URL escaped
  • Patch paths, would be JSON escaped automatically by Jackson.

Unless I'm missing something i think it would always be problematic to handle escaping outside of the context where it's used.

// remove leading and trailing slash
return jsonValue.substring(1, jsonValue.length() - 1);
} else if ("null".equals(jsonValue)) {
return null;
} else if ("true".equals(jsonValue)) {
Expand All @@ -182,11 +170,9 @@ private static Object parseJsonType(String jsonValue) {
return Integer.parseInt(jsonValue);
}
} catch (NumberFormatException e) {
LOG.warn("Unable to parse a json number: " + jsonValue);
throw new IllegalStateException("Unable to parse JSON number: " + jsonValue, e);
}
}

throw new IllegalStateException("Unable to parse JSON Value");
}

}
10 changes: 10 additions & 0 deletions ...im-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ public void testSimpleAnd() throws FilterParseException {
.build();
assertThat(filter).isEqualTo(new Filter("name.givenName EQ \"Bilbo\" AND name.familyName EQ \"Baggins\""));
}

@Test
public void testExtendedCharsAnd() throws FilterParseException {
Filter filter = FilterBuilder.create()
.equalTo("name.givenName", "Bílbo")
.and(r -> r.equalTo("name.familyName", "Bággins"))
.build();
Filter expected = new Filter("name.givenName EQ \"Bílbo\" AND name.familyName EQ \"Bággins\"");
assertThat(filter).isEqualTo(expected);
}

@Test
public void testSimpleOr() throws FilterParseException {
Expand Down
Loading