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

Jump to bottom

lib: sanitize conditional exclusion around MIME #9610

Closed
wants to merge 1 commit into from
Closed

lib: sanitize conditional exclusion around MIME #9610

wants to merge 1 commit into from

Conversation

monnerat
Copy link
Contributor

The introduction of CURL_DISABLE_MIME came with some additional bugs:

  • Disabled MIME is compiled-in anyway if SMTP and/or IMAP is enabled.
  • CURLOPT_MIMEPOST, CURLOPT_MIME_OPTIONS and CURLOPT_HTTPHEADER are conditioned on HTTP, although also needed for SMTP and IMAP MIME mail uploads.

In addition, the CURLOPT_HTTPHEADER and --header documentation does not mention their use for MIME mail.

This commit fixes the problems above.

jay
jay reviewed Sep 28, 2022
View reviewed changes
docs/libcurl/opts/CURLOPT_HTTPHEADER.3 Outdated
@@ -98,6 +98,9 @@ implies that if you tell libcurl to follow redirects
in the subsequent request. Redirects can of course go to other hosts and thus
those servers will get all the contents of your custom headers too.

This option must also be used to set the top-level headers of a MIME mail when
uploading the latter via SMTP or IMAP.

Copy link
Member

Choose a reason for hiding this comment

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

how is this a security concern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how is this a security concern

Thanks for pointing this out: I placed the paragraph in a wrong section.

@bagder
Copy link
Member

bagder commented Sep 28, 2022

I was not completely aware of this. I think the man page needs more massaging to better convey this fact. It is even unfortunate that it is named HTTPHEADER when in fact then isn't only for HTTP headers.

Should we consider a new alias for this called REQUESTHEADERS or something?

@monnerat
Copy link
Contributor Author

monnerat commented Sep 28, 2022 •
edited

Should we consider a new alias for this called REQUESTHEADERS or something?

I think we should. We already did this kind of thing (i.e.: CURLOPT_FTP_SSL) but at the time of mime coding, I did not want to make a revolution on existing public symbols. Then it came out of my mind.

IMAP and SMTP mime mails use this option because the mime api we implemented supports setting headers on parts only. Mime mail data need to support document-level headers.

We currently already have the (undocumented) CURLOPT_RTSPHEADER alias. As the use in the mime case is not related to the request and is multi-protocol, I think a good alias would be CURLOPT_MAILHEADERS. I tried to imagine a more generic name but discarded the use of simply CURLOPT_HEADERS because it might be confused with the existing CURLOPT_HEADER option. Maybe you have a better idea?

@bagder
Copy link
Member

bagder commented Sep 28, 2022

(It is of course a bit off topic for this PR)

I think i would prefer a new name to use for the option globally rather than to continue to invent special protocol aliases.

CURLOPT_SENDHEADERS ?
CURLOPT_MYHEADERS ?

An obvious downside with a name change (even when the old name is still supported) is that this is a very commonly used option so there exist a bazillion examples of its use on stackoverflow and other places online, and they will look worse if we change the name... That could be an argument for just adding an alias like CURLOPT_MAILHEADERS.

@monnerat
Copy link
Contributor Author

I think i would prefer a new name to use for the option globally rather than to continue to invent special protocol aliases.

Yes, me too. what about CURLOPT_CUSTOMHEADERS ?

An obvious downside with a name change ...

Agreed. But we did it already for CURLOPT_FTP_SSL and could still move CURLOPT_HTTPHEADER as a deprecated alias.
Are you planning to do this change in the current PR ?

@monnerat
Copy link
Contributor Author

Documentation updated. CURLOPT_HTTPHEADER not renamed.

@monnerat
lib: sanitize conditional exclusion around MIME
d208f81 
The introduction of CURL_DISABLE_MIME came with some additional bugs:
- Disabled MIME is compiled-in anyway if SMTP and/or IMAP is enabled.
- CURLOPT_MIMEPOST, CURLOPT_MIME_OPTIONS and CURLOPT_HTTPHEADER are
  conditioned on HTTP, although also needed for SMTP and IMAP MIME mail
  uploads.

In addition, the CURLOPT_HTTPHEADER and --header documentation does not
mention their use for MIME mail.

This commit fixes the problems above.
@bagder
Copy link
Member

bagder commented Sep 29, 2022

Thanks!

@bagder bagder closed this in 2437fac Sep 29, 2022
@monnerat monnerat deleted the disable-mime branch September 29, 2022 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jay jay jay left review comments

@bagder bagder bagder approved these changes

Assignees
No one assigned
Labels
build documentation
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@monnerat @bagder @jay