James Mime4j
  1. James Mime4j
  2. MIME4J-90

Consistent parsing of header field names

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.5
    • Fix Version/s: 0.6
    • Component/s: None
    • Labels:
      None

      Description

      RFC 822 defines a field as:
      field = field-name ":" [ field-body ] CRLF
      field-name = 1*<any CHAR, excluding CTLs, SPACE, and ":">

      This implies that a field name must consist of at least one character and may not contain spaces or tabs; not even trailing ones.

      Currently o.a.j.mime4j.parser.AbstractEntity#parseField accepts empty field names while o.a.j.mime4j.field.Field#parse accepts trailing spaces and tabs.

      1. mime4j-parse-name-body.patch
        8 kB
        Markus Wiederkehr
      2. mime4j-fieldname.patch
        5 kB
        Markus Wiederkehr

        Activity

        Hide
        Markus Wiederkehr added a comment -

        This patch should resolve the issue.

        As a consequence it also changes the MimeException thrown by Field#parse back to IllegalArgumentException (see also MIME4J-73). This is safe now because Field#parse never gets called with an illegal argument from a ContentHandler. AbstractEntity#parseField already drops invalid header fields.

        Show
        Markus Wiederkehr added a comment - This patch should resolve the issue. As a consequence it also changes the MimeException thrown by Field#parse back to IllegalArgumentException (see also MIME4J-73 ). This is safe now because Field#parse never gets called with an illegal argument from a ContentHandler. AbstractEntity#parseField already drops invalid header fields.
        Hide
        Oleg Kalnichevski added a comment -

        The patch looks good to me. I'll commit it if I hear no objections

        Oleg

        Show
        Oleg Kalnichevski added a comment - The patch looks good to me. I'll commit it if I hear no objections Oleg
        Hide
        Oleg Kalnichevski added a comment -

        Patch checked in

        Oleg

        Show
        Oleg Kalnichevski added a comment - Patch checked in Oleg
        Hide
        Robert Burrell Donkin added a comment -

        Field.parse is a public method. As such, it's not just used by Mime4J. I'm concerned about the potential impact on users of this method.

        Switching from throwing a MimeException to a runtime requires users of that method to revise their exception handling strategies. Switching to less lenient parsing only increases this risk.

        Show
        Robert Burrell Donkin added a comment - Field.parse is a public method. As such, it's not just used by Mime4J. I'm concerned about the potential impact on users of this method. Switching from throwing a MimeException to a runtime requires users of that method to revise their exception handling strategies. Switching to less lenient parsing only increases this risk.
        Hide
        Robert Burrell Donkin added a comment -

        I would prefer the a new name for this method.

        At least, I would like to see strong warnings in the documentation and release notes.

        I'm willing to dive in and help resolve this issue.

        Show
        Robert Burrell Donkin added a comment - I would prefer the a new name for this method. At least, I would like to see strong warnings in the documentation and release notes. I'm willing to dive in and help resolve this issue.
        Hide
        Markus Wiederkehr added a comment -

        Robert, please note that this method has always been throwing an IllegalArgumentException up to and including release 0.4. We changed it to a MimeException for 0.5 in MIME4J-73.

        At the time I was not aware of the fact that o.a.j.mime4j.parser.AbstractEntity#parseField already parses the header fields before it passes them to a ContentHandler. It splits the header field into key and value and checks if the key conforms to RFC 822. If this is not the case the header field gets dropped entirely.

        So the change to a MimeException was based on a false assumption. Changing it back to the way it was should not cause big problems for the users of this method.

        Show
        Markus Wiederkehr added a comment - Robert, please note that this method has always been throwing an IllegalArgumentException up to and including release 0.4. We changed it to a MimeException for 0.5 in MIME4J-73 . At the time I was not aware of the fact that o.a.j.mime4j.parser.AbstractEntity#parseField already parses the header fields before it passes them to a ContentHandler. It splits the header field into key and value and checks if the key conforms to RFC 822. If this is not the case the header field gets dropped entirely. So the change to a MimeException was based on a false assumption. Changing it back to the way it was should not cause big problems for the users of this method.
        Hide
        Robert Burrell Donkin added a comment -

        I don't seem to have done a very good job of explaining the issue.

        Using runtime exceptions make things difficult for correct protocol implementations. Typically, protocols have their own in-protocol error reporting systems. It is expected that exceptions will be checked so that developers can caught and correctly handle all expected known issues. Runtime exceptions are assumed to be fatal to the session and will typically result in the socket disconnecting (for security reasons, it's unwise to continue after a programming error).

        These are public classes and form a public API. The runtime was replaced by a checked exception to allow these classes to be safely used in protocol work in a standalone fashion. Reverting this design decision is a potentially dangerous decision since protocol implementations may be relying on the checked exception to correctly handle that condition. Upgrading will break any protocols which made this assumption.

        When breaking the contract of a public API, it is better to do this in a cleanly incompatible way. If this is not possible then it must be highlighted in the release notes.

        Show
        Robert Burrell Donkin added a comment - I don't seem to have done a very good job of explaining the issue. Using runtime exceptions make things difficult for correct protocol implementations. Typically, protocols have their own in-protocol error reporting systems. It is expected that exceptions will be checked so that developers can caught and correctly handle all expected known issues. Runtime exceptions are assumed to be fatal to the session and will typically result in the socket disconnecting (for security reasons, it's unwise to continue after a programming error). These are public classes and form a public API. The runtime was replaced by a checked exception to allow these classes to be safely used in protocol work in a standalone fashion. Reverting this design decision is a potentially dangerous decision since protocol implementations may be relying on the checked exception to correctly handle that condition. Upgrading will break any protocols which made this assumption. When breaking the contract of a public API, it is better to do this in a cleanly incompatible way. If this is not possible then it must be highlighted in the release notes.
        Hide
        Markus Wiederkehr added a comment -

        Here is another patch that would work for me. It keeps the MimeException in Field.parse(String raw) and adds another method Field.parse(String name, String value) that does not throw an exception.

        Please review.

        Show
        Markus Wiederkehr added a comment - Here is another patch that would work for me. It keeps the MimeException in Field.parse(String raw) and adds another method Field.parse(String name, String value) that does not throw an exception. Please review.
        Hide
        Robert Burrell Donkin added a comment -

        I think the proposed API changes will address my concerns. Feel free to close this issue.

        Show
        Robert Burrell Donkin added a comment - I think the proposed API changes will address my concerns. Feel free to close this issue.

          People

          • Assignee:
            Unassigned
            Reporter:
            Markus Wiederkehr
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development