Make WordPress Core

Opened 9 years ago

Closed 10 months ago

Last modified 8 months ago

#33196 closed defect (bug) (fixed)

ssh2_auth_pubkey_file() call can cause a fatal error because of non-existing key error in $this->options['password']

Reported by: ehsanakhgari's profile ehsanakhgari Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords: has-patch commit
Focuses: Cc:

Description

This happens when the input args do not include a password. I debugged this on my production website.

Attachments (2)

x (751 bytes) - added by ehsanakhgari 9 years ago.
Patch
x.2 (1.0 KB) - added by ehsanakhgari 9 years ago.
Patch (v2)

Download all attachments as: .zip

Change History (15)

@ehsanakhgari
9 years ago

  • Attachment x added

Patch

#1 @ehsanakhgari
9 years ago

  • Focuses administration added
  • Keywords has-patch added

#2 follow-up: @dd32
9 years ago

  • Component changed from General to Filesystem API
  • Focuses administration removed
  • Milestone changed from Awaiting Review to Future Release
  • Version 4.2.3 deleted

Can you post the exact fatal error which occurs in this situation?

Looking at the patch, it can probably be rewritten slightly to set $this->options['password'] before the empty-password no-keys error checks (Assuming that the $opt['password'] key is always set, and won't cause a notice in the current patch)

#3 in reply to: ↑ 2 ; follow-up: @ehsanakhgari
9 years ago

Replying to dd32:

Can you post the exact fatal error which occurs in this situation?

In my local configuration, I got a "connection reset" error page in Firefox in all pages where WordPress was trying to update itself/plugins/themes. After manual debugging using set_error_handler, I got an error message about the non-existing 'password' key in the options array.

Looking at the patch, it can probably be rewritten slightly to set $this->options['password'] before the empty-password no-keys error checks (Assuming that the $opt['password'] key is always set, and won't cause a notice in the current patch)

OK, I'll do that!

@ehsanakhgari
9 years ago

Patch (v2)

#4 in reply to: ↑ 3 ; follow-up: @dd32
9 years ago

Replying to ehsanakhgari:

Replying to dd32:

Can you post the exact fatal error which occurs in this situation?

In my local configuration, I got a "connection reset" error page in Firefox in all pages where WordPress was trying to update itself/plugins/themes. After manual debugging using set_error_handler, I got an error message about the non-existing 'password' key in the options array.

Can you check your PHP Error logs at all for the PHP errors? The notices/warnings about the password key aren't really interesting in this case :)

#5 in reply to: ↑ 4 @ehsanakhgari
9 years ago

Replying to dd32:

Replying to ehsanakhgari:

Replying to dd32:

Can you post the exact fatal error which occurs in this situation?

In my local configuration, I got a "connection reset" error page in Firefox in all pages where WordPress was trying to update itself/plugins/themes. After manual debugging using set_error_handler, I got an error message about the non-existing 'password' key in the options array.

Can you check your PHP Error logs at all for the PHP errors? The notices/warnings about the password key aren't really interesting in this case :)

Sorry for the dumb question, but where do I find the said error logs? It's been ages since I have done anything with PHP. :-) (I run an Ubuntu server.)

This ticket was mentioned in PR #4609 on WordPress/wordpress-develop by J-Dill.


10 months ago
#6

When you use the WP_Filesystem_SSH2 class along with public_key/private_key authentication, you get the following warning message in your debug logs:

“Undefined array key “password““ at `./wp-admin/includes/class-wp-filesystem-ssh2.php:153`

Providing a password with public/private key authentication is optional, but the code currently expects there to always be a password provided. This PR adds a null coalescing check to see if a password is defined in the options. If it is not, then it will define null for the parameter as before.

Trac ticket: https://core.trac.wordpress.org/ticket/33196

#7 @costdev
10 months ago

  • Keywords commit added
  • Milestone set to 6.3

PR 4609 tests well for me. I've approved the PR, but left a suggestion which may make this a little cleaner.


  • Milestoning for 6.3.
  • As the PR tests well and my suggestion on the PR is a preference rather than a blocker, I'm adding this for commit consideration.

#8 @SergeyBiryukov
10 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 56111:

Filesystem API: Define password as null if not set when using SSH2 with public/private keys.

This resolves an Undefined array key "password" PHP warning in WP_Filesystem_SSH2::connect() when using public/private key authentication, in which case providing a password is optional.

Follow-up to [8865].

Props J-Dill, costdev, ehsanakhgari, dd32.
Fixes #33196.

@SergeyBiryukov commented on PR #4609:


10 months ago
#9

Thanks for the PR! Merged in r56111.

@sabernhardt commented on PR #4609:


9 months ago
#10

@J-Dill If you have a moment, could you connect your GitHub account to your wordpress.org one? This change is slated to be included in the upcoming WordPress 6.3 release on August 8th, and it would be great if we could give you attribution by mentioning you on the credits screen. You can find more details on how contributions are tracked and how connecting GitHub accounts helps in this post on the Make WordPress Core blog.

@jdilladog commented on PR #4609:


9 months ago
#11

@sabernhardt Thanks for the notification, I have created an account in wordpress.org and linked it!

@sabernhardt commented on PR #4609:


9 months ago
#12

Sorry. We found 'lowlydev' from your email, but that does not show a connection to GitHub. Do you have a more appropriate WordPress.org username?

@jdilladog commented on PR #4609:


8 months ago
#13

Ah yes that account also is right. I linked my GitHub to that account as well.

Note: See TracTickets for help on using tickets.