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

imap_open throws \ErrorException and masks real error #571

Closed
underdpt opened this issue Jan 8, 2021 · 8 comments · Fixed by #652
Closed

imap_open throws \ErrorException and masks real error #571

underdpt opened this issue Jan 8, 2021 · 8 comments · Fixed by #652

Comments

@underdpt
Copy link
Contributor

underdpt commented Jan 8, 2021

Environment (please complete the following information):

  • PHP IMAP version: 4.1.0
  • PHP Version: 7.4.11 & 7.4.13
  • Type of execution: CLI

Your Text
A little context: I'm woking with Laravel, and am facing this issue when debuggin connection to Gmail accounts.

Whenever there's a credentials error, I get the next error:

imap_open(): Couldn't open stream {imap.gmail.com:993/ssl/novalidate-cert}

Trying to debug this, if catch that error and run imap_last_error():

[AUTHENTICATIONFAILED] Invalid credentials (Failure)

It seems that imap_open() is throwing an \ErrorException that is undocumented (it should simply return false) and this exception is masking the real error.

I think we could catch \ErrorException on PhpImap:702 and add the exception message as additional info to $lastError as a workaround. I can submit a PR if that seems ok.

@Sebbo94BY
Copy link
Collaborator

PR are very welcome! Let us see, which change you expect. Please also provide a current and after example, that we can see from which behaviour it would change to which new one.

@augustusbuckman
Copy link

I wan to know if there is any progress on this issue?

@dartrax
Copy link

dartrax commented Oct 6, 2021

I get warnings from imap_open() from time to time, I assume something is temporarly out of service while it tries to connect.

PHP Warning:  imap_open(): Couldn't open stream {xxxxxxxx.biz:993/imap/ssl/novalidate-cert}INBOX in /var/www/vhosts/xxxxxxxx.biz/xxxxxxxx.biz/vendor/php-imap/php-imap/src/PhpImap/Imap.php on line 702
PHP Notice:  Unknown: No such host as xxxxxxxx.biz (errflg=2) in Unknown on line 0

I wonder if imap_open() should just return false like said before. Since it doesn't, wouldn't

catch \ErrorException on PhpImap:702 and add the exception message as additional info to $lastError as a workaround. I can submit a PR if that seems ok.

help here, too?

@Sebbo94BY
Copy link
Collaborator

Does this issue still exist in the latest release 4.3.0?

@underdpt
Copy link
Contributor Author

Yes, it's still there. I'm sorry I don't have much time for this. I did a little tinkering with 4.3.0 today, simply trying to connect to a gmail account with invalid credentials.

On imap_last_error() I get Too many login failures, but there's an underlying error:

PHP Fatal error:  Uncaught ErrorException: Unknown: [ALERT] Application-specific password required: https://support.google.com/accounts/answer/185833 (Failure) (errflg=1) in Unknown:0

Debuggin a little more, I think the issue relies in the imap errors management on function open on src/PhpImap/Imap.php:689. If it tries to connect 3 times, and then gives up with Too many login failures, then in imap_errors() we have:

array:4 [
  0 => "[ALERT] Application-specific password required: https://support.google.com/accounts/answer/185833 (Failure)"
  1 => "[ALERT] Application-specific password required: https://support.google.com/accounts/answer/185833 (Failure)"
  2 => "[ALERT] Application-specific password required: https://support.google.com/accounts/answer/185833 (Failure)"
  3 => "Too many login failures"
]

But then on line 710 we first check the last error (which we throw) and there's another check that throws all errors but we never got to it:

public static function open(
    string $mailbox,
    string $username,
    string $password,
    int $options = 0,
    int $n_retries = 0,
    array $params = []
) {
    if (\preg_match("/^\{.*\}(.*)$/", $mailbox, $matches)) {
        $mailbox_name = $matches[1] ?? '';

        if (!\mb_detect_encoding($mailbox_name, 'ASCII', true)) {
            $mailbox = static::encodeStringToUtf7Imap($mailbox);
        }
    }

    \imap_errors(); // flush errors

    $result = @\imap_open($mailbox, $username, $password, $options, $n_retries, $params);

    if (!$result) {
        $lastError = \imap_last_error();

        if ((\is_string($lastError)) && ('' !== \trim($lastError))) {
            throw new UnexpectedValueException('IMAP error:'.$lastError);
        }

        throw new UnexpectedValueException('Could not open mailbox!', 0, self::HandleErrors(\imap_errors(), 'imap_open'));
    }

    return $result;
}

Given that before trying to connect we cleared all the imap_errors, then all errors should be related to the ongoing connection so shouldn't it just output all the errors. That would be as easy as removing lines 710 to 715:

public static function open(
    string $mailbox,
    string $username,
    string $password,
    int $options = 0,
    int $n_retries = 0,
    array $params = []
) {
    if (\preg_match("/^\{.*\}(.*)$/", $mailbox, $matches)) {
        $mailbox_name = $matches[1] ?? '';

        if (!\mb_detect_encoding($mailbox_name, 'ASCII', true)) {
            $mailbox = static::encodeStringToUtf7Imap($mailbox);
        }
    }

    \imap_errors(); // flush errors

    $result = @\imap_open($mailbox, $username, $password, $options, $n_retries, $params);

    if (!$result) {
        throw new UnexpectedValueException('Could not open mailbox!', 0, self::HandleErrors(\imap_errors(), 'imap_open'));
    }

    return $result;
}

@Sebbo94BY
Copy link
Collaborator

You don't have that much time, but provide that much and useful information. Thank you! :D

I'll re-check, investigate, test and fix this as soon as possible.

Sebbo94BY added a commit that referenced this issue Jan 7, 2022
…-and-masks-real-error

#571: Improve ConnectionException handling
@Sebbo94BY
Copy link
Collaborator

Sebbo94BY commented Jan 7, 2022

I've changed the behaviour. Please take a look at the respective merge request #652 for further information.

The change is on the master branch, but not yet released/tagged.

You can check test it by using the current master code:

composer require php-imap/php-imap:dev-master

I hope, this solves your issue here. :)

@Sebbo94BY Sebbo94BY reopened this Jan 7, 2022
@Sebbo94BY
Copy link
Collaborator

Changes have been released in version 4.4.0.

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.

4 participants