Copilot commented on code in PR #872:
URL: https://github.com/apache/ranger/pull/872#discussion_r3118689720
##########
kms/src/test/java/org/apache/hadoop/crypto/key/RangerMasterKeyTest.java:
##########
@@ -17,7 +17,7 @@
package org.apache.hadoop.crypto.key;
-import com.sun.jersey.core.util.Base64;
+import com.sun.org.apache.xml.internal.security.utils.Base64;
Review Comment:
This test now imports
`com.sun.org.apache.xml.internal.security.utils.Base64`, which is a
JDK-internal API. Depending on `--add-exports` for tests is brittle across JDK
updates and can break builds in more restricted environments. Prefer using
`java.util.Base64` (or another non-internal Base64 implementation already in
dependencies) and remove the need for module exports.
##########
dev-support/ranger-docker/docker-compose.ranger-kms.yml:
##########
@@ -31,6 +31,7 @@ services:
- KMS_VERSION
- RANGER_DB_TYPE
- KERBEROS_ENABLED
+ -
JAVA_OPTS=--add-exports=java.xml.crypto/com.sun.org.apache.xml.internal.security.utils=ALL-UNNAMED
Review Comment:
`JAVA_OPTS` is adding
`--add-exports=java.xml.crypto/com.sun.org.apache.xml.internal.security.utils=ALL-UNNAMED`,
which is only needed because tests are using JDK-internal `com.sun.*` Base64
APIs. This is a brittle workaround and increases operational complexity. Prefer
updating code/tests to use `java.util.Base64` and remove this export from the
container configuration.
##########
security-admin/src/main/java/org/apache/ranger/rest/RoleREST.java:
##########
@@ -663,7 +663,7 @@ public List<String>
getAllRoleNames(@QueryParam("serviceName") String serviceNam
@Path("/roles/{id}/addUsersAndGroups")
@Consumes("application/json")
@Produces("application/json")
- public RangerRole addUsersAndGroups(@PathParam("id") Long roleId,
List<String> users, List<String> groups, Boolean isAdmin) {
+ public RangerRole addUsersAndGroups(@PathParam("id") Long roleId,
@QueryParam("users") List<String> users, @QueryParam("groups") List<String>
groups, @QueryParam("isAdmin") Boolean isAdmin) {
Review Comment:
These endpoints were changed to take `users/groups/isAdmin` as
`@QueryParam`s while still declaring `@Consumes("application/json")`. This is
internally inconsistent (no JSON body is consumed) and is also a breaking
API/behavior change for clients that previously sent a JSON payload. Consider
introducing a single request-body DTO (e.g., `{ users: [...], groups: [...],
isAdmin: ... }`) and keep the content-type/contract consistent (or support both
for backward compatibility).
##########
security-admin/src/main/java/org/apache/ranger/rest/PublicAPIsv2.java:
##########
@@ -897,7 +897,7 @@ public List<String> getUserRoles(@PathParam("user") String
userName, @Context Ht
@Path("/api/roles/{id}/addUsersAndGroups")
@Consumes("application/json")
@Produces("application/json")
- public RangerRole addUsersAndGroups(@PathParam("id") Long roleId,
List<String> users, List<String> groups, Boolean isAdmin, @Context
HttpServletRequest request) {
+ public RangerRole addUsersAndGroups(@PathParam("id") Long roleId,
@QueryParam("users") List<String> users, @QueryParam("groups") List<String>
groups, @QueryParam("isAdmin") Boolean isAdmin, @Context HttpServletRequest
request) {
return roleREST.addUsersAndGroups(roleId, users, groups, isAdmin);
Review Comment:
This public API wrapper now passes `users/groups/isAdmin` as `@QueryParam`s
while still declaring `@Consumes("application/json")`. This changes the
on-the-wire contract and will break existing clients sending JSON bodies. Align
this method with the intended contract (prefer a single JSON request body DTO)
and ensure backward compatibility if these endpoints are already consumed
externally.
##########
plugin-nestedstructure/src/main/java/org/apache/ranger/authorization/nestedstructure/authorizer/RecordFilterJavaScript.java:
##########
@@ -54,8 +57,25 @@ public static boolean filterRow(String user, String
filterExpr, String jsonStrin
throw new MaskingException("cannot process filter expression due
to security concern \"this.engine\": " + filterExpr);
}
- NashornScriptEngineFactory factory = new NashornScriptEngineFactory();
- ScriptEngine engine =
factory.getScriptEngine(securityFilter);
+ ClassLoader clsLoader = Thread.currentThread().getContextClassLoader();
+ ScriptEngineManager mgr = new ScriptEngineManager(clsLoader);
+ ScriptEngine engine = mgr.getEngineByName("graal.js");
+
+ if (engine != null) {
+ try {
+ Map<String, Boolean> graalVmConfigs = new HashMap<>();
+
+ graalVmConfigs.put("polyglot.js.allowHostAccess",
Boolean.TRUE); // default is true for backward(Nashorn) compatibility
+ graalVmConfigs.put("polyglot.js.nashorn-compat",
Boolean.TRUE); // default is true for backward(Nashorn) compatibility
+
+ // enable configured script features
+ Bindings bindings =
engine.getBindings(ScriptContext.ENGINE_SCOPE);
+ bindings.putAll(graalVmConfigs);
+ engine.setBindings(bindings, ScriptContext.ENGINE_SCOPE);
Review Comment:
`polyglot.js.allowHostAccess` is being explicitly enabled (`Boolean.TRUE`)
when configuring the GraalJS engine. This effectively allows scripts to access
Java classes/host resources, which is a major security regression compared to
the previous Nashorn `ClassFilter` approach. Default should be host-access
disabled (and optionally allow enabling via an explicit config flag), and the
script-safety checks should not rely on string matching alone.
##########
plugin-yarn/src/main/java/org/apache/ranger/services/yarn/client/YarnClient.java:
##########
@@ -246,28 +250,32 @@ public List<String> run() {
}
if (client != null) {
- client.destroy();
+ client.close();
}
Review Comment:
`Response` objects should be closed to avoid leaking connections/resources.
In the success path (`status == 200`), `response` is kept for processing but
never closed (only the client is closed). Wrap the `Response` in try/finally
(or try-with-resources) so it is always closed after `readEntity()`/processing,
including the success case.
--
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]
