proxy: Return an error for invalid proxy URLs instead of crashing. #6597
Conversation
Refactor url parsing to simplify the state-passing (introducing a struct) and add a path parser for future reusability.
Introduce a url parser that defaults to treating poorly specified URLs as http URLs. For example: `localhost:8080` is treated as `http://localhost:8080/` by the http-biased url parsing, instead of a URL with a scheme `localhost` and a path of `8080`..
The common format for specifying proxy URLs is just 'host:port'. Handle the common case.
Test proxies specified by both host:port format in configuration options, environment variables, and `http.proxy` configuration.
|
Yikes. Thanks for this - we should definitely not crash. However, that's the common format that proxies are usually specified in, in my experience. We shouldn't be so rigid as to require it to be a URL -- especially in the environment variable and configuration options. I added a few commits on top of this to relax parsing rules -- would you mind taking a look? https://github.com/libgit2/libgit2/tree/ethomson/proxy |
|
Thanks for going above and beyond here. Networking is a weak point for me so I'm asking the following questions based on code aesthetics and not from a position of knowledge:
|
Doh. It was not intentional, and I pushed an update, but I think that you beat me to it. Added in 38b60fd but I must have seen that after you pulled. 🙁
It does not - I haven't seen a SOCKS proxy in the wild in a very long time, nor has anybody asked (yet). |
I saw it in a .gitconfig once this year and heard nothing since... I'm not asking :D I've merged your work into this pull request. I can always merge this fix into our codebase first to give it some bake time, let me know. |
|
Cool. I'm going to merge this into main and tag this to backport into the v1.7 branch. If you have any feedback from having it in your codebase, that would be 💯 |
I see the following behaviour in 1.7.0 but not in 1.5.0.
If the user's Git config specifies the proxy like this then libgit2 now crashes:
The scheme (and port) are required but many users leave it off (curl defaults to http I read in the man pages).
In this pull request I propose checking the validity of the url after parsing it. Something similar is done in winhttp.c, so I replaced that with a call to git_net_url_valid but not sure if that's the right thing to do.