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

tool: fix --capath when proxy support is disabled #12089

Closed
wants to merge 1 commit into from

Conversation

hwti
Copy link
Contributor

@hwti hwti commented Oct 11, 2023

After 95e8515, --capath always sets CURLOPT_PROXY_CAPATH, which fails with CURLE_UNKNOWN_OPTION when proxy support is disabled.

@bagder
Copy link
Member

bagder commented Oct 12, 2023

We try hard to not move CURL_DISABLE_ checks into the tool code as the tool and library should ideally remain as independent as possible. I think a better fix would be to let the tool handle the run-time error correctly, probably in the same way it already handles CURLE_NOT_BUILT_IN. What do you think about that?

@hwti
Copy link
Contributor Author

hwti commented Oct 12, 2023

So something like ?

          else if(result == CURLE_UNKNOWN_OPTION && !config->proxy_capath) {
            /* If proxy support is disabled, and we only have --capath, ignore the error */
          }

I wonder CURLE_UNKNOWN_OPTION is the correct error code.
It seems to be used for several proxy configs, since #ifndef CURL_DISABLE_PROXY flag many case CURLOPT_PROXY_*:.
But when SSL support is disabled, or the backend does not support CAPATH, setting CURLOPT_PROXY_CAPATH returns CURLE_NOT_BUILT_IN.

@bagder
Copy link
Member

bagder commented Oct 16, 2023

I was thinking like the patch below.

The *setopt() code returns CURLE_NOT_BUILT_IN for a lot of cases where it explicitly knows that the support was disabled from the build, but it also returns CURLE_UNKNOWN_OPTION for the cases when the entire option handling was removed. The latter return code will be returned for most/all CURLOPT_PROXY_* options when proxy support was disabled in the build. But also: if you would use an older libcurl without support for this option, before it was introduced, it would also return CURLE_UNKNOWN_OPTION for it.

So I think the safest and most sane approach is to check for both and treat them the same:

diff --git a/src/tool_operate.c b/src/tool_operate.c
index b60f57e0d..697b64e38 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -1774,11 +1774,12 @@ static CURLcode single_transfer(struct GlobalConfig *global,
        if(config->proxy_capath || config->capath) {
          result = res_setopt_str(curl, CURLOPT_PROXY_CAPATH,
                                  (config->proxy_capath ?
                                   config->proxy_capath :
                                   config->capath));
-          if(result == CURLE_NOT_BUILT_IN) {
+          if((result == CURLE_NOT_BUILT_IN) ||
+             (result == CURLE_UNKNOWN_OPTION)) {
            if(config->proxy_capath) {
              warnf(global,
                    "ignoring --proxy-capath, not supported by libcurl");
            }
          }

After 95e8515, --capath always sets CURLOPT_PROXY_CAPATH, which fails with
CURLE_UNKNOWN_OPTION when proxy support is disabled.
@hwti
Copy link
Contributor Author

hwti commented Oct 16, 2023

OK, this mean that using --proxy-capath when proxy support is disabled would only be a warning.
But since --proxy is still error, it's fine (no risk to send a direct request when the user wanted to use a proxy).

@bagder bagder closed this in 014ce7c Oct 21, 2023
@bagder
Copy link
Member

bagder commented Oct 21, 2023

Thanks!

@hwti hwti deleted the fix-capath-without-proxy branch October 23, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants