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

Setting schema-less URL for proxy URLs results in NullPointerException #1129

Closed
aaronq opened this issue Mar 23, 2023 · 2 comments · Fixed by #1133
Closed

Setting schema-less URL for proxy URLs results in NullPointerException #1129

aaronq opened this issue Mar 23, 2023 · 2 comments · Fixed by #1133
Assignees
Labels
enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client
Milestone

Comments

@aaronq
Copy link

aaronq commented Mar 23, 2023

In slack-api-client v1.28.0, there seems to be a bug with whole proxies can be set in the SlackConfig object.

Upon initialization of SlackConfig, it will call initProxyUrl() which looks at a bunch of system variables and if they are not set, environment variables. I believe the code is expecting HTTPS_PROXY or HTTP_PROXY to be set to the full url of the proxy (eg. http://proxy.com:8080/) which gets parsed later in ProxyUrlUtil.parse().

However, in our case, these environment variables are set with the syntax(which I believe is the norm) - hostname:port (eg. proxy.somewhere.com:8080) so when it gets parsed, it will throw a NPE when it tries to split the proxy string by ://.

Also it seems that setting slackConfig.setProxy() manually takes no effect after the config has been initialized and it has found a value in one of these variables.

The Slack SDK version

[INFO] +- com.slack.api:bolt-socket-mode:jar:1.28.0:compile
[INFO] | +- com.slack.api:slack-api-model:jar:1.28.0:compile
[INFO] | +- com.slack.api:slack-api-client:jar:1.28.0:compile
[INFO] | +- com.slack.api:slack-app-backend:jar:1.28.0:compile
[INFO] | - com.slack.api:bolt:jar:1.28.0:compile

Java Runtime version

openjdk version "11.0.9" 2020-10-20 LTS
OpenJDK Runtime Environment Zulu11.43+54-SA (build 11.0.9+11-LTS)
OpenJDK 64-Bit Server VM Zulu11.43+54-SA (build 11.0.9+11-LTS, mixed mode)

OS info

ProductName: macOS
ProductVersion: 13.2.1
BuildVersion: 22D68
Darwin Kernel Version 22.3.0: Mon Jan 30 20:42:11 PST 2023; root:xnu-8792.81.3~2/RELEASE_X86_64

Steps to reproduce:

Export HTTPS_PROXY='proxy.somewhere.com:8080' to set the environment variable

SlackConfig slackConfig = new SlackConfig();
slack = Slack.getInstance(slackConfig);

You should get NPE after you try running this code

Expected result:

Proxy logic should be able to handle the case where it doesn't have a scheme associated with it, and just generate the url if it sees that its only hostname:port

Actual result:

NPE happens because in this part of the code in ProxyUrlUtil#parse():

 String[] proxyUrlElements = proxyUrl.split("://");
 String schema = proxyUrlElements[0] + "://";
 String urlWithoutSchema = proxyUrlElements[1];

Will NPE when it tries to grab a value from proxyUrlElements[1]

It would be nice if slackConfig.setProxy() would work after the the SlackConfig is initialized. I think it works if the library detects no environment variables and proxyUrl is null. (At least this is what happens in v1.13.0 of the library, but because this version does not try to look at the HTTPS_PROXY and HTTP_PROXY environment variables)

@seratch seratch added enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client and removed untriaged labels Mar 23, 2023
@seratch seratch added this to the 1.28.1 milestone Mar 23, 2023
@seratch
Copy link
Member

seratch commented Mar 23, 2023

Hi @aaronq, thanks for reporting this. The parsing logic can be improved in future releases.

It would be nice if slackConfig.setProxy() would work after the the SlackConfig is initialized.

I am not sure if we can do this properly but will check.

@seratch
Copy link
Member

seratch commented Mar 27, 2023

I'll come up with a pull request for fixing the error with schema-less patterns and the change will be released in the next patch version shortly. However, the second point need careful consideration (consistency, race condition etc.) and efforts. So, I've created a new issue for it: #1132

@seratch seratch self-assigned this Mar 27, 2023
@seratch seratch changed the title Assumed HTTPS_PROXY and HTTP_PROXY values incorrect and causing NullPointerException error Seting schema-less URL for proxy URLs results in NullPointerException Mar 27, 2023
@seratch seratch changed the title Seting schema-less URL for proxy URLs results in NullPointerException Setting schema-less URL for proxy URLs results in NullPointerException Mar 27, 2023
seratch added a commit to seratch/java-slack-sdk that referenced this issue Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality project:slack-api-client project:slack-api-client
Projects
None yet
2 participants