- Notifications
You must be signed in to change notification settings - Fork2.5k
proxy: Return an error for invalid proxy URLs instead of crashing.#6597
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Refactor url parsing to simplify the state-passing (introducing astruct) and add a path parser for future reusability.
Introduce a url parser that defaults to treating poorly specified URLsas http URLs. For example: `localhost:8080` is treated as`http://localhost:8080/` by the http-biased url parsing, instead of aURL with a scheme `localhost` and a path of `8080`..
The common format for specifying proxy URLs is just 'host:port'. Handlethe common case.
Test proxies specified by both host:port format in configurationoptions, environment variables, and `http.proxy` configuration.
Yikes. Thanks for this - we should definitelynot 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 in38b60fd but I must have seen that after you pulled. 🙁
It does not - I haven't seen a SOCKS proxy in the wild in avery 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 💯 |
Uh oh!
There was an error while loading.Please reload this page.
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.