Copilot commented on code in PR #931: URL: https://github.com/apache/ranger/pull/931#discussion_r3197141571
########## hive-agent/src/main/java/org/apache/ranger/services/hive/client/JdbcUrlValidator.java: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ranger.services.hive.client; + +import org.apache.ranger.plugin.client.HadoopException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.net.URLDecoder; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +public final class JdbcUrlValidator { + private static final Logger LOG = LoggerFactory.getLogger(JdbcUrlValidator.class); + private static final Set<String> BLOCKED_PARAMS = Collections.unmodifiableSet( + new HashSet<>(Arrays.asList( + "socketfactory", "socketfactoryarg", "sslfactory", "sslfactoryarg", + "sslhostnameverifier", "authenticationpluginclassname", "loggerclassname", + "kerberosservername", "gssdelegatecred", "sslpasswordcallback"))); + + private JdbcUrlValidator() { + } + + public static void validate(String jdbcUrl) throws HadoopException { + if (jdbcUrl == null || jdbcUrl.trim().isEmpty()) { + HadoopException e = new HadoopException("jdbc.url must not be null or empty"); + e.generateResponseDataMap(false, "Validation failed", "jdbc.url is required", + null, "jdbc.url"); + throw e; + } + String trimmed = jdbcUrl.trim(); + int queryStart = trimmed.indexOf('?'); + if (queryStart == -1) { + queryStart = trimmed.indexOf(';'); + } + if (queryStart != -1) { + String queryString = trimmed.substring(queryStart + 1); + validateQueryString(queryString, trimmed); + } Review Comment: JdbcUrlValidator.validate() chooses '?' as the query-start delimiter even if a ';' occurs earlier in the URL. This can miss parameters placed in the ';' section (e.g., "...;socketFactory=evil?user=x"), allowing blocked/dangerous params to bypass validation. Compute the query-start as the earliest occurrence of either '?' or ';' (when present), and ensure the validation covers the entire parameter section(s). ########## hive-agent/src/main/java/org/apache/ranger/services/hive/client/JdbcUrlValidator.java: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ranger.services.hive.client; + +import org.apache.ranger.plugin.client.HadoopException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.net.URLDecoder; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +public final class JdbcUrlValidator { + private static final Logger LOG = LoggerFactory.getLogger(JdbcUrlValidator.class); + private static final Set<String> BLOCKED_PARAMS = Collections.unmodifiableSet( + new HashSet<>(Arrays.asList( + "socketfactory", "socketfactoryarg", "sslfactory", "sslfactoryarg", + "sslhostnameverifier", "authenticationpluginclassname", "loggerclassname", + "kerberosservername", "gssdelegatecred", "sslpasswordcallback"))); + + private JdbcUrlValidator() { + } + + public static void validate(String jdbcUrl) throws HadoopException { + if (jdbcUrl == null || jdbcUrl.trim().isEmpty()) { + HadoopException e = new HadoopException("jdbc.url must not be null or empty"); + e.generateResponseDataMap(false, "Validation failed", "jdbc.url is required", + null, "jdbc.url"); + throw e; + } + String trimmed = jdbcUrl.trim(); + int queryStart = trimmed.indexOf('?'); + if (queryStart == -1) { + queryStart = trimmed.indexOf(';'); + } + if (queryStart != -1) { + String queryString = trimmed.substring(queryStart + 1); + validateQueryString(queryString, trimmed); + } + LOG.debug("jdbc.url passed validation: {}", sanitizeForLog(trimmed)); + } + + private static void validateQueryString(String queryString, String fullUrl) throws HadoopException { + String[] tokens = queryString.split("[&;]"); + for (String token : tokens) { + if (token.trim().isEmpty()) { + continue; + } + int eqIdx = token.indexOf('='); Review Comment: validateQueryString() splits tokens only on '&' and ';'. If validation starts at the first ';', a later '?' (e.g., "ssl=true?socketFactory=evil") will keep "?socketFactory" inside the same token and skip detection. Consider splitting on '?' as well (or otherwise normalizing the remaining string) so all parameter names are checked regardless of separator order. ########## hive-agent/src/main/java/org/apache/ranger/services/hive/client/JdbcUrlValidator.java: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ranger.services.hive.client; + +import org.apache.ranger.plugin.client.HadoopException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.net.URLDecoder; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashSet; +import java.util.Set; + +public final class JdbcUrlValidator { + private static final Logger LOG = LoggerFactory.getLogger(JdbcUrlValidator.class); + private static final Set<String> BLOCKED_PARAMS = Collections.unmodifiableSet( + new HashSet<>(Arrays.asList( + "socketfactory", "socketfactoryarg", "sslfactory", "sslfactoryarg", + "sslhostnameverifier", "authenticationpluginclassname", "loggerclassname", + "kerberosservername", "gssdelegatecred", "sslpasswordcallback"))); + + private JdbcUrlValidator() { + } + + public static void validate(String jdbcUrl) throws HadoopException { + if (jdbcUrl == null || jdbcUrl.trim().isEmpty()) { + HadoopException e = new HadoopException("jdbc.url must not be null or empty"); + e.generateResponseDataMap(false, "Validation failed", "jdbc.url is required", + null, "jdbc.url"); + throw e; + } + String trimmed = jdbcUrl.trim(); + int queryStart = trimmed.indexOf('?'); + if (queryStart == -1) { + queryStart = trimmed.indexOf(';'); + } + if (queryStart != -1) { + String queryString = trimmed.substring(queryStart + 1); + validateQueryString(queryString, trimmed); + } + LOG.debug("jdbc.url passed validation: {}", sanitizeForLog(trimmed)); + } + + private static void validateQueryString(String queryString, String fullUrl) throws HadoopException { + String[] tokens = queryString.split("[&;]"); + for (String token : tokens) { + if (token.trim().isEmpty()) { + continue; + } + int eqIdx = token.indexOf('='); + String paramName = (eqIdx >= 0 ? token.substring(0, eqIdx) : token).trim(); + String decodedParamName = paramName; + try { + decodedParamName = URLDecoder.decode(paramName, "UTF-8"); + } catch (Exception e) { + LOG.warn("Failed to decode parameter name: {}", paramName); + } + + String normalized = decodedParamName.toLowerCase().trim().replaceAll("[._-]", ""); + if (BLOCKED_PARAMS.contains(normalized)) { + logAndThrow("blocked parameter", normalized, paramName, fullUrl); + } + String[] dangerPatterns = {"socketfactory", "sslfactory", "autodeserialize"}; + for (String danger : dangerPatterns) { + if (normalized.contains(danger)) { + logAndThrow("dangerous pattern '" + danger + "'", normalized, paramName, fullUrl); + } + } Review Comment: The dangerPatterns array is allocated on every parameter token iteration. Making this a static final constant (or at least declared once outside the loop) avoids repeated allocations and keeps the list of dangerous patterns in one place. -- 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]
