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

curl: allow --header and --proxy-header read from file #1486

Closed
wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented May 15, 2017

So many headers can be provided as @filename.

Suggested-by: Timothe Litt

@mention-bot
Copy link

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @captain-caveman2k and @kdudka to be potential reviewers.

@bagder
Copy link
Member Author

bagder commented May 15, 2017

Oops, strtok_r isn't universally available. Will post an update for this soon.

@jay
Copy link
Member

jay commented May 15, 2017

Ref: https://curl.haxx.se/mail/archive-2017-05/0029.html

I don't know what he's talking about with the securing curlrc, if he uses --config foo it's still going to read the default curlrc. I will follow up on that on the list. So a change for this isn't necessary. Also this is breaking due to lack of strtok_r in Windows CI. strtok.h is only in the library so that would need to be changed or you could just use strtok and add a comment overidding checksrc and saying allow strtok since not threaded

@bagder
Copy link
Member Author

bagder commented May 15, 2017

a change for this isn't necessary

It isn't necessary for only that reason, no. But this is not the first time I've seen this request and I think it fits in how curl works already to have us consider supporting it.

@tlhackque
Copy link

tlhackque commented May 15, 2017

Thanks for the quick work. Sorry I missed that --config doesn't supersede the user's .curlrc ; I didn't connect --disable with --config. It's not intuitive behavior. I'm not a heavy curl user, but I did read the man page. It might be helpful if it were organized with related options together instead of alphabetically...

One comment on the code - you do an fopen with mode 'rb'. 'b' is binary (on systems that care). This is text input. So you probably want 'r'... you can't count on in the file for building the header; it might be there; might not; might be before the or after. Or you might get record length bytes... or untranslated wide chars.

Thanks again.
P.s. that also fixes the strtok; you'll always get '\n' on a sane text file, so you can just use strchr to find it, and always emit \015\012 for the network.

@bagder
Copy link
Member Author

bagder commented May 16, 2017

that also fixes the strtok; you'll always get '\n' on a sane text file

I don't see that as a safe bet at all. If you would save regular incoming received HTTP headers in a file, they are CRLF separated, and if you then use such a file as input to this on a *nix... I think it is safest to check for both. Updated version coming up...

So many headers can be provided as @filename.

Suggested-by: Timothe Litt
@bagder
Copy link
Member Author

bagder commented May 16, 2017

I missed that --config doesn't supersede the user's .curlrc ; I didn't connect --disable with --config. It's not intuitive behavior.

I'm not sure I agree. Why would --config disable the reading of .curlrc ? I don't think the option name implies that and the man page doesn't say so either. In fact, the man page expressly say that the default is read unless you use --disable...

I edited the wording for --config slightly in the man page now to perhaps make it clearer.

It might be helpful if it were organized with related options together instead of alphabetically

We tried that in the past and it turns out really hard and more confusing than this. Most options are related to at least a few others so in which order should they come, really?

@bagder bagder added the feature-window A merge of this requires an open feature window label May 16, 2017
@tlhackque
Copy link

I don't see that as a safe bet at all. If you would save regular incoming received HTTP headers in a file, they are CRLF separated, and if you then use such a file as input to this on a *nix... I think it is safest to check for both.

Well, that's a question of where the bug is. Both the network and filesystems have defined rules, and curl is supposed to translate.

I'd argue that whomever is saving headers to a text file should be removing the network-required CR,LF and writing to a text file with \n. That's portable. \n is defined to be "whatever the target filesystem wants"; CRLF; CR; LFCR; LF; or even (control-word, data). Headers are network ASCII.

Of course, the file should have been opened with "w", not "wb". Then the C RTL won't add CR when it's written. Except on a CRLF file system. Opening that file with 'r' will always yield \n -- even on a CRLF system, where the C RTL will remove the CR. (And \n isn't always 012...)

When you play by the rules, what's written will always be correct for native utilities - e.g. diff, your favorite editor, etc. dos2unix shouldn't be required to compare 2 files...

You can be tolerant of stray on input from files - but definitely should not be reading and writing text files with 'b' and '\n'. You must of course, deal with network ASCII by it's rules - emit , be tolerant of bare , and use \012 for LF - not '\n'. (On some systems, \n is 015 - and odder things. )

In any case, I appreciate your work. It's rare to see such a quick response.

@bagder bagder closed this in 84b9458 Jun 15, 2017
@bagder bagder deleted the bagder/curl-H-file branch June 15, 2017 09:10
@dfandrich
Copy link
Contributor

Test 1147 that tests this is failing the torture tests: https://curl.haxx.se/dev/log.cgi?id=20170625054623-24701#prob1154

jay added a commit that referenced this pull request Jun 26, 2017
@jay
Copy link
Member

jay commented Jun 26, 2017

Test 1147 that tests this is failing the torture tests

Fixed in 922f800, thanks.

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018
@bagder bagder removed the feature-window A merge of this requires an open feature window label Sep 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants