Skip to content

http: accept https:// proxies again - #2161

Closed
dscho wants to merge 1 commit into
gitgitgadget:aw/validate-proxy-url-schemefrom
dscho:fix-bug-in-validate-proxy-url-scheme
Closed

http: accept https:// proxies again#2161
dscho wants to merge 1 commit into
gitgitgadget:aw/validate-proxy-url-schemefrom
dscho:fix-bug-in-validate-proxy-url-scheme

Conversation

@dscho

@dscho dscho commented Jun 27, 2026

Copy link
Copy Markdown
Member

Cc: Aliwoto aminnimaj@gmail.com

Since 663d7ab (http: reject unsupported proxy URL schemes,
2026-05-05), set_curl_proxy_type() returns 0 only for the "http"
and SOCKS variants via dedicated early returns, and -1 for
everything else. The "https" branch configures the CURL handle for
HTTPS proxying but then falls through to the trailing `return -1`
intended for unknown schemes, so the caller in get_curl_handle()
treats a perfectly valid https:// proxy URL as unsupported and
refuses to use it.

Noticed while looking into a Coverity report against the same
function; the unchecked curl_easy_setopt() return values it flags
are orthogonal to this fix.

Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho self-assigned this Jun 27, 2026
@dscho

dscho commented Jun 27, 2026

Copy link
Copy Markdown
Member Author

/submit

@gitgitgadget

gitgitgadget Bot commented Jun 27, 2026

Copy link
Copy Markdown

Submitted as pull.2161.git.1782580676734.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2161/dscho/fix-bug-in-validate-proxy-url-scheme-v1

To fetch this version to local tag pr-2161/dscho/fix-bug-in-validate-proxy-url-scheme-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2161/dscho/fix-bug-in-validate-proxy-url-scheme-v1

@gitgitgadget

gitgitgadget Bot commented Jun 29, 2026

Copy link
Copy Markdown

This branch is now known as js/http-https-proxy-fix.

@gitgitgadget

gitgitgadget Bot commented Jun 29, 2026

Copy link
Copy Markdown

This patch series was integrated into seen via git@1274757.

@gitgitgadget

gitgitgadget Bot commented Jun 29, 2026

Copy link
Copy Markdown

This patch series was integrated into master via git@1274757.

@gitgitgadget

gitgitgadget Bot commented Jun 29, 2026

Copy link
Copy Markdown

This patch series was integrated into next via git@1274757.

@gitgitgadget

gitgitgadget Bot commented Jun 29, 2026

Copy link
Copy Markdown

Congratulations! 🎉 Your patch series was merged into upstream via 1274757.

Note: this pull request will show as "Closed" rather than "Merged" because the merge happened in the upstream repository, not on GitHub. This is expected — your contribution has been accepted!

@dscho
dscho deleted the fix-bug-in-validate-proxy-url-scheme branch June 30, 2026 11:49
@gitgitgadget

gitgitgadget Bot commented Jul 6, 2026

Copy link
Copy Markdown

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

>  http.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/http.c b/http.c
> index 8e5a4d8bcf..8c0f831365 100644
> --- a/http.c
> +++ b/http.c
> @@ -802,6 +802,8 @@ static int set_curl_proxy_type(CURL *result, const char *protocol)
>  		if (has_proxy_cert_password())
>  			curl_easy_setopt(result, CURLOPT_PROXY_KEYPASSWD,
>  					 proxy_cert_auth.password);
> +
> +		return 0;
>  	}
>  
>  	return -1;

That lack of "return 0" is so glaringly obvious when you point it
out like this patch does, and it is surprising it has been missed
initially.

From this function nothing returns an error anymore, and looking at
the preimage of 663d7abe (http: reject unsupported proxy URL
schemes, 2026-05-05) that is the source of the bug, the original did
not do anything when the corresponding code did not find and set any
proxy settings, either.

So perhaps it is a better fix to make it just a function that
returns void with early returns?

Thanks.

@gitgitgadget

gitgitgadget Bot commented Jul 6, 2026

Copy link
Copy Markdown

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Junio C Hamano <gitster@pobox.com> writes:

> From this function nothing returns an error anymore, and looking at
> the preimage of 663d7abe (http: reject unsupported proxy URL
> schemes, 2026-05-05) that is the source of the bug, the original did
> not do anything when the corresponding code did not find and set any
> proxy settings, either.
>
> So perhaps it is a better fix to make it just a function that
> returns void with early returns?

Nah, I was being stupid.  Disregard the above.

The whole point of 663d7abe was that we wanted to reject what we did
not recognise, and we cannot do so without returning "good/bad" from
that function.  The bug was that we did recognise https:// but still
returned -1 because of the bug, which the patch in the thread fixed.

Thanks.

@gitgitgadget

gitgitgadget Bot commented Jul 6, 2026

Copy link
Copy Markdown

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> From this function nothing returns an error anymore, and looking at
>> the preimage of 663d7abe (http: reject unsupported proxy URL
>> schemes, 2026-05-05) that is the source of the bug, the original did
>> not do anything when the corresponding code did not find and set any
>> proxy settings, either.
>>
>> So perhaps it is a better fix to make it just a function that
>> returns void with early returns?
>
> Nah, I was being stupid.  Disregard the above.
>
> The whole point of 663d7abe was that we wanted to reject what we did
> not recognise, and we cannot do so without returning "good/bad" from
> that function.  The bug was that we did recognise https:// but still
> returned -1 because of the bug, which the patch in the thread fixed.

And as an important bugfix, this patch of course has been
fast-tracked.  I'll make sure we have it in 'master' before Git 2.55
gets tagged.

Thanks.

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.

1 participant