GNOME Bugzilla – Bug 733876
Proxy protocol 'http' is not supported.
Last modified: 2018-04-12 02:06:28 UTC
When trying to access IMAP (over SSL) servers (i.e. gmail.com) or NNTP servers outside of my network -- which requires using the CONNECT method of an HTTP proxy server, I am getting the error: Proxy protocol 'http' is not supported. This is clearly GIO/libproxy/etc. all at work. I was even getting the above errors even for IMAP access until I specified the IMAP and SMTP server names such that libproxy could figure out they were local and used "direct://" rather than the HTTP proxy. But the question of how to use GIO to make CONNECT based calls through a proxy to reach remote IMAP and NNTP servers. Maybe this is not an evolution problem at all. I'm not sure. If not, I'd be happy to have this redirected to the correct parties/projects.
glib-networking provides HTTP proxy support. Check that you have it installed.
Yes, it's installed: $ rpm -q glib-networking glib-networking-2.40.1-1.fc20.x86_64
Given that I have glib-networking installed and the problem is still there, what next?
So given that I have glib-networking installed, can we confirm that this is a bug in evolution now? A pretty serious regression IMHO given that 3.10.x worked in my environment just fine and 3.12.x completely fails to connect to external (to my network) servers? This will affect anyone who has a proxy server configured to reach outside their network.
I have the same problem on Fedora 20. For some reason I decided to try Gnome 3.12. In order to use Gnome 3.12, I enabled repository "Copr repo for f20-gnome-3-12 owned by rhughes (x86_64)" (http://copr-be.cloud.fedoraproject.org/results/rhughes/f20-gnome-3-12/fedora-$releasever-x86_64/) and run "yum update". Everything looks ok, but Evolution now cannot receive any messages. BTW, I am using POP protocol, not IMAP. glib-networking is installed.
Thanks for a bug report. The e-mail related (Camel) connections are using SOCKS proxy. When that is configured, then that is used. I do not think you need to specify the proxy host as http://proxy.server.com, rather use only the server host name, like proxy.server.com. Either set this in the system settings, or in Edit->Preferences->Network Preferences->.... There was one issue with the settings read/write, as reported at bug #737381. Maybe if you do not face just this.
Right. What I am suggesting is that connections should be able to use an HTTP proxy also using the CONNECT method. Contrary to popular opinions, this is not necessarily insecure. An administrator of such a proxy does not have to allow CONNECT methods to any and all hosts on any and all ports. An administrator of such a system can very narrowly target (i.e. whitelist) to where CONNECT requests are allowed to be made.
I do not know what you mean with the "CONNECT method". Maybe we are on the same line, maybe not. The proxy settings has set multiple protocols, currently: HTTP Proxy HTTPS Proxy Socks Proxy The first proxy is used for http:// connections, the second for https:// and finally the third (socks) is user for mail (in evolution terms 'Camel') connections. The proxy is used as entered by the user, as far as I know. If not, then it should be fixed in GLib (evolution uses GProxyResolver from there). Do not expect to have used HTTP proxy settings for HTTPS or Socks proxy on its own, if you want it to be used, then fill it in Preferences as such.
(In reply to comment #8) > I do not know what you mean with the "CONNECT method". HTTP[S] proxies such as squid support a "CONNECT" method for tunneling through the proxy server. Traditionally this is used to connect clients to SSL web-servers on port 443. The client connects to the proxy server and issues a command such as: CONNECT www.gnome.org:443 after the success of which the client is directly tunneled to www.gnome.org on port 443. http://en.wikipedia.org/wiki/HTTP_tunnel#HTTP_CONNECT_tunneling Of course the port doesn't have to be 443. It can be whatever the admin wants to allow. > Maybe we are on the same > line, maybe not. The proxy settings has set multiple protocols, currently: > HTTP Proxy > HTTPS Proxy > Socks Proxy Yes, for any/all that might be available. > The first proxy is used for http:// connections, the second for https:// In fact per the above, the second can be used for any connections allowed by the proxy server. > and > finally the third (socks) is user for mail (in evolution terms 'Camel') > connections. But the point is that this does not have to be the case. An IMAP or NNTP connection can go through an HTTP[S] proxy using CONNECT per the above.
Thanks for the clarification. As Evolution doesn't have any influence on this anymore, I'm moving this to glib (it's GProxyResolver's responsibility now).
Funny there should be some activity on this in the last few hours. I spent a bit of time last night looking into this. I didn't really get very far but did get to the point of understanding (I think, please correct me if I am wrong) that all of the proxy decisions (yes/no/how) are being offloaded to libproxy now, is that correct? I have traditionally used the typical http_proxy/HTTPS_PROXY/no_proxy environment variables for my proxy configuration but was thinking last night that if libproxy is going to be used anyway I may as well go all in and use the proxy PAC that I point my browsers at. libproxy didn't seem to do so well with that. Even with the simplest PAC file, pointed at with http_proxy=pac+<url_to_pac_file>, set to _ALWAYS_ return a proxy I kept getting direct:// when testing with the "proxy" command. I could see that libproxy was fetching that PAC file every time but it still seemed to be (effectively) ignoring it's content. I sent a message to the libproxy list but I think it's being held for moderation. What was additionally concerning though, while I was looking at that list was this message: https://groups.google.com/forum/#!topic/libproxy/L_pttoc6Sjg which, TL;DR is that libproxy is effectively not being maintained. The current maintainer has lost interest. :-(
(In reply to comment #11) > all of the proxy decisions (yes/no/how) are being offloaded to libproxy > now, is that correct? Not entirely. glib-networking contains two GProxyResolver implementations, one that uses libproxy for everything, and another that reads directly from GSettings (ie, what the GNOME Control Center writes to), and only uses libproxy to do WPAD/PAC. If both are installed, the GNOME-specific one has higher priority. > I have traditionally used the typical http_proxy/HTTPS_PROXY/no_proxy > environment variables for my proxy configuration but was thinking last night > that if libproxy is going to be used anyway I may as well go all in and use the > proxy PAC that I point my browsers at. FWIW, if your configuration isn't dynamic, that's kind of a lose; PAC answers are inherently un-cacheable (since you might be basing your answer on the time of day, or randomly load-balancing between multiple servers), so it has to run your PAC file again for every single connection attempt. > libproxy didn't seem to do so well with that. Even with the simplest PAC file, > pointed at with http_proxy=pac+<url_to_pac_file>, set to _ALWAYS_ return a > proxy I kept getting direct:// when testing with the "proxy" command. I could > see that libproxy was fetching that PAC file every time but it still seemed to > be (effectively) ignoring it's content. Syntax error in your PAC file maybe? > which, TL;DR is that libproxy is effectively not being maintained. Yeah... there's a bug open about implementing PAC ourselves (bug 659857).
Points taken on PAC and dynamicness. So the problem I had with PAC resolution turned out to be that while I had the libproxy-pacrunner package installed I had not installed a JS engine such as mozjs or webkit. Seems like a small deficiency in packaging there to not require one or the other but that's a digression that needs to go downstream. So back to the original point of this ticket... Proxy protocol 'http' is not supported. What is this telling me exactly? It's a result of evolution trying to make a connection to an NNTP server in a proxied enviroment. A small test program that I wrote to simply do a: g_proxy_resolver_lookup() tells me "http://proxy:3128/" for a proxy resolution of "nntp://news.gmane.org/". So why "Proxy protocol 'http' is not supported." for that? The squid proxy at port 3128 is the way to get there. It is configured to allow that.
(In reply to comment #13) > So why "Proxy protocol 'http' is not supported." for that? The squid proxy at > port 3128 is the way to get there. It is configured to allow that. It means that GLib has figured out that you want to use an http proxy, but it doesn't know how to do that; the only proxy type that GLib supports natively is SOCKS. To fix this, we'd need a new GProxy implementation, similar to gio/gsocks4proxy.c, gsocks4aproxy.c, and gsocks5proxy.c, but implementing http CONNECT rather than SOCKS. This would most easily be done by adding the new GProxy to glib-networking, where we would be able to depend on libsoup to do the http bits (including authentication).
For the records, spice-gtk has a http/https proxy support based on Wocky (and I am not entirely convinced using libsoup for it is for the best, given how limited the scope is) http://cgit.freedesktop.org/spice/spice-gtk/tree/gtk/wocky-http-proxy.c
(In reply to comment #15) > For the records, spice-gtk has a http/https proxy support based on Wocky (and I > am not entirely convinced using libsoup for it is for the best, given how > limited the scope is) > > http://cgit.freedesktop.org/spice/spice-gtk/tree/gtk/wocky-http-proxy.c Oh nice. Any reason not to push this upstream into glib (he asks after very quickly skimming it)?
(In reply to comment #16) > (In reply to comment #15) > > For the records, spice-gtk has a http/https proxy support based on Wocky (and I > > am not entirely convinced using libsoup for it is for the best, given how > > limited the scope is) > > > > http://cgit.freedesktop.org/spice/spice-gtk/tree/gtk/wocky-http-proxy.c > > Oh nice. Any reason not to push this upstream into glib (he asks after very > quickly skimming it)? I remember asking the same question to Nicolas, and he said it would need a more thorough implementation and testing. It's relatively hard to test this against many proxy. I know it to work with apache, squid, and some hand made python http proxy,etc also there should probably be other authentication method implemented. Perhaps this is something libsoup already provides?
(In reply to comment #17) > > I remember asking the same question to Nicolas, and he said it would need a > more thorough implementation and testing. It's relatively hard to test this > against many proxy. I know it to work with apache, squid, and some hand made > python http proxy,etc I would tend towards calling that the perfect being the enemy of the good. Currently there is zero support for any http proxy at all. Even some would be better than none and getting it in there would give it the wider testing that it (only) may need. > also there should probably be other authentication method implemented. But again, let's not let the perfect beat out the good. > Perhaps > this is something libsoup already provides? Perhaps. Actually I'd be surprised if it's not. But again, something is better than nothing until somebody gets to replacing it with libsoup workings. IMHO. :-)
(In reply to comment #17) > also there should probably be other authentication method implemented. Perhaps > this is something libsoup already provides? I doubt anyone uses Digest auth with proxies, but there are people who need NTLM auth. But yeah, this could come later. (The other argument for using libsoup would be that it would give you better compatibility with lame broken servers. But there are way fewer proxy implementations than HTTP server implementations, so you're less likely to encounter a broken one, so that's not likely to matter too much.)
Created attachment 292307 [details] [review] http proxy implementation with WockyProxy I've dropped the previously mentioned WockyProxy work into gio, and it works! :-) Yay!
So one odd side effect of having a GProxy implementation capable of using an http proxy is that evolution-rss is now trying to open CONNECT operations on port 80 instead of using GET, etc. And sometimes, it seems to mangling the host names that it's trying to connect to: TCP_DENIED/407 3761 CONNECT resrcs.feedsportal.com.s3.amazonaws.com:80 - HIER_NONE/- text/html TCP_DENIED/407 3693 CONNECT pi.feedsportal.comhttp:80 - HIER_NONE/- text/html evolution-rss is also still using the deprecated EProxy stuff. I guess it really needs to be brought up to current glib standards. :-( I wish I was more familiar with all of this glib/gio/soup/etc. stuff.
(In reply to comment #21) > So one odd side effect of having a GProxy implementation capable of using an > http proxy is that evolution-rss is now trying to open CONNECT operations on > port 80 instead of using GET, etc. Ugh... I looked at the evolution-rss proxy code and then immediately regretted it :-} (At a minimum, someone should rip out support for libsoup-2.2 [the last release of which was 7 years ago], require at least libsoup-2.4 >= 2.34 [which shipped with GNOME 3.0, 3.5 years ago], rip out all references to libsoup-gnome, and use SoupProxyResolverDefault unconditionally.) Anyway, it looks like it's GSocketClient's fault. Try the attached patch.
Created attachment 292381 [details] [review] GSocketClient: fix handling of application proxies g_socket_client_add_application_proxy() claimed "When the indicated proxy protocol is returned by the #GProxyResolver, #GSocketClient will consider this protocol as supported but will not try to find a #GProxy instance to handle handshaking." But in fact, it did the checks in the wrong order, so GProxy proxies ended up overriding application-specified ones. Fix that. Also, simplify the code a bit by making use of g_hash_table_add() and g_hash_table_contains().
(In reply to comment #22) > > Ugh... I looked at the evolution-rss proxy code and then immediately regretted > it :-} I know, right? > Anyway, it looks like it's GSocketClient's fault. Try the attached patch. That works a treat! Thanks much!
Comment on attachment 292307 [details] [review] http proxy implementation with WockyProxy >+ gwockyproxy.h \ of course this should be renamed to ghttpproxy/GHttpProxy >+//#include "glib-compat.h" drop that... >+G_DEFINE_TYPE_WITH_CODE (WockyHttpProxy, wocky_http_proxy, G_TYPE_OBJECT, >+ G_IMPLEMENT_INTERFACE (G_TYPE_PROXY, >+ wocky_http_proxy_iface_init) >+ g_string_append_printf (request, >+ "CONNECT %s:%i HTTP/1.0\r\n" >+ "Host: %s:%i\r\n" The indentation of continuation lines is wrong throughout the file. Eg, "G_IMPLEMENT_INTERFACE" should be lined up with "WockyHttpProxy", "wocky_http_proxy_iface_init" should be lined up with "G_TYPE_PROXY", and '"CONNECT...' and '"Host...' should both be lined up with 'request'. >+ "Bad HTTP proxy reply"); Strings that might end up being displayed to the user should be marked for translation. (And don't forget to add this file to po/POTFILES.in.) >+ while (*ptr == ' ') ptr++; split that onto two lines >+ if (err_code < 200 || err_code >= 300) >+ { >+ const gchar *msg_start; Don't try to parse the "reason phrase" out of the response. It's not likely to be terribly useful (and it's guaranteed not to be localized). Just look at the err_code, and return a suitable (localized) error message based on that: 403 - can't connect to that host/port 407 - auth needed 5xx - proxy is experiencing technical difficulties? anything else - it's probably not actually a proxy server? >+ tlsconn = g_tls_client_connection_new (io_stream, >+ G_SOCKET_CONNECTABLE(proxy_address), (nitpick: space before the open paren) >+ GTlsCertificateFlags tls_validation_flags = G_TLS_CERTIFICATE_VALIDATE_ALL; ... >+#ifdef DEBUG >+ tls_validation_flags &= ~(G_TLS_CERTIFICATE_UNKNOWN_CA | G_TLS_CERTIFICATE_BAD_IDENTITY); >+#endif >+ g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (tlsconn), >+ tls_validation_flags); "ALL" is the default, so you can just put all of that code inside the #ifdef; you don't need to do anything in the normal case. (Likewise in the async code.) >+ data_in = g_data_input_stream_new (in); ... >+ buffer = g_data_input_stream_read_until (data_in, HTTP_END_MARKER, NULL, >+ cancellable, error); >+ g_object_unref (data_in); ... >+ return io_stream; GDataInputStream buffers the underlying stream, so it's possible here that it will read beyond the HTTP_END_MARKER, getting part of the tunnelled communication as well, and that data would get lost when you unref data_in and return the underlying io_stream instead. (The code will work in protocols where the client always speaks before the server does, and it will *usually* work even if the server speaks first, since there will most likely be a delay between the proxy's 200 response and the first tunnelled packet from the server. But if things are slow, then those two packets might end up getting coalesced, and returned in a single read() call, and then things would break.) So don't use GDataInputStream. Just read and buffer the response byte-by-byte, and stop when you reach HTTP_END_MARKER. (The SOCKS proxy has an easier time with this since SOCKS uses fixed-size protocol elements...) >+ simple = g_simple_async_result_new (G_OBJECT (proxy), >+ callback, user_data, >+ wocky_http_proxy_connect_async); This should really be updated to use GTask instead...
Created attachment 298159 [details] [review] add ghttpproxy This patch adds ghttpproxy based on the wockyproxy code. This patch is exactly the same code as the one contributed above but with the class renamed to ghttpproxy as suggested above. There are no logic changes, only the name, the codestyle and cosmetic issues, the usage of the right includes from inside glib and the use of gettext for translated messages. The following 3 items in Dan's review are still valid: 1) do not bother parsing the server error message 2) port to GTask 3) do not use GDataInput stream They will be fixed in follow up patches to make review easier
Created attachment 298160 [details] [review] port to gtask
Created attachment 298161 [details] [review] do not parse error string This patch removes the parsing of the http response as requested in the review. I did not treat 5xx code in a specific way because on http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html I did not see any 5xx code specific to proxies
Created attachment 298162 [details] [review] do not parse error string
Review of attachment 298159 [details] [review]: See the comments. ::: gio/ghttpproxy.c @@ +1,1 @@ + /* GIO - GLib Input, Output and Streaming Library wrong align @@ +59,3 @@ + G_IMPLEMENT_INTERFACE (G_TYPE_PROXY, + g_http_proxy_iface_init) + _g_io_modules_ensure_extension_points_registered (); the socks extension is doing something different here, not sure if worth checking @@ +73,3 @@ + +static gchar * +create_request (GProxyAddress *proxy_address, gboolean *has_cred) each param in one line @@ +541,3 @@ + +#define g_https_proxy_get_type _g_https_proxy_get_type +G_DEFINE_TYPE_WITH_CODE (GHttpsProxy, g_https_proxy, G_TYPE_HTTP_PROXY, I am not sure we want https and http in the same file, I'll let dan to decide that ::: gio/ghttpproxy.h @@ +1,1 @@ + /* GIO - GLib Input, Output and Streaming Library wrong align @@ +38,3 @@ +GType _g_http_proxy_get_type (void); + +#if GLIB_CHECK_VERSION(2, 28, 0) we do not want this glib check
Created attachment 298164 [details] [review] do not use gdatainputstream read byte-by-byte instead of using data input stream
Review of attachment 298160 [details] [review]: See comment, also it is not easy to review this from the diff ::: gio/ghttpproxy.c @@ +471,2 @@ return; } do an: if (data->buffer == NULL || !check_reply) return_error; else return_pointer unref_task() ?
Review of attachment 298162 [details] [review]: See the comments ::: gio/ghttpproxy.c @@ +151,3 @@ { + case 403: + g_set_error (error, G_IO_ERROR, G_IO_ERROR_PROXY_NOT_ALLOWED, literal @@ +156,3 @@ + case 407: + if (has_cred) + g_set_error (error, G_IO_ERROR, G_IO_ERROR_PROXY_AUTH_FAILED, set_error_literal? @@ +159,3 @@ + _("HTTP proxy authentication failed")); + else + g_set_error (error, G_IO_ERROR, G_IO_ERROR_PROXY_NEED_AUTH, same here
Review of attachment 298164 [details] [review]: See comments ::: gio/ghttpproxy.c @@ +225,3 @@ + buffer = g_malloc (buffer_length); + + /* We read byte-by-byte a instead of using GDataInputStream typo? "a instead" @@ +426,3 @@ + in = g_io_stream_get_input_stream (data->io_stream); + + /* We read byte-by-byte a instead of using GDataInputStream ditto @@ +487,3 @@ } + g_input_stream_read_async (G_INPUT_STREAM (source), I think we could avoid duplicating this by calling reply_read_cb with a NULL result?
Thanks for the review. I'll attach an updated pacthset with all the requested changes except the following: (In reply to Ignacio Casal Quinteiro (nacho) from comment #30) > the socks extension is doing something different here, not sure if worth > checking > I did the same that is done in gsocks5proxy > > I am not sure we want https and http in the same file, I'll let dan to > decide that They need to be in in the same file since they use the same implementation (In reply to Ignacio Casal Quinteiro (nacho) from comment #33) > Review of attachment 298160 [details] [review] [review]: > do an: > if (data->buffer == NULL || !check_reply) > return_error; > else > return_pointer > > unref_task() ? I fixed this in the last patch of the patchset that changes this codepath (In reply to Ignacio Casal Quinteiro (nacho) from comment #35) > Review of attachment 298164 [details] [review] [review]: > I think we could avoid duplicating this by calling reply_read_cb with a NULL > result? This code is actually part of reply_read_cb :)
Created attachment 298165 [details] [review] add ghttpproxy
Created attachment 298166 [details] [review] port to gtask
Created attachment 298167 [details] [review] do not parse error string
Created attachment 298168 [details] [review] do not use gdatainputstream
Review of attachment 298165 [details] [review]: See comment ::: gio/ghttpproxy.h @@ +27,3 @@ + +#define G_TYPE_HTTP_PROXY (_g_http_proxy_get_type ()) +#define G_HTTP_PROXY(o) (G_TYPE_CHECK_INSTANCE_CAST ((o), G_TYPE_HTTP_PROXY, GHttpProxy)) should we use the G_DECLARE_FINAL in another patch?
I think you should squash the commits together before merging; just credit the original author in the commit message (In reply to Paolo Borelli from comment #28) > I did not treat 5xx code in a specific way because on > http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html I did not see any 5xx > code specific to proxies Proxies are just a kind of server, and can return any of the same 5xx responses that any other server can. But anyway, I guess just a generic message (which includes the error code) for everything except 403 and 407 is fine. >+ while (g_input_stream_read_all (in, buffer + bytes_read, 1, using g_input_stream_read_ALL() with count==1 is kinda silly... >+ if (bytes_read == 0) > { > if (error && (*error == NULL)) > g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_PROXY_FAILED, with the new code, it's not possible for error to be unset if bytes_read is 0. But also, it's possible that error *is* set even if bytes_read isn't 0. You need to redo the error handling a bit more. >+ bytes_read = g_input_stream_read_finish (G_INPUT_STREAM (source), >+ res, &error); ... >+ g_assert (bytes_read == 1); That assertion will fail if the server closed the connection (in which case you'll get a 0-length read). Doing a series of 1-byte async reads seems likely to be horrifically inefficient. It would probably be better to just use g_task_run_in_thread() to run the sync version in another thread.
(In reply to Dan Winship from comment #42) > I think you should squash the commits together before merging; just credit > the original author in the commit message > Sure, I split mostly to make review of the incremental changes easier. > > >+ while (g_input_stream_read_all (in, buffer + bytes_read, 1, > > using g_input_stream_read_ALL() with count==1 is kinda silly... yeah... I did it to make it more explicit, but no strong opinion. I'll change it. > > >+ if (bytes_read == 0) > > { > > if (error && (*error == NULL)) > > g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_PROXY_FAILED, > > with the new code, it's not possible for error to be unset if bytes_read is > 0. But also, it's possible that error *is* set even if bytes_read isn't 0. > You need to redo the error handling a bit more. > I'll double check. > >+ bytes_read = g_input_stream_read_finish (G_INPUT_STREAM (source), > >+ res, &error); > ... > >+ g_assert (bytes_read == 1); > > That assertion will fail if the server closed the connection (in which case > you'll get a 0-length read). > Right, sorry about that, that's something I considered and added that assert as a reminder and then forgot to point out... What should I do if I get 0? unref the task and return NULL? Set an error? > Doing a series of 1-byte async reads seems likely to be horrifically > inefficient. It would probably be better to just use g_task_run_in_thread() > to run the sync version in another thread. It is very inefficient, but it is also true that it is just a small text handshake. Anyway ok, I'll rework this to use g_task_run_in_thread
(In reply to Paolo Borelli from comment #43) > > That assertion will fail if the server closed the connection (in which case > > you'll get a 0-length read). > > > > Right, sorry about that, that's something I considered and added that assert > as a reminder and then forgot to point out... What should I do if I get 0? > unref the task and return NULL? Set an error? The proxy connection has failed, so you want to set an error. Specifically, the server closed the connection without even having returned a complete HTTP response... you could possibly still try to parse it with check_reply() anyway (since that will set an appropriate error even if the buffer doesn't contain an HTTP response), or else just set a "Proxy server closed connection" error or something like that.
Created attachment 298503 [details] [review] add ghttpproxy Here is the squashed patch with the reported issues fixed and using g_task_run_in_thread. Tested with our app and a squid instance
Comment on attachment 298503 [details] [review] add ghttpproxy I think it's too late for 2.44 at this point, but this can go in post-branch. >+ do { >+ } while (TRUE); but fix the indentation here: do { ... ... } while (TRUE);
(In reply to Dan Winship from comment #46) > I think it's too late for 2.44 at this point, but this can go in post-branch. It is late, but it does not add any API... it would be great to have it in the next stable without waiting a cycle and I am not sure that waiting gives us any extra testing given that using http proxy seems pretty unlikely in our normal testers (jhbuild users etc) > >+ do { > > >+ } while (TRUE); > > but fix the indentation here: > As you prefer, but I grepped other usages of do..while in glib since I was not sure how to indent them in GNU style and I found the indentation I used.
(In reply to Paolo Borelli from comment #47) > (In reply to Dan Winship from comment #46) > > I think it's too late for 2.44 at this point, but this can go in post-branch. > > It is late, but it does not add any API... it would be great to have it in > the next stable without waiting a cycle and I am not sure that waiting gives > us any extra testing given that using http proxy seems pretty unlikely in > our normal testers (jhbuild users etc) Well, let's see what other maintainers think. (In particular, I'm worried about the code causing problems in situations where it's not even supposed to be getting used, like with comment 21 above.) > As you prefer, but I grepped other usages of do..while in glib since I was > not sure how to indent them in GNU style and I found the indentation I used. Hm... it looks like both variants exist in the glib sources. But the multi-line version is correct GNU style (https://www.gnu.org/prep/standards/html_node/Formatting.html, bottom of the page)
I don't know much of the particulars here, but I'd be inclined to agree with paolo here. This seems like it is purely additive (although comment 21 clearly shows its not fully so) and its in a weird enough case that it is unlikely to get any more testing in the next cycle from what it already got in this bug. And it solves real problems for users.
I also see no problem with adding it now. I do have a slight concern, however, about this code growing over time as we encounter weird http servers that we need to add support for. Using soup would certainly have been nicer, but at 300 lines (as it is) it's hard to argue too much...
OK, let's get this in then. (In reply to Ryan Lortie (desrt) from comment #50) > I do have a slight concern, however, about this code growing over time as we > encounter weird http servers that we need to add support for. Using soup > would certainly have been nicer, but at 300 lines (as it is) it's hard to > argue too much... As I said above, "weird http servers" is probably much less of an issue with proxies, since there are many fewer proxy implementations (and probably many more quickly-hacked-together client-side CONNECT implementations, such that the broken-implementation-compatibility constraints probably run in the other direction here, with proxy servers being forced to deal with badly-written clients rather than the reverse). And anyway, we can replace/supplement this with a libsoup-based implementation in glib-networking later on if we decide we need to.
Comment on attachment 298503 [details] [review] add ghttpproxy I amended the patch to fix the style of the do..while and pushed to master. Thank you for your patience :) I am not closing the bug since there is the patch at comment #23 and it is not clear to me if that should be pushed as well. In the case of my application the http proxy worked without the need for that patch
oops, yes, that patch is needed. I thought I had committed it a long time ago. Attachment 292381 [details] pushed as 6ce79e5 - GSocketClient: fix handling of application proxies
*** Bug 748591 has been marked as a duplicate of this bug. ***
*** Bug 714853 has been marked as a duplicate of this bug. ***