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

NSProtocol additional sanity check #318

Closed
wants to merge 2 commits into from

Conversation

onflapp
Copy link
Contributor

@onflapp onflapp commented Sep 1, 2023

Prevent setting headers if key or value are nil.
I've encountered an URL where headers are corrupted and this causes internal exception within NSURLConnection.

@onflapp onflapp requested a review from rfm as a code owner September 1, 2023 11:47
@rfm
Copy link
Contributor

rfm commented Sep 6, 2023

Isn't this change (stopping the low level code from raising an exception) just hiding the actual issue?
I would prefer to address the problem by making the higher level code check for it and refrain from setting bad header values.
Perhaps the header is merely unusual rather than corrupt, but the parsing code is failing in some way?
Can you provide a test case demonstrating the corrupt header (so we can see how that header is parsed/handled earlier on) ?

@onflapp
Copy link
Contributor Author

onflapp commented Sep 6, 2023

Isn't this change (stopping the low level code from raising an exception) just hiding the actual issue?
I would prefer to address the problem by making the higher level code check for it and refrain from setting bad header values.
Perhaps the header is merely unusual rather than corrupt, but the parsing code is failing in some way?
Can you provide a test case demonstrating the corrupt header (so we can see how that header is parsed/handled earlier on) ?

Yes this is possible, although I wasn't able to debug this.
I encountered this problem while downloading one of the OpenStep Previous images.
https://winworldpc.com/download/3fc5bdc2-bde2-80a2-533f-11c3a4c2a90f

@rfm
Copy link
Contributor

rfm commented Sep 6, 2023

I changed NSURLResponse.m to warn about bad (missing) header values. Perhaps that will help determine where they are coming from?

@rfm
Copy link
Contributor

rfm commented Apr 4, 2024

I made setting nil equivalent to setting an empty string as a header value (some servers seem to do that) and allowed it.

@rfm rfm closed this Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants