Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Proxy type kCFProxyTypeHTTPS expects to be HTTP proxy on macOS #48

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

sergio-nsk
Copy link
Collaborator

@sergio-nsk sergio-nsk commented Aug 9, 2023

Also fixes possible unsigned integer overflow max_len - list_len resulting in a huge buffer length passed to string functions,

@sergio-nsk sergio-nsk added the bug Something isn't working label Aug 9, 2023
resolver_mac.c Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #48 (91fe30d) into master (644e40f) will increase coverage by 0.06%.
The diff coverage is 47.36%.

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
+ Coverage   56.55%   56.61%   +0.06%     
==========================================
  Files          53       53              
  Lines        4601     4608       +7     
  Branches     1060     1063       +3     
==========================================
+ Hits         2602     2609       +7     
+ Misses       1313     1310       -3     
- Partials      686      689       +3     
Flag Coverage Δ
macos 50.35% <47.36%> (-0.23%) ⬇️
ubuntu 53.33% <ø> (ø)
ubuntu_curl 52.99% <ø> (+0.36%) ⬆️
windows 56.55% <ø> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
resolver_mac.c 58.78% <47.36%> (-4.34%) ⬇️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sergio-nsk sergio-nsk force-pushed the sergio_nsk/mac-https-proxy/1 branch 2 times, most recently from 518022e to f63d0d7 Compare August 9, 2023 01:10
else if (CFEqual(proxy_type, kCFProxyTypeHTTP))
scheme = "http://";
else if (CFEqual(proxy_type, kCFProxyTypeHTTPS))
// "HTTPS Proxy" on macOS means "use CONNENCT verb for a https:// URL", the proxy still should be an HTTP proxy.
Copy link
Owner

@nmoinvaz nmoinvaz Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this seems to suggest that macOS does not support HTTPS directive from PAC file. However, an HTTPS proxy can still probably be manually configured in settings dialog?

https://support.apple.com/guide/mac-help/change-proxy-settings-on-mac-mchlp2591/mac
image

Is that your take as well?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it would make sense it does not support HTTP/HTTPS directives because it uses multi-legacy.com in tests which I have commented in the pac file used in the test:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK, it is first time mentioned in macOS Montgomery, but we are targeting macOS 10.13. It may change the behavior if we change the target. If it still returns CFProxyTypeHTTPS for the PROXY directive, it is useless for us.

@nmoinvaz
Copy link
Owner

nmoinvaz commented Aug 9, 2023

Just looks like some lint test failing now, otherwise gtg.

@nmoinvaz
Copy link
Owner

nmoinvaz commented Aug 9, 2023

I will make a new proxyres release/tag after this is merged so you can update to it in the client.

@nmoinvaz nmoinvaz merged commit e3b3217 into master Aug 9, 2023
16 of 17 checks passed
@nmoinvaz nmoinvaz deleted the sergio_nsk/mac-https-proxy/1 branch August 9, 2023 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants