Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpUtil: fix invalid URI #4546

Merged
merged 5 commits into from
Jan 10, 2025
Merged

HttpUtil: fix invalid URI #4546

merged 5 commits into from
Jan 10, 2025

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Jan 8, 2025

Providing an invalid URI input was throwing an IllegalArgumentException from the underlying jetty library. This change properly catches it and throws an IOException instead, aligned with the error treatment in the rest of the method.

See also openhab/openhab-addons#18066

Signed-off-by: Mark Herwege <[email protected]>
@mherwege mherwege requested a review from a team as a code owner January 8, 2025 11:17
Signed-off-by: Mark Herwege <[email protected]>
@holgerfriedrich
Copy link
Member

@mherwege not a review yet, just a remark:
Since we are using Java 21 now, URL(string) ctors are anyway deprecated and need to be replaced by URI.create(...).toUrl().
I have not managed to do this yet, but already got into trouble by the different handling of exceptions. URL throws MalformedURLException, URI.create may throw NullPointerException or IllegalArgumentException.
So this sounds familiar.

Can url be null? Then method shouldUseProxy could be problematic as well, as it may call .matches on a null string if exclusions are given.

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Jan 9, 2025

Since we are using Java 21 now, URL(string) ctors are anyway deprecated and need to be replaced by URI.create(...).toUrl().

I don't think that is entirely what the documentation says. It says to replace it with URI.toUrl(). But to create the URI, new URI(String str) is still OK and the URI.create(String str) method documentation explicitely states that this convenience method should only be used when you are sure str is a well formed URI.
The issue I try to solve is that HttpClient.newRequest(String str) from Jetty expects a well formed URI as input, and it was called with a not checked URL. It is actually calling URI.create(url) internally. So I make sure it is a well formed URI before calling it. And indeed, that lead to the IllegalArgumentException.
I didn't find any direct use of the URL(String str) constructor in this class.

Can url be null?

As this is called from static public functions and never checked, I guess it could be. There are also no Null annotations. I will put in some checks for this.

Then method shouldUseProxy could be problematic as well, as it may call .matches on a null string if exclusions are given.

This will also be solved by checking for a null url before calling the executeUrlAndGetReponse method.

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Jan 9, 2025

I didn't find any direct use of the URL(String str) constructor in this class.

I didn't look carefully. Replaced now.

Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

@holgerfriedrich I created a separate PR to remove all uses of the URL constructor from core.

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@holgerfriedrich holgerfriedrich merged commit 6dffaa6 into openhab:main Jan 10, 2025
3 checks passed
@holgerfriedrich holgerfriedrich added the bug An unexpected problem or unintended behavior of the Core label Jan 10, 2025
@holgerfriedrich holgerfriedrich added this to the 5.0 milestone Jan 10, 2025
holgerfriedrich pushed a commit that referenced this pull request Jan 10, 2025
@holgerfriedrich holgerfriedrich added the patch A PR that has been cherry-picked to a patch release branch label Jan 10, 2025
@mherwege mherwege deleted the httputil branch January 11, 2025 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of the Core patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants