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.
Comment | File | Size | Author |
---|---|---|---|
#22 | drupal-ajax-error-reporting-level-1587858-22.patch | 1.04 KB | apotek |
#9 | make_ajax_alerts_respect_error_reporting_settings-D8-1587858-9.patch | 1.06 KB | apotek |
#1 | drupal-ajax-error-reporting-level-1587858-1.patch | 1.04 KB | apotek |
Comments
Comment #1
apotek CreditAttribution: apotek commentedAnd.... here's the patch.
Comment #1.0
apotek CreditAttribution: apotek commentedEditing the error output.
Comment #2
nod_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.
Comment #3
nod_to fix in 8.x first
Comment #4
apotek CreditAttribution: apotek commentedMy 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?
Comment #5
nod_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?
Comment #7
apotek CreditAttribution: apotek commentedHello 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.
Comment #8
nod_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?
Comment #9
apotek CreditAttribution: apotek commentedHere you go....
Comment #10
apotek CreditAttribution: apotek commentedComment #11
ericduran CreditAttribution: ericduran commented@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.
Comment #12
apotek CreditAttribution: apotek commentedYes, 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.
Comment #13
nod_Awesome, will make a last quick check and very likely rtbc this thing tomorrow or so, if someone don't beat me to it :)
Comment #14
nod_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.
Comment #15
q0rban CreditAttribution: q0rban commentedThis patch looks great, very simple and makes sense. I agree with the logic outlined in comment #4, also.
Comment #16
nod_anyway, error_displayable is out of scope let's just go with it.
Comment #17
apotek CreditAttribution: apotek commentedHello _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.
Comment #18
nod_the config system changed in D8 (that's messing with variable_get) that might be why.
Comment #19
apotek CreditAttribution: apotek commentedHello 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.]
Comment #20
ericduran CreditAttribution: ericduran commented@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 :)
Comment #21
catchFix looks right to me. Committed/pushed to 8.x, moving to 7.x for backport.
Comment #22
apotek CreditAttribution: apotek commentedHere's the patch for latest dev D7
Comment #23
apotek CreditAttribution: apotek commentedforgot to change the status to needs review.
Comment #24
ericduran CreditAttribution: ericduran commentedLooks good.
Comment #25
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/ed197d6
Comment #27
apotek CreditAttribution: apotek commentedThanks 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.
Comment #28
nod_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.
Comment #28.0
nod_modified the proposed solution to more accurately reflect the submitted patch.