WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 6 months ago

#24907 reopened defect (bug)

Escape admin_url() when used for ajax_url in admin header

Reported by: jeremyfelt Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 2.7
Component: Security Keywords: has-patch needs-testing
Focuses: Cc:
PR Number:

Description

As admin_url() is filtered right before returning, it should be escaped when output for use as the ajax_url in the admin.

Attachments (3)

24907.diff (780 bytes) - added by jeremyfelt 6 years ago.
24907.2.patch (1.4 KB) - added by c3mdigital 6 years ago.
json encode javascript variables output in admin header
24907.3.patch (1.4 KB) - added by aliso 6 years ago.
Adding esc_js to variable values to use in place of json_encode

Download all attachments as: .zip

Change History (10)

@jeremyfelt
6 years ago

#1 @c3mdigital
6 years ago

I think we should be json encoding PHP string variables when outputting to javascript. This eliminates the need for escaping and surrounding with quotes.

Results of json_encode patch on output in header:

var ajaxurl = "\/wp-admin\/admin-ajax.php",
	pagenow = "edit-post",
	typenow = "post",
	adminpage = "edit-php",
	thousandsSeparator = ",",
	decimalPoint = ".",
	isRtl = 0;
Last edited 6 years ago by c3mdigital (previous) (diff)

@c3mdigital
6 years ago

json encode javascript variables output in admin header

#2 @nacin
6 years ago

esc_url() isn't right, as it encodes ampersands for display. At most you'd want esc_url_raw() — but really, we're just looking to avoid issues with escaping data for a JS string.

@aliso
6 years ago

Adding esc_js to variable values to use in place of json_encode

#3 @helen
6 years ago

esc_js() seems sensible.

#4 @azaozz
6 years ago

esc_js() would work but is intended for escaping of inline JS. The _wp_specialchars() used there could break it. Don't think we have a suitable esc_* function when we echo arbitrary PHP strings inside a <script> tag.

#5 @chriscct7
4 years ago

  • Keywords needs-testing added

#7 @iandunn
10 months ago

  • Status changed from new to reopened

Reopening being this still seems like useful hardening.

#8 @SergeyBiryukov
8 months ago

  • Milestone set to Awaiting Review
Note: See TracTickets for help on using tickets.