Support additional upstream proxy connection formats #185
Conversation
|
|
pimterry
left a comment
pimterry
left a comment
There was a problem hiding this comment.
Thanks for this! I wasn't aware of those proxy URL formats before but it makes sense, I'm happy to support that. I think there's a few tweaks we should make here but in general it looks like a good approach imo, thanks for taking the time to dig into this! 👍
| // Translates the proxy host input into standard form if needed | ||
| const normalizeProxyHost = (host: string): string => { | ||
| const idx = PROXY_HOST_REGEXES.findIndex(regex => regex.test(host)); | ||
| if (idx === -1) return host; // Should not occur |
There was a problem hiding this comment.
Given that it shouldn't happen, let's throw an error instead.
| default: // Does not include auth or already in standard format | ||
| return host; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I think we should test this, and probably pull it out of the pure UI code too. Can you please:
- Move all this proxy logic into the model, e.g. src/model/http/proxy.ts. I think
validateProxyHostshould stay here (it's linked to the UI input logic) but the rest should be imported. - Create a unit test in test/unit/model/http/proxy.spec.ts that covers each of these different URL format cases, and a few invalid examples.
| // 3) host:port:user:pass | ||
| new RegExp(`^(${HOST_PATTERN}):(${PORT_PATTERN}):(${CRED_PATTERN}):(${CRED_PATTERN})$`), | ||
| // 4) user:pass:host:port | ||
| new RegExp(`^(${CRED_PATTERN}):(${CRED_PATTERN}):(${HOST_PATTERN}):(${PORT_PATTERN})$`), |
There was a problem hiding this comment.
Apparently user@host:port also exists (implicitly empty password) and isn't that rare. I'm pretty sure that works now so it would be good not to break it.
Technically user:@..., :password@... and even :@... are valid as well but I think we can probably ignore those as too weird to appear in practice, and just wait & see if anybody complains.
There was a problem hiding this comment.
Updated the logic in #186 to support this. I also ended up allowing an optional port user:pass@host (and user@host) as the previous regex allowed this.
|
Ran into some rebasing issues, created a new CR: #186 |
3 participants
This request addresses request #184 that I raised to supporting additional proxy formats. I tested my changes using my own proxy. The cases I tested are:
proxy.host.comproxy.host.com:1234user_12345,type_residential,session_123:pass@proxy.host.com:1080proxy.host.com:1080:user_12345,type_residential,session_123:passproxy.host.com:1080@user_12345,type_residential,session_123:passuser_12345,type_residential,session_123:pass:proxy.host.com:1080user_12345,type_residential,session_123:pass:proxy.host.com:1080:extra(invalid)user_12345,type_residential,session_123:pass:https://proxy.host.com:1080(invalid)Proxies in the format of 4-6 get saved to the
rulesStoreas format 3. Note that the username contains,and_.