WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#42102 closed defect (bug) (fixed)

File Editor: Make fatal error detection more robust for edits to PHP files

Reported by: westonruter Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch needs-testing commit
Focuses: Cc:

Description (last modified by westonruter)

Follow-up on #21622.

I found that on sites that have HTTP Basic auth enabled, the loopback request to do a fatal error scrape can fail, causing a bad PHP file edit to not be rolled back and instead appear as successful. So we should pass basic auth in error scraping loopback requests in addition to cookies.

Additionally, there are a couple other things to make it more robust:

  1. Set a timeout for the loopback requests to make sure that the spawning process runs longer than the spawned processes.
  2. Output needle on success too as a way confirm request went through successfully.
  3. Check response code and if not 200 then rollback the file change.

See https://wordpress.slack.com/archives/C02RQBWTW/p1507181594000012

Attachments (2)

42102.0.diff (7.1 KB) - added by westonruter 2 years ago.
42102.1.diff (7.4 KB) - added by westonruter 2 years ago.
Pass along HTTP Basic credentials in loopback requests

Download all attachments as: .zip

Change History (10)

#1 @westonruter
2 years ago

  • Description modified (diff)
  • Owner set to westonruter
  • Status changed from new to accepted
  • Summary changed from File Editor: Pass basic auth in error scraping loopback requests in addition to cookies to File Editor: Make fatal error detection more robust for edits to PHP files

#2 @pixolin
2 years ago

Hello,

I'm testing 4.9-beta1-41785 and get following reproducable issue:

I go to Appearance => Editor, Select functions.php of theme Twenty Seventeen (or any other), delete a closing bracket and Update File, my change gets confirmed with a message "File edited successfully.". If I then leave the editor and open any other webpage in front- or backend, I receive a

Fatal error: syntax error, unexpected ';' in /Users/bego/MAMP/dev/wp-content/themes/twentyseventeen/functions.php on line 16

While this is the expected result due to the code change, it breaks the website and can only be fixed by chaning the file via FTP. The fatal error was not detected in the theme editor.

If I go back in browsing history, I can see the file editor and replace the missing bracket, but get an error message "save error" (which looks like a correct error message, as the website is broken).

Last edited 2 years ago by pixolin (previous) (diff)

#3 @westonruter
2 years ago

@pixolin Thanks for the details, and that this happened to you locally. The problem is probably that your WP install is unable to successfully make the loopback request. This will be fixed with the proposed fixes I have above, but it will then mean that you wouldn't be able to make any PHP change at all because it would not be able to make a successful loopback request to check for the fatal error. So in that case we'll probably need to show a notice saying that it was not able to communicate with the site, and that the PHP file change must be uploaded via FTP instead.

@westonruter
2 years ago

#4 follow-up: @westonruter
2 years ago

  • Keywords has-patch needs-testing added

@pixolin Would you test 42102.0.diff?

#5 in reply to: ↑ 4 @pixolin
2 years ago

  • Resolution set to worksforme
  • Status changed from accepted to closed

Replying to westonruter:

@pixolin Would you test 42102.0.diff?

Tested the patch: works perfectly'''

After intentionally adding a syntax error and saving the changes I get an error message that my "code changes were rolled back due to an error" pointing exactly at the wrong syntax. I can revert the change, save and then continue to work. Thank you.


Ups. Didn't want to close the issue – just signaling, it works for me. Sorry.

Last edited 2 years ago by pixolin (previous) (diff)

#6 @pixolin
2 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

#7 @westonruter
2 years ago

  • Keywords commit added
  • Status changed from reopened to accepted

@westonruter
2 years ago

Pass along HTTP Basic credentials in loopback requests

#8 @westonruter
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 41805:

File Editor: Increase robustness of fatal error checking when saving PHP file edits.

  • Increase PHP execution time limit prior to issuing loopback requests where are themselves given timeouts to ensure PHP file can be reverted.
  • Output scrape messages on success and failure so that absence of either can also be flagged as an error condition.
  • Forward browser's HTTP Basic Auth credentials in loopback requests to admin and home URL.
  • Display more helpful message when loopback request fails.

Amends [41721].
See #21622.
Fixes #42102.

Note: See TracTickets for help on using tickets.