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

support empty password #615

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jsvd
Copy link
Member

@jsvd jsvd commented Jul 4, 2017

fixes #614

@jsvd jsvd requested a review from andrewvc July 4, 2017 11:01
@jsvd
Copy link
Member Author

jsvd commented Jul 4, 2017

these test failures are very strange, it seems only the tests are being used, not the library code

@jsvd jsvd closed this Jul 18, 2017
@jsvd jsvd reopened this Jul 18, 2017
@jsvd jsvd force-pushed the support_empty_password branch from 3cadc18 to b051ac5 Compare August 23, 2017 14:45
@jsvd jsvd force-pushed the support_empty_password branch from b051ac5 to a65404c Compare March 16, 2018 11:37
@jsvd jsvd force-pushed the support_empty_password branch from a65404c to 8377695 Compare May 16, 2018 14:37
@webmat
Copy link
Contributor

webmat commented May 28, 2018

The CGI.unescape exceptions happen in another code path: the healthcheck code path. In it, manticore_adapter.rb does try to read back the password from the ::LogStash::Util::SafeURI instance (which in turn returns nil when empty).

If we guard for a nil password here as well (e.g. with CGI.unescape(url.password || '')) then the we get 1/2 tests passing. The one with the empty password in the options.

The other one still reads a nil password from the hosts URI instances, and password is not forced to '' when nil.

I wonder if just adding the aforementioned || '' in manticore_adapter.rb would actually be enough to support empty passwords.

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

Successfully merging this pull request may close these issues.

empty password causes logstash to crash
3 participants