[ZEPPELIN-6204] do not allow init params in JDBC URLs for H2 by pjfanning · Pull Request #4949 · apache/zeppelin

pjfanning · 2025-06-25T14:07:52Z

What is this PR for?

Slight tidy of the existing disallow list for strings in JDBC urls so that they are checked against just the query params and not the hostname in the URL.

What type of PR is it?

Bug Fix

Todos

- Task

What is the Jira issue?

Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN/
Put link here, and add [ZEPPELIN-Jira number] in PR title, eg. [ZEPPELIN-533]

How should this be tested?

Strongly recommended: add automated unit tests for any new or changed behavior
Outline any manual steps to test the PR here.

Screenshots (if appropriate)

Questions:

Does the license files need to update? no
Is there breaking changes for older versions? no
Does this needs documentation? no

Reamer · 2025-06-26T19:48:37Z

I don't know whether we are preventing features that others want to use with these changes.

Doesn't it make more sense to secure the H2 server according to the requirements? After all, Zeppelin is just one JDBC client of many.

pjfanning · 2025-07-08T10:08:10Z

@Reamer @jongyoul is it ok to consider this for merge now? There are probably a few more query params that could be added to the disallow list but this covers some of the more commonly mentioned ones.

tbonelee · 2025-07-12T14:15:48Z

Just a small thought. I was wondering if decoding the full URL before parsing could lead to unexpected behavior in some edge cases. For example, if a parameter value contains encoded characters like %26 (&) or %3D (=), decoding them too early might interfere with how the parameters are parsed.

I’m not sure if values like passwords are actually allowed or commonly used in this context, so this might not be relevant. But I thought it was worth mentioning just in case. 🙏

seung-00 · 2025-07-12T14:49:19Z

+  private static final String ALLOW_LOAD_LOCAL = "allowLoadLocal";
+
  private static final String ALLOW_LOAD_LOCAL_IN_FILE_NAME = "allowLoadLocalInfile";

-  private static final String AUTO_DESERIALIZE = "autoDeserialize";
+  private static final String ALLOW_LOAD_LOCAL_IN_FILE_IN_PATH = "allowLoadLocalInfileInPath";

  private static final String ALLOW_LOCAL_IN_FILE_NAME = "allowLocalInfile";

  private static final String ALLOW_URL_IN_LOCAL_IN_FILE_NAME = "allowUrlInLocalInfile";

+  private static final String AUTO_DESERIALIZE = "autoDeserialize";
+
+  private static final String SOCKET_FACTORY = "socketFactory";
+
+  private static final String INIT = "INIT";
+


could be nice to group these into a Set.

I'm happy to stick with the existing code style.

seung-00 · 2025-07-12T14:56:56Z

+    }
+
+    // Split the URL into the base part and the parameters part
+    String[] parts = url.split("[?&;]");


could be a bit faster if the regex was cached to avoid rebuilding every time

the cost of this regex will be tiny compared to the I/O cost of using JDBC commands so I'm not very interested in optimising every line of code here

pjfanning · 2025-07-12T15:57:29Z

Just a small thought. I was wondering if decoding the full URL before parsing could lead to unexpected behavior in some edge cases. For example, if a parameter value contains encoded characters like %26 (&) or %3D (=), decoding them too early might interfere with how the parameters are parsed.

I’m not sure if values like passwords are actually allowed or commonly used in this context, so this might not be relevant. But I thought it was worth mentioning just in case. 🙏

The code in this PR ignores the param values. It only worries about the param names.
The original URL as entered by the user is passed to the JDBC layer - my code splits up the URL but only for my checks for suspicious param names.

tbonelee · 2025-07-12T16:53:47Z

@pjfanning Thanks for the clarification. I took a closer look and you're right. Decoding encoded special characters in values before validation would at most introduce an extra key, but it does not interfere with the parsing of subsequent parameters. I had initially worried about false negatives (i.e. cases where disallowed parameters might go undetected), but that doesn't seem to be the case. Thanks again, and sorry for the confusion.

pjfanning · 2025-08-03T16:08:27Z

@jongyoul @Reamer would you be able to review this change?

pjfanning · 2025-10-08T18:18:38Z

@jongyoul @Reamer would you be able to review this change? I think this fixes a couple of issues in the existing implementation.

Reamer

LGTM

pjfanning · 2025-10-27T21:57:14Z

@jongyoul do you think this could be merged?

jongyoul

LGTM

pjfanning · 2025-11-10T09:21:16Z

@jongyoul @Reamer I am not a committer so can't merge this. Would either of you be in a position to do so?

tbonelee · 2025-11-11T04:31:34Z

I’ve merged this into master.
Since I considered it a breaking change, I didn’t include it in branch-0.12.
Do you think it needs to be backported?

pjfanning · 2025-11-11T09:07:58Z

Thanks @tbonelee - I would regard it more as a bug fix or fixes. It shouldn't change the behaviour for any use cases that we want to support.

Reamer previously approved these changes Jul 1, 2025

View reviewed changes

pjfanning dismissed Reamer’s stale review via 9353662 July 2, 2025 09:58

pjfanning marked this pull request as draft July 2, 2025 11:34

pjfanning marked this pull request as ready for review July 8, 2025 10:06

seung-00 reviewed Jul 12, 2025

View reviewed changes

pjfanning added 14 commits August 3, 2025 15:08

ZEPPELIN-6204 do not allow init params in JDBC URLs for H2

b8034cf

line too long

810ffcf

checkstyle issue

ffc4553

Update JDBCInterpreter.java

995a46a

another dangerous query param

713ae26

more bad params

cb1f712

support mysql urls with props inside parens

7664378

more tests

d1fbec2

checkstyle: long lines

281dfd8

Update JDBCInterpreter.java

c97ab1a

Update JDBCInterpreter.java

e5e30a5

Update JDBCInterpreter.java

0bbe1e7

extra tests

7c95c7f

checkstyle issue

fbcec66

pjfanning force-pushed the ZEPPELIN-6204-jdbc branch from fd38a07 to fbcec66 Compare August 3, 2025 14:15

Reamer approved these changes Oct 8, 2025

View reviewed changes

jongyoul approved these changes Oct 28, 2025

View reviewed changes

tbonelee merged commit 4882e07 into apache:master Nov 11, 2025
16 of 18 checks passed

pjfanning deleted the ZEPPELIN-6204-jdbc branch November 11, 2025 09:08

Conversation

pjfanning commented Jun 25, 2025

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

Reamer commented Jun 26, 2025

Uh oh!

pjfanning commented Jul 8, 2025

Uh oh!

tbonelee commented Jul 12, 2025

Uh oh!

Uh oh!

Uh oh!

seung-00 Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

pjfanning Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

seung-00 Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

pjfanning Jul 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pjfanning commented Jul 12, 2025

Uh oh!

tbonelee commented Jul 12, 2025

Uh oh!

pjfanning commented Aug 3, 2025

Uh oh!

pjfanning commented Oct 8, 2025

Uh oh!

Reamer left a comment

Choose a reason for hiding this comment

Uh oh!

pjfanning commented Oct 27, 2025

Uh oh!

jongyoul left a comment

Choose a reason for hiding this comment

Uh oh!

pjfanning commented Nov 10, 2025

Uh oh!

Uh oh!

tbonelee commented Nov 11, 2025

Uh oh!

pjfanning commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants