Page MenuHomePhabricator

load.php should not return HTML if DB throws exception
Closed, ResolvedPublic

Description

If load.php cannot connect to db, it returns the same html page as index.php does, and has a status code of 200.

I think the more expected behaviour would be to return the status 500 (probably should do that for index.php too) and just have a comment /* Could not connect to db */ instead of outputting html (although if outputting a 500 error code, returning html isn't as bad).

Discovered on [[mw:Thread:Project:Support desk/Edit toolbar is not there. Broken]]


See Also:
T37305: Borked API response has HTML DB error appended in case of database lock timeout

Details

Reference
bz33917
ReferenceSource BranchDest BranchAuthorTitle
repos/sre/mediabackups!1T359176mainjynussql: Increase the maximum storage path to 300 bytes
repos/abstract-wiki/aw-ci-chart!29T359179mainjforresterDrop the health check tests, API is going away
Customize query in GitLab

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 12:04 AM
bzimport set Reference to bz33917.
bzimport added a subscriber: Unknown Object (MLST).

Steps to reproduce:

  • Load local wiki page in browser
  • Change $wgDBpassword in LocalSettings.php to a wrong password
  • Enter JavaScript console and do a mw.loader command for a module that isn't loaded yet --

mw.loader.load( 'filepage' );
mw.loader.using( 'jquery.ui.button', function () {

console.log( 'button: ok' );

}, function () {

console.log( 'button: err' );

});

Expected result:

  • load.php does something that causes the JavaScript client's registry to register state "error" and trigger the error callback

Actual result:

  • WebKit console:

WARNING> Resource interpreted as Other but transferred with MIME type undefined.
ERROR> [load.php: line1] Uncaught SyntaxError: Unexpected token <

  • < mw.loader.getState( 'jquery.ui.button' )

"loading"

GET /mw/trunk/phase3/load.php?debug=false&lang=en&modules=jquery.ui.button%2Ccore%2Cwidget&skin=vector&version=20120128T163417Z&* HTTP/1.1
Host: localhost
Connection: keep-alive
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_2) AppleWebKit/535.11 (KHTML, like Gecko) Chrome/17.0.963.12 Safari/535.11
Accept: */*
Referer: http://localhost/mw/trunk/phase3/index.php/Main_Page

HTTP/1.1 200 OK
Date: Sat, 28 Jan 2012 16:34:24 GMT
Server: Apache/2.2.20 (Unix) mod_ssl/2.2.20 OpenSSL/0.9.8r DAV/2 PHP/5.3.6 with Suhosin-Patch
X-Powered-By: PHP/5.3.6
X-Content-Type-Options: nosniff
Content-Length: 1149
Keep-Alive: timeout=5, max=95
Connection: Keep-Alive
Content-Type: text/html; charset=utf-8

<h1>Sorry! This site is experiencing technical difficulties.</h1><p>Try waiting a few minutes and reloading.</p><p><small>(Can't contact the database server: <span dir="ltr">Access denied for user 'root'@'localhost' (using password: YES) (127.0.0.1)</span>)</small></p><hr /><div style="margin: 1.5em">You can try searching via Google in the meantime.<br />
<small>Note that their indexes of our content may be out of date.</small></div>
<!-- SiteSearch Google -->
<form method="get" action="//www.google.com/search" id="googlesearch">
<input type="hidden" name="domains" value="http://localhost" />

  • Don't give HTML response
  • Module should not stay in state "loading" but do state "error"
  • Client should call "error" callback

If it's a JS resource we could probably even throw in a bit of code to make it show the error message in the console.

(In reply to comment #3)

If it's a JS resource we could probably even throw in a bit of code to make
it
show the error message in the console.

The elaborate message can be found in the /* comment */ when debugging the request and/or accessing the url directly, and it is also logged to the error log file.

Logging the console is somewhat redundant and complicated:

  • the response should be valid CSS and JS
  • mw.log may not be available yet
  • console.log is not reliably available at this stage

and re-inventing all this logic in load.php is imho not a good idea.

(In reply to comment #2)

  • Don't give HTML response
  • Module should not stay in state "loading" but do state "error"
  • Client should call "error" callback
  • Bug 42166 has been marked as a duplicate of this bug. ***
Krinkle renamed this task from load.php should return HTTP 500 if one or more exceptions were thrown to load.php should not return HTML if DB throws exception.Aug 28 2015, 9:58 AM
Krinkle set Security to None.
Krinkle removed a subscriber: wikibugs-l-list.
Krinkle claimed this task.

I was not able to trivially reproduce this. I suspect because we've reduced some of the PHP code in the early webstart path (T189966) and so perhaps the code triggering these no longer exists.

I suppose it is still possible to trigger this if another exception is thrown on load.php prior to ResourceLoader being instantiated. For example, a syntax error in LocalSettings.php, or some kind of fatal error during the construction of services used by ResourceLoader (e.g. DBLoadBalancerFactory).

These seem very rare, and so long as the status code is correct I'm not sure it's worth addressing. If anything, one could argue such errors are most likely not going to be unique to load.php but affect all entry points in which case two things would be the case:

  1. The browser won't make the load.php request because the index.php request will have failed first.
  2. If you do access load.php directly in the browser then its a navigation where HTML would and is arguably more useful than JS encoding.

I do note that with T228895 the surface area for this would be even smaller. Ultimately, there is no user-facing impact of this. The request fails either way. The request is not cacheable either way. The interface will be broken in the same exact way. The request will be marked as failed in the DevTools Network panel if the user is a developer or tech-savvy user that looks there. It just renders it differently which for very rare cases like this seems acceptable.

The main catch 22 here is that if load.php fails in general MW WebStart.php prior to the ResourceLoader service being active, we don't actually know that we are in a JS response vs HTML. In fact, it may very well not be JS because it could also end up being a CSS response, PNG response, or SVG response. And there is always goingn to be some amount of code where you try to determine that and that logic itself could fail. I think we've made thus sufficiently narrow over the years to consider this resolved. (The original example is resolved.)