feat(jdbc): enforce strict JDBC URL parsing and sync DataSource properties#4107
feat(jdbc): enforce strict JDBC URL parsing and sync DataSource properties#4107keshavdandeva wants to merge 12 commits intomainfrom
DataSource properties#4107Conversation
Summary of ChangesHello @keshavdandeva, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the BigQuery JDBC driver by implementing stricter URL parsing to prevent configuration errors and by aligning the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request enhances the robustness of JDBC URL parsing through stricter validation, refactored logic, and synchronization of DataSource properties. However, the new parsing logic introduces a connection string injection vulnerability due to unescaped delimiters in property values and insufficient validation for sensitive properties like LogPath. This flaw could enable path traversal or the overriding of security-sensitive connection settings. It is recommended to implement proper escaping for property values and add validation for security-critical properties during URL parsing.
google-cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryDriver.java
Show resolved
Hide resolved
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly improves the JDBC driver's connection string parsing by introducing stricter validation for property names, preventing silent failures, and correctly URL-encoding property values. It also synchronizes DataSource properties with BigQueryConnection for enhanced feature parity. However, a potential information exposure vulnerability exists: the parseUrl method could leak sensitive data in SQLException messages if a secret is accidentally provided as a property key, as the raw key is included in error messages. Additionally, there is a minor suggestion to reduce code duplication.
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Outdated
Show resolved
Hide resolved
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Outdated
Show resolved
Hide resolved
google-cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryDriver.java
Show resolved
Hide resolved
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Show resolved
Hide resolved
| try { | ||
| BigQueryJdbcUrlUtility.parseUrl(connectionUri); | ||
| } catch (BigQueryJdbcRuntimeException e) { | ||
| throw new BigQueryJdbcException(e.getMessage(), e); |
There was a problem hiding this comment.
What's the reason to wrap BigQueryJdbcRuntimeException with BigQueryJdbcException?
There was a problem hiding this comment.
Because the JDBC spec required the connect() method to throw SQLException for connection failures and BigQueryJdbcException extends SQLException
| private Integer metadataFetchThreadCount; | ||
| private String sslTrustStorePath; | ||
| private String sslTrustStorePassword; | ||
| private String labels; |
There was a problem hiding this comment.
labels supposed to be Map<String, String>, right?
There was a problem hiding this comment.
Good catch! Also, I realised for properties like labels and queryProperties we needed to serialize maps
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Outdated
Show resolved
Hide resolved
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Outdated
Show resolved
Hide resolved
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Show resolved
Hide resolved
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Outdated
Show resolved
Hide resolved
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Outdated
Show resolved
Hide resolved
...cloud-bigquery-jdbc/src/main/java/com/google/cloud/bigquery/jdbc/BigQueryJdbcUrlUtility.java
Outdated
Show resolved
Hide resolved
| } | ||
| String[] kv = part.split("=", 2); | ||
| String key = kv[0].trim(); | ||
| if (kv.length == 1) { |
There was a problem hiding this comment.
That seems easier to read?
String key = kv[0].trim().toUpperCase();
if (kv.length != 2 || !PROPERTY_NAME_MAP.containsKey(key) ) {
throw new BigQueryJdbcRuntimeException(String.format("Wrong value or unknown setting found in connection string: %s", part.substring(0, Math.min(part.length(), 12)))
}
String value = kv[1].trim();
map.put(key, CharEscapers.decodeUriPath(value));
There was a problem hiding this comment.
Adopted your suggestion but with modified error handling. I kept it to log the key if available (truncated to 32 chars) or the truncated part only if the property is malformed.
b/429272203
This PR refactors the JDBC connection string parsing logic to be stricter and more robust, preventing silent failures due to typos. It also synchronizes
DataSourceproperties withBigQueryConnectionto ensure full feature parity.