WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#12122 closed defect (bug) (fixed)

Unchecked Indexes in admin-ajax.php

Reported by: miqrogroove Owned by: westi
Milestone: 3.0 Priority: normal
Severity: minor Version: 2.9
Component: Warnings/Notices Keywords: has-patch tested commit
Focuses: Cc:

Description

Notices produced by URL guessing.

Notice: Undefined index: action in /wp-admin/admin-ajax.php on line 25

Notice: Undefined index: action in /wp-admin/admin-ajax.php on line 204

Notice: Undefined index: action in /wp-admin/admin-ajax.php on line 1428

Attachments (2)

12122.diff (463 bytes) - added by sivel 5 years ago.
Since admin-ajax.php requires $_REQUESTaction? just die early on if it's not set
12122.2.diff (712 bytes) - added by sivel 5 years ago.
Same as before but adds an additional isset

Download all attachments as: .zip

Change History (10)

@sivel5 years ago

Since admin-ajax.php requires $_REQUESTaction? just die early on if it's not set

comment:1 @sivel5 years ago

  • Keywords has-patch added

comment:2 @miqrogroove5 years ago

umm.. if wp_die() isn't appropriate for AJAX, could we at least put an HTTP status in there?

comment:3 @azaozz5 years ago

If the server was reached and responded the HTTP status should stay at 200.

I like the proposed patch no need to proceed if action is not set.

comment:4 @miqrogroove5 years ago

I don't like web spiders seeing status 200 on random URLs.

comment:5 @sivel5 years ago

  • Keywords tested commit added

If we sent any status at all the preferred status would be 400. However to properly send a 400 we would need to move the check after the call to wp-load.php so that we could use status_header() or we end up duplicating code.

Honestly I still prefer to just die with a -1 since that is what we already do when the action is not hooked. Otherwise we start sending inconsistent results back.

comment:6 @nacin5 years ago

With this we can probably remove an isset() check or two later in the file.

Also, it looks like we check for !empty() for wp_ajax_nopriv_ but only isset() for wp_ajax_. Standardizing this on !empty() up top would be good. I don't see a reason for firing the literal hook "wp_ajax_" if admin-ajax.php?action is called.

comment:7 @nacin5 years ago

Scratch that, we'll still need the isset() check around $_GETaction?, as it may be $_POST.

That also means this won't fix everything. Checking for $_REQUESTaction? at the top may still generate a notice on line 25 as $_POSTautosave? may not be set, because it may be GET.

I like the strategy though.

@sivel5 years ago

Same as before but adds an additional isset

comment:8 @nacin5 years ago

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

(In [13175]) Fix notices, props sivel. Fixes #12122

Note: See TracTickets for help on using tickets.