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

DateTime::parse_from_rfc2822 not compliant with RFC2822 comments #732

Closed
Finomnis opened this issue Jul 8, 2022 · 7 comments · Fixed by #733
Closed

DateTime::parse_from_rfc2822 not compliant with RFC2822 comments #732

Finomnis opened this issue Jul 8, 2022 · 7 comments · Fixed by #733

Comments

@Finomnis
Copy link
Contributor

Finomnis commented Jul 8, 2022

Problem

According to RFC2822, an RFC2822 string can end with a CFWS.

However, the DateTime::parse_from_rfc2822 cannot deal with comments:

use chrono::DateTime;

fn main() {
    let time = DateTime::parse_from_rfc2822("2 Nov 2021 14:26:12 +0000 (UTC)").unwrap();
    println!("{}", time);
}
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ParseError(TooLong)', src/main.rs:4:80

Implementation Details

The implementation explicitly distances itself from comments and calls itself not an actual rfc2822 parser:

    // - we do not recognize a folding white space (FWS) or comment (CFWS).
    //   for our purposes, instead, we accept any sequence of Unicode
    //   white space characters (denoted here to `S`). any actual RFC 2822
    //   parser is expected to parse FWS and/or CFWS themselves and replace
    //   it with a single SP (`%x20`); this is legitimate.

I'd argue that chrono is the defacto go-to library for anything date/time related in Rust, so this parser should be an actual parser.

Rationale

The documentation of DateTime::parse_from_rfc2822 states:

Parses an RFC 2822 date and time string such as Tue, 1 Jul 2003 10:52:37 +0200, then returns a new DateTime with a parsed FixedOffset.

RFC 2822 is the internet message standard that specifices the representation of times in HTTP and email headers.

This sounds like it should:

  • accept the official RFC 2822 standard
  • be compatible with email headers

Sadly, most email headers use a comment after the string that reflects the timezone, like "2 Nov 2021 14:26:12 +0000 (UTC)". So I'd argue that the behaviour of the library is not consistent with the documentation; it should state that this is not an actual RFC2822 parser, but instead only parses a subset and is explicitly not compatible with email headers.

Solutions

I can see two possible solutions:

  • adjust the documentation to reflect that this is not an actual RFC2822 parser
  • adjust the code to make this an actual RFC2822 parser

This issue was already brought up in #462, but back then it was dismissed as not being a valid RFC2822 string, which is incorrect.

@djc
Copy link
Contributor

djc commented Jul 8, 2022

I would tend to agree that we should probably provide an actual RFC 2822 parser. If you would submit a PR to make the necessary changes, I would like to review the amount of complexity necessary to do so.

@Finomnis
Copy link
Contributor Author

Finomnis commented Jul 8, 2022

@djc I already thought about it and it seems to be not quite trivial; comments are (roughly) defined as:

  • start with (
  • end with )
  • can contain escaped characters \( or \)
  • can be nested ((a)(b))
  • can contain whitespace

Might work on that, but after an initial review I don't think I'll be able to do so within the next week, probably. I'll make an attempt, but if someone else feels like he could do it easily, please don't wait for me

@djc
Copy link
Contributor

djc commented Jul 8, 2022

I'm not in a hurry, and will definitely not have time to move this forward myself.

@Finomnis
Copy link
Contributor Author

Finomnis commented Jul 9, 2022

@djc Well, seems like I found the time after all. Feedback appreciated.

@Finomnis
Copy link
Contributor Author

Ping :)

@djc
Copy link
Contributor

djc commented Jul 14, 2022

Your PR is on my list and won't be forgotten, but I'm currently on holidays, so please have some patience.

@Finomnis
Copy link
Contributor Author

No problem. Didn't mean to stress you out :) enjoy your holiday

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants