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

[pjfanning]

What is this PR for?

ZEPPELIN-6204

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]

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]

@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]

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]

Suggested change

	if (containsKeyIgnoreCase(params, ALLOW_LOAD_LOCAL) ||
	containsKeyIgnoreCase(params, ALLOW_LOAD_LOCAL_IN_FILE_NAME) ||
	containsKeyIgnoreCase(params, ALLOW_LOCAL_IN_FILE_NAME) ||
	containsKeyIgnoreCase(params, ALLOW_URL_IN_LOCAL_IN_FILE_NAME) ||
	containsKeyIgnoreCase(params, ALLOW_LOAD_LOCAL_IN_FILE_IN_PATH) ||
	containsKeyIgnoreCase(params, AUTO_DESERIALIZE) ||
	containsKeyIgnoreCase(params, SOCKET_FACTORY)) {
	throw new IllegalArgumentException("Connection URL contains sensitive configuration");
	}
	Stream.of(ALLOW_LOAD_LOCAL, ALLOW_LOCAL_IN_FILE_NAME, ...).forEach(rule -> {
	if (containsKeyIgnoreCase(params, rule)) throw new IllegalArgumentException("Connection URL contains sensitive configuration: " + rule);
	});

I find this version easier to read.

[pjfanning]

I can't commit this unless it compiles so please don't use the suggestion option to suggest incomplete code.
I also disagree that this is more readable. The existing code in this PR is closer to the existing style in the existing codebase.

[seung-00]

could be nice to group these into a Set.

[pjfanning]

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

[seung-00]

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

[pjfanning]

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

[seung-00]

Does it work well if parentheses are in the URL query parameters?
jdbc:mysql://.../db?password=abc(def)

[pjfanning]

the code doesn't care about what the values are, it only cares about the param names - write some test cases if you are suspicious

[seung-00]

I tested the following cases:

testBannedMySQLQueryParamWithValidParam("password=allowLoadLocalInfile"); // 1. fail
testBannedMySQLQueryParamWithValidParam("password=(allowLoadLocalInfile)"); // 2. pass

I might be missing something, but test case 2 seems unintended.
Part of the value allowLoadLocalInfile appears to be treated as a key due to the parentheses.

[pjfanning]

thanks - I'll look into the issue - but in the end of the day, it's not a very likely password
and the existing checks before this PR, they just do a string match on the whole URL so that code wouldn't handle this either

[pjfanning]

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]

@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.