Improvements to HTTP web proxy #742
Conversation
|
|
Bug: proxy connect packet is malformed due to a space after HTTP/1.1. Feature: Pass the host name so proxy host acls can be matched. Feature: Switch to the WebRequest.DefaultWebProxy which allows for overriding in app.settings file and falls back to the system proxy if not configured. Improvement: Use ordinal comparision for IndexOf on the proxy response.
IanLeeClaxton
force-pushed
the
feature/improve-http-proxy
branch
from
ac21007
to
7c7d2d9
Compare
|
Hi @IanLeeClaxton, is this still a "living" fix for you? If I rebase to latest, can you review and help me see it through to merge? |
|
Sure Grant, as far as I'm aware this is still being used at the company I wrote it for. Let me know what you need me to do |
|
@gbirchmeier, I fixed the merge conflicts and this branch is ready for you to test/merge. I recommend regression testing my changes again with a corporate class proxy and you could also automate testing using squid as part of your release pipeline. |
|
@IanLeeClaxton any chance you can tag or pass on a contact to someone at the company that's using it? If nothing else, I'd just like to let them know that this fix is going in. |
|
I have sent a DM to Vijay who works there and will ask him to confirm on this thread |
|
@gbirchmeier - please proceed with this request. Thank you @IanLeeClaxton |
…ies as requested.
… always set to avoid using the ternary operator
|
@gbirchmeier I have applied your recommendations to this PR |
|
This PR seems to have no problem. |
IanLeeClaxton
force-pushed
the
feature/improve-http-proxy
branch
3 times, most recently
from
95a786f
to
69f2b21
Compare
| @@ -50,7 +50,7 @@ public static class StreamFactory | |||
| Socket socketThruProxy = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); | |||
| socketThruProxy.Connect(proxyEndPoint); | |||
|
|
|||
| string proxyMsg = $"CONNECT {destIp}:{destPort} HTTP/1.1 \n\n"; | |||
| string proxyMsg = $"CONNECT {destHostName}:{destPort} HTTP/1.1\nHost: {destHostName}:{destPort}\n\n"; | |||
There was a problem hiding this comment.
You never did answer my original question: Will this new string work seamlessly with existing non-Squid users?
Why was it using destIp before if that wasn't really needed? Was that just the wrong way to do it?
The current http proxy implementation has a few issues that I encountered when trying to implement behind a Squid proxy so we were unable to use it. This update should resolve the issues we encountered
Bug: proxy connect packet is malformed due to a space after HTTP/1.1.
Feature: Pass the host name so proxy host acls can be matched.
Feature: Switch to the WebRequest.DefaultWebProxy which allows for overriding in app.settings file and falls back to the system proxy if not configured.
Improvement: Use ordinal comparision for IndexOf on the proxy response.