Problem/Motivation

If an exception or other error is thrown during an AJAX operation, the end user receives an alert dialog with an error message, and a fair amount of diagnostic information. This is great for a development environment. However, it's possible in a production environment, that a sysadmin might not want this information exposed to end users. (I've bracketed out and bold-faced the potentially dangerous information).

Here is an example of an alert dialog i get if I create a flaky db connection in my back end during an autocomplete session.

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: https://drupal7.dev.instance.local/taxonomy/autocomplete/field_ugc_collection_tags
StatusText: Service unavailable (with message)
ResponseText: PDOException: SQLSTATE[28000] [1045] Access denied for user '[db username]'@'[DB HOST]' [(using password: YES)] in lock_may_be_available() ([line 164 of /path/to/application/code/docroot/includes/lock.inc]).

Proposed resolution

Regular exception handling (to the browser window) consults the value of the error_displayable() function before rendering an error the client.

<?php
    if (error_displayable($error)) {
    // more logic to determine what/how to render error
?>

However, the AJAX error part of the function does not consult error_displayable(). It only checks if the error is fatal.

<?php
  if (isset($_SERVER['HTTP_X_REQUESTED_WITH']) && $_SERVER['HTTP_X_REQUESTED_WITH'] == 'XMLHttpRequest') {
    if ($fatal) {
      // When called from JavaScript, simply output the error message.
      print t('%type: !message in %function (line %line of %file).', $error);

?>

My patch would change the AJAX error handling part of _drupal_log_error() to also consult the return value of error_displayable(), changing

<?php
  if (isset($_SERVER['HTTP_X_REQUESTED_WITH']) && $_SERVER['HTTP_X_REQUESTED_WITH'] == 'XMLHttpRequest') {
    if ($fatal) {
      // When called from JavaScript, simply output the error message.
      print t('%type: !message in %function (line %line of %file).', $error);
?>

to

<?php
  if (isset($_SERVER['HTTP_X_REQUESTED_WITH']) && $_SERVER['HTTP_X_REQUESTED_WITH'] == 'XMLHttpRequest') {
    if ($fatal) {
       if (error_displayable($error)) {
        // When called from JavaScript, simply output the error message.
        print t('%type: !message in %function (line %line of %file).', $error);
       }
       exit;
   }    
?>

Remaining tasks

Discuss. Critique. Review. Test, please. One thing I'm not happy with is that the header still says: Service unavailable (with message) in the xmlhttp request, rather than just Service unavailable.

User interface changes

None.

API changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

apotek’s picture

And.... here's the patch.

apotek’s picture

Issue summary: View changes

Editing the error output.

nod_’s picture

We're going to have a new setting for js errors: #1232416-76: Drupal alerts "An AJAX HTTP request terminated abnormally" during normal site operation, confusing site visitors/editors so that could be something to use here as well.

Also about error reporting check out #1419652: JavaScript logging and error reporting

If you could make a new JS-only logging level setting and use that to check in error_displayable that would be perfect.

nod_’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Needs work

to fix in 8.x first

apotek’s picture

My opinion: It's safest if display_errors in php.ini rules all. And then if Drupal can provide settings to override that default. The more variables to keep track of in order to set a site up safely, the more difficult it is for people to secure their installations out of the box. I was already perturbed to find that setting display_errors to 0 in php.ini was not enough to silence Drupal. But I'm sure there's been a lot of talk already and I doubt I'm going to change the direction of that conversation :-).

In the interest of iterative improvements, rather than leaving a potential vulnerability open until an enormous reconceptualization of error reporting in D8 takes place, what's wrong with at least baking in (to D7 at least) a partial improvement that can at the very least plug the hole for the meantime? This patch changes nothing in the API, does not require re-writing the signature of any functions, and can, as is, allow a site administrator to improve the security of a production site.

Since D8 wants to go the route of a fully re-conceptualized error model, can we please switch this issue back to D7/D6 and view it simply as a security patch rather than a new feature?

nod_’s picture

The JS error level is because you might want to have all the error displayed but none of the PHP ones. Thinking some more about it it doesn't really make sense, if you don't want errors just write clean code and all will be well. Also outputting log/debug messages on front is not performance-friendly, it should be done during dev, not prod.

Nothing specific was decided on the error logging thing, the meta issue is there to discuss it actually, feel free to post your ideas there. I have no idea what is planned overall for error logging in D8.

All that to say, patch is welcome. Does the patch as it is currently still outputs a JS error event if you don't print the error text or does it silently fails?

apotek’s picture

Hello nod_. An error alert will still be presented. All this patch does is make sure your desire to not output dangerous information to the end user is respected.

The example I use is a failed db connection. Before the patch, even if you have ini_set('display_errors', 0), and $conf['error_level'] = 0;, if there is a db connection failure during an ajax call, the user will see your DB connection information in an alert dialog! (username, hostname etc etc).

After the patch, the user will still see an error alert, but without the rendered db connection exception information, since the patch ensures that the settings above will be respected, even with regards to ajax errors.

if (error_displayable($error)) {

The patch uses the already available api to determine whether to print out error information or not. As this API is improved, the patch will not need to be further modified.

In a dev environment, error information will still be displayed to the end user/developer, as long as you don't have $conf['error_level'] set to 0.

nod_’s picture

I was just worrying since responseText will be empty sometimes. As long as an alert comes up, empty of not, that will be fine.

Can you reroll the patch for D8 please?

apotek’s picture

apotek’s picture

Status: Needs work » Needs review
ericduran’s picture

@nod_ I like @apotek solution here. This seems like a simple approach that can easily be back-ported to D7 for now.

Haven't tested so I wont change to RTBC, but I'll do some quick test later see how it goes.

apotek’s picture

I was just worrying since responseText will be empty sometimes.

Yes, the responseText field in the xmlhttp request object will be empty. We *could* try to make a generic t('Application Error') instead of returning nothing, but I think a site admin, if they want no error returned should have no error returned. With the current AJAX implementation, an alert will still pop up, complete with error/status code etc. The user will still be notified, but nothing about the nature of the error will be rendered, which is exactly as it should be if the return value of error_displayable() is false.

nod_’s picture

Awesome, will make a last quick check and very likely rtbc this thing tomorrow or so, if someone don't beat me to it :)

nod_’s picture

I think i'm running into a D8 issue that the error_displayable function doesn't work. otherwise it looks good to me, will rtbc once i get some more info about this error_displayable issue.

q0rban’s picture

This patch looks great, very simple and makes sense. I agree with the logic outlined in comment #4, also.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

anyway, error_displayable is out of scope let's just go with it.

apotek’s picture

Hello _nod. The odd thing is that error_displayable() in D7 and D8 is, line for line, exactly the same. Not sure why there would be an issue in the function itself in D8.

nod_’s picture

the config system changed in D8 (that's messing with variable_get) that might be why.

apotek’s picture

Hello nod_, this issue seems to have been closed for D8 (patch was applied?), and is marked needs backport for D7.

I have the patch for D7 already (it's in comment #1). Do you need me to roll and submit a new one?

Thanks.

[Edit: Oh, wait. I see the D8 patch was never applied. So I guess we're in limbo. Thanks again.]

ericduran’s picture

@apotek the patch's current status is RTBC (Review and tested by the community). It'll stay in this state until one of the D8 maintainers most likely (catch) go through the queue and applies all the RTBC patches. It might take a while to get to this one because the priority is normal. Once that's committed to D8 then we'll have to re-roll the patch for the D7 and go through the same process :)

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Fix looks right to me. Committed/pushed to 8.x, moving to 7.x for backport.

apotek’s picture

Here's the patch for latest dev D7

apotek’s picture

Status: Patch (to be ported) » Needs review

forgot to change the status to needs review.

ericduran’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.15 release notes

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

apotek’s picture

Thanks David_Rothstein! Glad I could help. I guess the --author stuff in the patch doesn't get preserved in commits to Drupal core. Nevertheless, glad this is done.

nod_’s picture

they don't commit with --author for core. usernames of people involved are added to the commit comment. Not great but that comes from CVS days.

nod_’s picture

Issue summary: View changes

modified the proposed solution to more accurately reflect the submitted patch.