Copilot commented on code in PR #880:
URL: https://github.com/apache/ranger/pull/880#discussion_r2964117115
##########
agents-common/src/main/java/org/apache/ranger/plugin/client/BaseClient.java:
##########
@@ -184,6 +185,164 @@ 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 " + pattern + ": [" + value + "]. 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;
+ }
Review Comment:
In `matchesSqlPattern()`, the PatternSyntaxException message formats the
pattern/value in a confusing way: `Invalid <pattern>: [<value>]`. If this path
is hit, it will be hard to diagnose which input is invalid. Consider changing
it to clearly identify the invalid *pattern* and include the associated value
separately.
##########
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()` error message lists `_` as an allowed character, but the
regex does not permit underscores. This will reject valid schema metadata
patterns/names containing `_`. Add `_` to the allowed character class (or align
the message with the actual allowed set).
##########
agents-common/src/main/java/org/apache/ranger/plugin/client/BaseClient.java:
##########
@@ -184,6 +185,164 @@ 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_*?\\[\\]\\-\\$%\\{\\}\\=\\/\\.]+$")) {
Review Comment:
`validateSqlIdentifier()` claims underscores are allowed, but the validation
regex does not include `_`. This will incorrectly reject common identifiers
like `test_db`/`table_name` and will likely break the new Hive/Presto/Trino
validation paths (and the added tests that use underscores). Update the
character class to include `_` (and keep the error message consistent).
```suggestion
if
(!identifier.matches("^[a-zA-Z0-9*?\\[\\]\\-\\$%\\{\\}\\=\\/\\._]+$")) {
```
--
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]
