Copilot commented on code in PR #880:
URL: https://github.com/apache/ranger/pull/880#discussion_r2964047152
##########
agents-common/src/main/java/org/apache/ranger/plugin/client/BaseClient.java:
##########
@@ -184,6 +185,162 @@ private void init() {
}
}
+ protected void validateSqlIdentifier(String identifier, String
identifierType) throws HadoopException {
+ if (StringUtils.isBlank(identifier)) {
+ return;
+ }
+ if (identifier.contains("..") || identifier.contains("//") ||
identifier.contains("\\")) {
+ String msgDesc = "Invalid " + identifierType + ": [" + identifier
+ "]. Path traversal patterns are not allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ if
(!identifier.matches("^[a-zA-Z0-9_*?\\[\\]\\-\\$%\\{\\}\\=\\/\\.]+$")) {
+ String msgDesc = "Invalid " + identifierType + ": [" + identifier
+ "]. Only alphanumeric characters along with ( ., _, -, *, ?, [], {}, %, $, =
/ ) are allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ }
+
+ protected String convertToSqlPattern(String pattern) throws
HadoopException {
+ if (pattern == null || pattern.isEmpty() || pattern.equals("*")) {
+ return "%";
+ }
+ return pattern.replace("*", "%");
+ }
+
+ protected boolean matchesSqlPattern(String value, String pattern) throws
HadoopException {
+ if (pattern == null || pattern.equals("%")) {
+ return true;
+ }
+
+ String regex = convertSqlPatternToRegex(pattern);
+ try {
+ return value.matches(regex);
+ } catch (PatternSyntaxException pe) {
+ String msgDesc = "Invalid " + value + ": [" + value + "]. Only
alphanumeric characters, underscores, asterisks, dots, hyphen, forward slash,
dollar signs, curly braces, percent signs are allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
Review Comment:
In `matchesSqlPattern()`, the `PatternSyntaxException` message is built
using the *value* (twice) and says "Invalid <value>: [<value>]". If this
exception occurs, it’s the generated regex/pattern that’s invalid, not the
matched value. Consider referencing the offending pattern/regex in the message
and aligning the allowed-character description with the actual validation logic
to make troubleshooting actionable.
##########
agents-common/src/main/java/org/apache/ranger/plugin/client/BaseClient.java:
##########
@@ -184,6 +185,162 @@ private void init() {
}
}
+ protected void validateSqlIdentifier(String identifier, String
identifierType) throws HadoopException {
+ if (StringUtils.isBlank(identifier)) {
+ return;
+ }
+ if (identifier.contains("..") || identifier.contains("//") ||
identifier.contains("\\")) {
+ String msgDesc = "Invalid " + identifierType + ": [" + identifier
+ "]. Path traversal patterns are not allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ if
(!identifier.matches("^[a-zA-Z0-9_*?\\[\\]\\-\\$%\\{\\}\\=\\/\\.]+$")) {
+ String msgDesc = "Invalid " + identifierType + ": [" + identifier
+ "]. Only alphanumeric characters along with ( ., _, -, *, ?, [], {}, %, $, =
/ ) are allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ }
+
+ protected String convertToSqlPattern(String pattern) throws
HadoopException {
+ if (pattern == null || pattern.isEmpty() || pattern.equals("*")) {
+ return "%";
+ }
+ return pattern.replace("*", "%");
+ }
+
+ protected boolean matchesSqlPattern(String value, String pattern) throws
HadoopException {
+ if (pattern == null || pattern.equals("%")) {
+ return true;
+ }
+
+ String regex = convertSqlPatternToRegex(pattern);
+ try {
+ return value.matches(regex);
+ } catch (PatternSyntaxException pe) {
+ String msgDesc = "Invalid " + value + ": [" + value + "]. Only
alphanumeric characters, underscores, asterisks, dots, hyphen, forward slash,
dollar signs, curly braces, percent signs are allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ }
+
+ protected void validateUrlResourceName(String resourceName, String
resourceType) throws HadoopException {
+ if (resourceName == null) {
+ return;
+ }
+ if (resourceName.contains("..") || resourceName.contains("//") ||
resourceName.contains("\\")) {
+ String msgDesc = "Invalid " + resourceType + ": [" + resourceName
+ "]. Path traversal patterns are not allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ if (!resourceName.matches("^[a-zA-Z0-9_.*\\-]+$")) {
+ String msgDesc = "Invalid " + resourceType + ": [" + resourceName
+ "]. Only alphanumeric characters, underscores, dots, hyphens, and simple
wildcards are allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ }
+
+ protected void validateWildcardPattern(String pattern, String patternType)
throws HadoopException {
+ if (pattern == null || pattern.isEmpty()) {
+ return;
+ }
+ if (pattern.contains("..") || pattern.contains("//") ||
pattern.contains("\\")) {
+ String msgDesc = "Invalid " + patternType + ": [" + pattern + "].
Path traversal patterns are not allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ if (!pattern.matches("^[a-zA-Z0-9_.*?\\[\\]\\-\\$%\\{\\}\\=\\/]+$")) {
+ String msgDesc = "Invalid " + patternType + ": [" + pattern + "].
Only alphanumeric characters, underscores, dots, hyphens, curly braces and
simple wildcards are allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
DEFAULT_ERROR_MESSAGE, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
+ }
+
+ protected String convertSqlPatternToRegex(String pattern) {
+ StringBuilder regexBuilder = new StringBuilder("^");
+
+ for (int i = 0; i < pattern.length(); i++) {
+ char c = pattern.charAt(i);
+ switch (c) {
+ case '%':
+ // SQL LIKE wildcard: zero or more characters
+ regexBuilder.append(".*");
+ break;
+ case '_':
+ // SQL LIKE wildcard: exactly one character
+ regexBuilder.append('.');
+ break;
+ case '.':
+ case '^':
+ case '$':
+ case '+':
+ case '?':
+ case '{':
+ case '}':
+ case '[':
+ case ']':
+ case '(':
+ case ')':
+ case '|':
+ case '\\':
+ // Escape regex metacharacters so they are treated
literally
+ regexBuilder.append('\\').append(c);
+ break;
+ default:
+ regexBuilder.append(c);
+ break;
+ }
+ }
+
+ return regexBuilder.toString();
+ }
+
+ protected String convertWildcardToRegex(String wildcard) {
+ if (wildcard == null || wildcard.isEmpty()) {
+ return ".*";
+ }
+ StringBuilder regex = new StringBuilder("^");
+ for (int i = 0; i < wildcard.length(); i++) {
+ char c = wildcard.charAt(i);
+ switch (c) {
+ case '*':
+ regex.append(".*");
+ break;
+ case '?':
+ regex.append(".");
+ break;
+ case '.':
+ case '\\':
+ case '^':
+ case '$':
+ case '|':
+ regex.append('\\').append(c);
+ break;
+ case '{':
+ case '}':
+ regex.append('\\').append(c);
+ break;
+ default:
Review Comment:
`validateWildcardPattern()` allows `[` and `]`, but
`convertWildcardToRegex()` does not escape them. Inputs like `foo[bar` will
pass validation, produce an invalid regex, and then callers like `HBaseClient`
will hit an uncaught `PatternSyntaxException` at `Pattern.compile(...)` (i.e.,
runtime failure instead of a `HadoopException`). Either disallow `[`/`]` in the
validation regex or escape them in `convertWildcardToRegex()` (and ensure any
other allowed regex metacharacters are handled safely).
##########
plugin-schema-registry/src/main/java/org/apache/ranger/services/schema/registry/client/AutocompletionAgent.java:
##########
@@ -118,15 +120,65 @@ public String toString() {
}
List<String> expandSchemaMetadataNameRegex(List<String> schemaGroupList,
String lookupSchemaMetadataName) {
+ validatePattern(lookupSchemaMetadataName, "schema metadata pattern");
+ String safePattern = convertWildcardToRegex(lookupSchemaMetadataName);
List<String> res = new ArrayList<>();
Collection<String> schemas = client.getSchemaNames(schemaGroupList);
schemas.forEach(sName -> {
- if (sName.matches(lookupSchemaMetadataName)) {
+ if (sName.matches(safePattern)) {
res.add(sName);
}
});
return res;
}
+
+ private void validatePattern(String pattern, String patternType) {
+ if (pattern == null || pattern.isEmpty()) {
+ return;
+ }
+ if (!pattern.matches("^[a-zA-Z0-9_*?\\[\\]\\-\\$%\\{\\}\\=\\/\\.]+$"))
{
+ String msgDesc = "Invalid " + patternType + ": [" + pattern + "].
Only alphanumeric characters along with ( _, -, *, ?, [], {}, %, $, = / ) are
allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
ERROR_MSG, null, null);
+ LOG.error(msgDesc);
+ throw hdpException;
+ }
Review Comment:
`validatePattern()` allows `.` and `/` (and therefore strings like `../...`)
but the error message doesn’t mention `.` and there is no path-traversal style
check (unlike the new shared validations in `BaseClient`). Consider either
tightening validation (e.g., reject `..`, `//`, `\\`) or updating the
allowed-character set/message so the behavior is consistent and predictable for
callers.
##########
plugin-trino/src/main/java/org/apache/ranger/services/trino/client/TrinoClient.java:
##########
@@ -576,6 +549,9 @@ private static String escapeSql(String str) {
if (str == null) {
return null;
}
- return StringUtils.replace(str, "'", "''");
+ str = str.replace("\\", "\\\\");
+ str = str.replace("\"", "\"\"");
+ str = str.replace("'", "''");
+ return str;
Review Comment:
`escapeSql()` is now unused in this class (all SQL string concatenation was
removed in favor of `DatabaseMetaData` APIs). Keeping and modifying an unused
escaping helper adds dead code and can be misleading for future changes;
consider removing it, or (if still needed elsewhere) using it via a shared
utility and adding unit coverage for its intended behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
