Copilot commented on code in PR #880:
URL: https://github.com/apache/ranger/pull/880#discussion_r2964197173
##########
knox-agent/src/main/java/org/apache/ranger/services/knox/client/KnoxClient.java:
##########
@@ -420,4 +421,32 @@ public List<String> getServiceList(List<String>
knoxTopologyList, String service
}
return serviceList;
}
+
+ private void validateResourceName(String resourceName, String
resourceType) {
+ 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 +
ERROR_MSG, null, null);
+
+ LOG.error(msgDesc);
+
+ throw hdpException;
+ }
+
+ if (!resourceName.matches("^[a-zA-Z0-9_.*\\-]+$")) {
+ String msgDesc = "Invalid " + resourceType + ": [" +
resourceName + "]. Only alphanumeric characters, dots, underscores, hyphens,
and wildcards are allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
ERROR_MSG, null, null);
+
+ LOG.error(msgDesc);
+
+ throw hdpException;
+ }
Review Comment:
`validateResourceName()` duplicates path-traversal + allowlist validation
logic that now exists in `BaseClient.validateUrlResourceName()`. Consider
extracting to a shared utility (or reusing `BaseClient`’s implementation) to
avoid divergence in validation rules and error text across plugins.
```suggestion
BaseClient.validateUrlResourceName(resourceName, resourceType,
ERROR_MSG);
```
##########
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("*", "%");
Review Comment:
`convertToSqlPattern()` only converts `*` to `%`, but
`validateSqlIdentifier()` explicitly allows `?` and the error message suggests
`?` is supported. With the current implementation, `?` is treated as a literal
in JDBC metadata patterns and in `matchesSqlPattern()`, which is likely
surprising/inconsistent for callers. Consider either (a) converting `?` to SQL
LIKE `_` in `convertToSqlPattern()`, or (b) disallowing `?` / updating the
allowed-chars message to match actual behavior.
```suggestion
if (pattern == null || pattern.isEmpty()) {
return "%";
}
// Convert custom wildcards to SQL LIKE pattern:
// '*' -> '%' (multi-character wildcard)
// '?' -> '_' (single-character wildcard)
String sqlPattern = pattern.replace("*", "%").replace("?", "_");
return sqlPattern;
```
##########
plugin-solr/src/main/java/org/apache/ranger/services/solr/client/ServiceSolrClient.java:
##########
@@ -619,6 +619,34 @@ private void login(Map<String, String> configs) {
}
}
+ private void validateResourceName(String resourceName, String
resourceType) {
+ 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 +
RangerSolrConstants.errMessage, null, null);
+
+ LOG.error(msgDesc);
+
+ throw hdpException;
+ }
+
+ if (!resourceName.matches("^[a-zA-Z0-9_.*\\-]+$")) {
+ String msgDesc = "Invalid " + resourceType + ": [" +
resourceName + "]. Only alphanumeric characters, dots, underscores, hyphens,
and wildcards are allowed.";
+ HadoopException hdpException = new HadoopException(msgDesc);
+
+ hdpException.generateResponseDataMap(false, msgDesc, msgDesc +
RangerSolrConstants.errMessage, null, null);
+
+ LOG.error(msgDesc);
+
+ throw hdpException;
+ }
Review Comment:
`validateResourceName()` duplicates the same path-traversal + character
allowlist validation logic now present in
`BaseClient.validateUrlResourceName()`. Consider extracting this to a shared
utility (or adjusting visibility/placement) so URL resource validation stays
consistent across plugins and future changes only need to be made in one place.
```suggestion
BaseClient.validateUrlResourceName(resourceName, resourceType);
```
##########
plugin-schema-registry/src/main/java/org/apache/ranger/services/schema/registry/client/AutocompletionAgent.java:
##########
@@ -38,6 +39,7 @@ public class AutocompletionAgent {
private static final String errMessage = "You can still save the
repository and start creating policies, but you would not be able to use
autocomplete for resource names. Check server logs for more info.";
private static final String successMsg = "ConnectionTest Successful";
+ private static final String ERROR_MSG = " You can still save the
repository and start creating policies, but you would not be able to use
autocomplete for resource names. Check server logs for more info.";
Review Comment:
`errMessage` and `ERROR_MSG` contain the same text (with only a
leading-space difference) and are used in different paths. Keeping two
near-identical constants increases the chance of inconsistent user-facing
messages; consider consolidating to a single constant (and add the leading
space at concatenation time if needed).
```suggestion
private static final String ERROR_MSG = " " + errMessage;
```
##########
plugin-presto/src/main/java/org/apache/ranger/services/presto/client/PrestoClient.java:
##########
@@ -300,43 +301,41 @@ private List<String> getCatalogs(String needle,
List<String> catalogs) throws Ha
List<String> ret = new ArrayList<>();
if (con != null) {
- Statement stat = null;
- ResultSet rs = null;
- String sql = "SHOW CATALOGS";
+ ResultSet rs = null;
try {
- if (needle != null && !needle.isEmpty() &&
!needle.equals("*")) {
- // Cannot use a prepared statement for this as presto does
not support that
- sql += " LIKE '" + escapeSql(needle) + "%'";
- }
-
- stat = con.createStatement();
- rs = stat.executeQuery(sql);
+ validateSqlIdentifier(needle, "catalog pattern");
+ String catalogPattern = convertToSqlPattern(needle);
+ rs = con.getMetaData().getCatalogs();
while (rs.next()) {
- String catalogName = rs.getString(1);
+ String catalogName = rs.getString("TABLE_CAT");
if (catalogs != null && catalogs.contains(catalogName)) {
continue;
}
- ret.add(catalogName);
+ if (catalogPattern == null || catalogPattern.equals("%")
|| matchesSqlPattern(catalogName, catalogPattern)) {
+ ret.add(catalogName);
+ }
}
} catch (SQLTimeoutException sqlt) {
- String msgDesc = "Time Out, Unable to execute
SQL [" + sql + "].";
+ String msgDesc = "Time Out, Unable to retrieve
catalog list.";
HadoopException hdpException = new HadoopException(msgDesc,
sqlt);
hdpException.generateResponseDataMap(false, getMessage(sqlt),
msgDesc + ERR_MSG, null, null);
+ throw hdpException;
} catch (SQLException se) {
- String msg = "Unable to execute SQL [" + sql + "]. ";
+ String msg = "Unable to retrieve catalog list. ";
HadoopException he = new HadoopException(msg, se);
he.generateResponseDataMap(false, getMessage(se), msg +
ERR_MSG, null, null);
throw he;
+ } catch (HadoopException he) {
+ throw he;
Review Comment:
There is a minor formatting issue: `throw he;` has an extra space, which
can trip style checks in some builds. Please normalize to a single space.
```suggestion
throw he;
```
--
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]
