It seems that AHAH callbacks such as function upload_js() do not do any error checking - a user visiting the corresponding path will see lots of ugly errors

CommentFileSizeAuthor
#16 check-POST-222588-7x-16.patch11.67 KBJaza
#15 check-POST-222588-7x-15.patch11.42 KBpwolanin
#8 check-POST-222588-8.patch11.07 KBpwolanin
#3 check-POST-222588-3.patch7.06 KBpwolanin
#1 check-POST-222588-1.patch967 bytespwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Active » Needs review
FileSize
check-POST-222588-1.patch967 bytes

starter patch for upload.module

keith.smith’s picture

keith.smith CreditAttribution: keith.smith commented
Title: AHAH callbacks do not do error cehcking » AHAH callbacks do not do error checking
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
check-POST-222588-3.patch7.06 KB

function book_form_update() looks ok, reworking upload and poll to generally match it's logic (i.e. everything in an if block)

cwgordon7’s picture

cwgordon7 CreditAttribution: cwgordon7 commented

Tested, seems to work fine, I got stuff printed to the screen before but nothing that bad now.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Title: AHAH callbacks do not do error checking » AHAH callbacks and ahah.js do not do error checking

one thing I'm noticing now in trying to improve the patch, we return an array in these functions like
drupal_json(array('status' => TRUE, 'data' => $output));
but if status is set to FALSE, ahah.js happily still processes this response and replaces part of the form.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

also, upload.module has a code comment that drupal_json() should not be used with ahah.js, but both book and poll use drupal_json() - so either the code comment in upload is wrong, or the code in book and poll is wrong.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Upload and Poll modules are including status messages in the returned markup. How does that work?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
check-POST-222588-8.patch11.07 KB

ok, a step towards more complete error checking and consistency, at least in the PHP part

yched’s picture

yched CreditAttribution: yched commented

subscribe

bdragon’s picture

bdragon CreditAttribution: bdragon commented

subscribing

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

Is there some way we cold report back the exact error?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@moshe - it seem like now a couple of these would end up stuffing the raw error messages back into the form. That might be ok for deggging, but doesn't seem like the best approach for end users? Hence my intent to catch the errors before this point.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

patch in #8 no longer applies to 7.x, but does apply to 6.x

trevorwh’s picture

trevorwh CreditAttribution: trevorwh commented

Alright folks...I needed to do something so that I could display drupal messages through the AHAH api.

I've created a demo..take a look and see what you think. It uses existing drupal_set_message() calls, and adds a "messages" portion to the array. If success returns false, the form remains the same.

To test, attempt to enter an affiliate code and see what happens.

https://converge.epsilonlimited.com/cart/view

Let me know your thoughts. This is for a custom module, but can be extended however people would like. I'm happy to release the code.

-Trevor White

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
check-POST-222588-7x-15.patch11.42 KB

here's a re-roll for 7.x - did basic testing of all 3 forms. I think the patch above mostly didn't apply due to changes in the use of the concat operator.

Jaza’s picture

Jaza CreditAttribution: Jaza commented
FileSize
check-POST-222588-7x-16.patch11.67 KB

@pwolanin: at the very start of this issue, you say that the main bug you're trying to eliminate with this patch is:

a user visiting the corresponding path will see lots of ugly errors

The current patch does not solve this. When you type in the AHAH callback and access it directly in a browser, you still get encoded JSON output. If the page is accessed directly rather than through AHAH, it should return something other than an ugly chunk of JSON. I think the most appropriate thing to do is to print 'page not found', since conceptually the page "doesn't exist" if accessed directly.

Attached is an update of the previous patch, with the following code added at the start of the AHAH callbacks for book / poll / upload:

if (empty($_POST)) {
  drupal_not_found();
  exit();
}

If the callback is reached through an asynchronous request, then $_POST will always have something. But if it's reached by the user typing in the URL directly, then $_POST is completely empty. So this seems like a reasonable check to me. $_POST should only be empty for a human visitor - for an AHAH visitor that has supplied invalid data, the JSON 'status' => FALSE should still be outputted.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Hmm, I'd almost rather just have an empty page. And the path exists, so I don't think we want to return a 404 - could that confuse browsers that subsequently want to use the callback?

Actually, looking at this issue more, it's not so much the ugliness of the output that I'm concerned about as potentially invalid data getting added to the cache. My feeling is that a human visitor should not see anything disclosed, but the empty JSON data is pretty innocuous.

In fact, you see something similar if you visit some of the non-AHAH callbacks like drupal.org/user/autocomplete/jaza

dmitrig01’s picture

dmitrig01 CreditAttribution: dmitrig01 commented

What about making a custom access callback, drupal_json_access, that looks like this:

function drupal_json_access($perm) {
  return user_access($perm) && ($_SERVER['X_HTTP_REQUESTED_WITH'] == 'XMLHTTPRequest');
}
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

I'd like to get some basic version of this in D6 - for that we should be making a minimal change to the current behavior. So, to achieve the D6 bugfix, does the current version look ok? Once that's in we can consider features for D7.

paul.lovvik’s picture

paul.lovvik CreditAttribution: paul.lovvik commented

I reviewed the patch under comment #15 on Drupal 7, and didn't see any problems with it. With the patch the message that results from entering a url like http://localhost/drupal/poll/js is a user friendly message rather than json. Not sure how critical this is, but the user experience is clearly much better with the patch.

I ran all of the tests for Drupal 7, and this patch is not causing test failures.

paul.lovvik’s picture

paul.lovvik CreditAttribution: paul.lovvik commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Still applies, no tests harmed.

Heine’s picture

Heine CreditAttribution: Heine commented
Status: Reviewed & tested by the community » Needs work

The changes to D6 need to get in.

Freso’s picture

Freso CreditAttribution: Freso commented

How do changes to D6 affect a patch for D7's status?

That being said, the patch from #16 doesn't seem to apply.

Freso’s picture

Freso CreditAttribution: Freso commented
Issue tags: +Needs tests

Also, we'll want a test to make sure raw JSON isn't re-introduced on the callback pages.