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

URLUtils.isValidURL #3548

Closed
shawkins opened this issue Oct 26, 2021 · 7 comments · Fixed by #3558
Closed

URLUtils.isValidURL #3548

shawkins opened this issue Oct 26, 2021 · 7 comments · Fixed by #3558
Labels
Milestone

Comments

@shawkins
Copy link
Contributor

shawkins commented Oct 26, 2021

In this method there's a check of if the string form contains the string null, presumably to guard against null concatenation. However in general it's quite possible for the string "null" to appear in a valid URL.

@MUzairS15
Copy link
Contributor

Can you help me understand better? Under what conditions, this check may fail.

@manusa manusa changed the title UriUtils.isValidURL URLUtils.isValidURL Oct 27, 2021
@manusa
Copy link
Member

manusa commented Oct 27, 2021

😳 This check should probably be removed.

Added in #1139, not sure why.

@manusa manusa added the bug label Oct 27, 2021
@MUzairS15
Copy link
Contributor

MUzairS15 commented Oct 28, 2021

@manusa
Copy link
Member

manusa commented Nov 2, 2021

To be clear, the only thing you should remove is the url.contains("null") check, the rest should stay.

MUzairS15 added a commit to MUzairS15/kubernetes-client that referenced this issue Nov 2, 2021
MUzairS15 added a commit to MUzairS15/kubernetes-client that referenced this issue Nov 2, 2021
@manusa
Copy link
Member

manusa commented Nov 9, 2021

@MUzairS15 are you working on this and going to provide a PR?

@rohanKanojia rohanKanojia linked a pull request Nov 9, 2021 that will close this issue
11 tasks
@rohanKanojia
Copy link
Member

Looks like @MUzairS15 had created PR but didn't link the issue #3558

@MUzairS15
Copy link
Contributor

MUzairS15 commented Nov 10, 2021

Sorry @manusa I didn't linked the issue, thankyou @rohanKanojia for mentioning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants