ruby / net-imap

Ruby client api for Internet Message Access Protocol
https://ruby.github.io/net-imap
Other
55 stars 27 forks source link

invalid flag-list #228

Closed Eguthrie3214 closed 9 months ago

Eguthrie3214 commented 10 months ago

After updating from 0.4.1 to 0.4.3 we noticed errors when running a test that selects an inbox

rails aborted! Net::IMAP::ResponseParseError: invalid flag-list /Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser/parser_utils.rb:234:in parse_error' /Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser/parser_utils.rb:205:inmatch_re' /Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser.rb:1713:in flag_list' /Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser.rb:1155:inmailbox_data__flags' /Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser.rb:571:in `response_data'

I was able to track this down to #212 and the change to flag parsing.

Given the same input of Screenshot 2023-11-13 at 2 51 40 PM for both versions, 0.4.3 fails

If it helps the combo of services im using looks like this

version: '3'

services:

  db:
    image: postgres
    ports:
      - "6099:5432"
    environment:
      - POSTGRES_USER=workflows
      - POSTGRES_PASSWORD=password
    volumes:
      - postgres-data:/var/lib/postgresql/data

  redis:
    image: redis
    ports:
      - "7099:6379"

  mail:
    image: greenmail/standalone
    ports:
      - "3465:3465" #SMTPS
      - "3993:3993" #IMAPS
      - "3025:3025" #SMTP
      - "3143:3143" #IMAP
  roundcube:
    image: roundcube/roundcubemail:latest
    depends_on:
      - mail
    ports:
      - "8000:80"
    environment:
      - ROUNDCUBEMAIL_DEFAULT_HOST=mail  # IMAP server - tls:// prefix for STARTTLS, ssl:// for SSL/TLS
      - ROUNDCUBEMAIL_DEFAULT_PORT=3143               # IMAP port
      - ROUNDCUBEMAIL_SMTP_SERVER=mail  # SMTP server - tls:// prefix for STARTTLS, ssl:// for SSL/TLS
      - ROUNDCUBEMAIL_SMTP_PORT=3025
  dynamodb:
    image: amazon/dynamodb-local
    ports:
      - "8001:8000"
    volumes:
      - dynamodb-data:/home/dynamodblocal/data
    command: "-jar DynamoDBLocal.jar -sharedDb -dbPath /home/dynamodblocal/data/"

volumes:
  postgres-data:
  dynamodb-data:

and for an example of what im running to trigger this

    puts imap.list("", "*")
    imap.select("INBOX")

imap is an instance of the imap client The list command works, but it breaks during the select.

Thank you for the time and I would be happy to provide more information!

nevans commented 10 months ago

Sorry about this! I do have a few questions:

Is there a missing "\r" in the example screenshot? Because "* LIST () \".\" \"INBOX\"\r\n" (with the \r) parses just fine for me, but "* LIST () \".\" \"INBOX\"\n" (without it) is expected to fail. But also, the @str in that screenshot doesn't seem to match the stacktrace. That stacktrace seems to match a FLAGS response of some sort.

If you use Net::IMAP.debug = true, it should print out more useful information whenever a parse error is encountered. Could you copy/paste that debug result here, please?

Eguthrie3214 commented 10 months ago

Hello, sorry about any confusion, I took screen shots of both the input when it worked and when it didnt so I may have gotten them mixed up. Here is a trace with the debug option enabled.


Net::IMAP::ResponseParser parse_error: invalid flag-list
  tokenized : "* FLAGS "
  remaining : "(\\Answered \\Deleted \\Draft \\Flagged \\Seen \\*)\r\n"
  @lex_state: EXPR_BEG
  @pos      : 8
  @token    : nil
  caller[ 0]: match_re                       (parser_utils:205)
  caller[ 1]: flag_list                      (response_parser:1713)
  caller[ 2]: mailbox_data__flags            (response_parser:1155)
  caller[ 3]: response_data                  (response_parser:571)
  caller[ 4]: response                       (response_parser:509)
  caller[ 5]: parse                          (response_parser:33)
  caller[ 6]: get_response                   (imap:2544)
  caller[ 7]: receive_responses              (imap:2439)
  caller[ 8]: start_receiver_thread          (imap:2416)
rails aborted!
Net::IMAP::ResponseParseError: invalid flag-list
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser/parser_utils.rb:234:in `parse_error'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser/parser_utils.rb:205:in `match_re'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser.rb:1713:in `flag_list'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser.rb:1155:in `mailbox_data__flags'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser.rb:571:in `response_data'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser.rb:509:in `response'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap/response_parser.rb:33:in `parse'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap.rb:2544:in `get_response'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap.rb:2439:in `receive_responses'
/Users/evanguthrie/.rvm/gems/ruby-3.1.0@v3-workflows/gems/net-imap-0.4.3/lib/net/imap.rb:2416:in `block in start_receiver_thread'

If I break on line 201 of parser_utils.rb I get Screenshot 2023-11-15 at 2 47 21 PM

Let me know if you want me to do anything else and I would be happy to do it :) This isn't currently an issue as we've locked our gem to 0.4.1, but it would be nice to able to upgrade.

nevans commented 10 months ago

Thanks.

So this looks like a bug in the server: while \* is valid for PERMANENTFLAGS, it is not for FLAGS. As part of updating the parser code for rev2 and various LIST extensions, I had changed it to be more strict about this.

While I'd like to parse strictly when we can get away with it, we do include many workarounds for "quirky" servers. I'll probably want to add configuration params in the future, but for now, what do you think about printing a warning message for this sort of invalid response?

Eguthrie3214 commented 10 months ago

I think that would be great. I work with email quite a bit (as do you it seems) and its almost always problems of like "x client or server does this, and thats incompatible with y" so I feel like being somewhat permissive, or giving an option to configure if needed would be cool.

Thanks so much for the help on this!

nevans commented 10 months ago

@Eguthrie3214 I pushed a branch with a fix, and I made an issue with greenmail. That project seems to be active and responsive, so I want to hear their response before I turn the branch into a PR (and merge) But, if it's useful, you can test or use the branch immediately (e.g. using bundler: gem "net-imap", github:, branch:).

Eguthrie3214 commented 10 months ago

Thank you so much! Ive tested the branch and it works. Ill keep track of your greenmail issue as well because the best solution is probably them just making the mail client adhere to the rfc.

nevans commented 9 months ago

@Eguthrie3214 FYI: I'm not really keeping the workaround branch up-to-date. But I won't delete it either, at least not any time soon. The greenmail issue was closed and marked to be backported, but I don't know how often they make new releases for backports.

I'm going to close the ticket. Please reopen it if you think it should be kept open or addressed. I'll reopen if I see any evidence of servers in the wild sending a response like that.

nevans commented 9 months ago

Aaaaaand Gmail has a bug similar to this one! 😆 It's slightly different (worse actually), so I'm opening another issue for it: #241.

nevans commented 9 months ago

@Eguthrie3214 For what it's worth, PR #246 to fix #241 was released with v0.4.8, and it does work around this issue too.

Even though greenmail fixed their bug and I haven't seen it anywhere else, I figured that it was at least worth capturing this scenario in our test suite. So I just merged #249 to do that. 🙂