Adds additional escaping throughout plugin (VIP) #1495

tomjn · 2018-05-16T16:20:14Z

There's a number of issues flagged during VIP review, I'm currently going through and fixing them by adding in the missing escaping. WIP PR, questions welcome

…e passed

… script tags

jom · 2018-05-17T22:18:28Z

includes/class-wp-job-manager-ajax.php

@@ -139,7 +139,7 @@ public function get_listings() {
 			'orderby'            => $orderby,
 			'order'              => sanitize_text_field( $_REQUEST['order'] ),
 			'offset'             => ( absint( $_REQUEST['page'] ) - 1 ) * absint( $_REQUEST['per_page'] ),
-			'posts_per_page'     => absint( $_REQUEST['per_page'] ),
+			'posts_per_page'     => max( 1, absint( $_REQUEST['per_page'] ) ),


As a behavior change, I might cherry-pick this commit to a separate PR, if that works for you? There could be some themes doing things they shouldn't with -1.

Sounds good

I started doing this and then realized absint() prevented -1 😉 . This commit could probably be removed.

eh but it does prevent passing 0, I wanted to add a min too so there was an upper limit but i figured that would be problematic

In testing, 0 resulted in the default per_page, but oh well.

…n the ajax filters

jom

Thanks for this PR, @tomjn! Huge improvement. I've added some (mostly linting) notes.

Did some general testing and seems to work well. I've marked it for 1.31.1. I can update the template versions before merging.

jom · 2018-05-23T10:26:54Z

includes/admin/class-wp-job-manager-cpt-legacy.php

@@ -48,18 +48,23 @@ public function __construct() {
 	public function add_bulk_actions_legacy() {
 		global $post_type, $wp_post_types;

+		$bulk_actions = [];


WPJM still has PHP 5.2 compat, unfortunately. Please expand short array syntax to old school array().

jom · 2018-05-23T10:27:04Z

includes/admin/class-wp-job-manager-cpt-legacy.php

@@ -48,18 +48,23 @@ public function __construct() {
 	public function add_bulk_actions_legacy() {
 		global $post_type, $wp_post_types;

+		$bulk_actions = [];
+		foreach( $this->get_bulk_actions() as $key => $bulk_action ) {
+			$bulk_actions[] = [


Same as above.

jom · 2018-05-23T10:28:00Z

includes/admin/class-wp-job-manager-cpt-legacy.php

+		foreach( $this->get_bulk_actions() as $key => $bulk_action ) {
+			$bulk_actions[] = [
+				'key' => $key,
+				'label' => sprintf( $bulk_action[ 'label' ], $wp_post_types[ 'job_listing' ]->labels->name )


Lint: spacing around single apostrophes in array key.

jom · 2018-05-23T10:29:12Z

includes/admin/class-wp-job-manager-cpt.php

@@ -372,16 +371,15 @@ public function jobs_meta_filters() {
 	private function jobs_filter_dropdown( $param, $options ) {
 		$selected = isset( $_GET[ $param ] ) ? $_GET[ $param ] : '';

-		$output = "<select name=\"$param\" id=\"dropdown_$param\">";
+		echo '<select name="'.esc_attr( $param ).'" id="dropdown_'.esc_attr( $param ).'">';


Lint: spacing around . in string concatenation.

jom · 2018-05-23T10:30:48Z

includes/admin/class-wp-job-manager-cpt.php

@@ -753,17 +755,17 @@ public function extend_submitdiv_post_status() {
 			$selected AND $display = $name;

 			// Build the options
-			$options .= "<option{$selected} value='{$status}'>{$name}</option>";
+			$options .= "<option{$selected} value='{$status}'>".esc_html( $name )."</option>";


Lint: spacing around . in string concatenation.

jom · 2018-05-23T10:54:23Z

templates/job-submit.php

@@ -40,7 +40,7 @@

 		<?php foreach ( $job_fields as $key => $field ) : ?>
 			<fieldset class="fieldset-<?php echo esc_attr( $key ); ?>">
-				<label for="<?php echo esc_attr( $key ); ?>"><?php echo $field['label'] . apply_filters( 'submit_job_form_required_label', $field['required'] ? '' : ' <small>' . __( '(optional)', 'wp-job-manager' ) . '</small>', $field ); ?></label>
+				<label for="<?php echo esc_attr( $key ); ?>"><?php echo esc_html( $field['label']) . wp_kses_post( apply_filters( 'submit_job_form_required_label', $field['required'] ? '' : ' <small>' . __( '(optional)', 'wp-job-manager' ) . '</small>', $field ) ); ?></label>


Lint: Spacing around argument list (esc_html( $field['label'])).

jom · 2018-05-23T10:55:28Z

tests/php/bootstrap.php

@@ -144,7 +144,7 @@ public function convert_to_exception( $errno, $errstr, $errfile, $errline ) {

 		// PHP 5.2 doesn't show the error from Exceptions.
 		if ( version_compare( phpversion(), '5.3.0', '<' ) ) {
-			echo "Error ($errno) - $description\n";
+			echo "Error (".esc_html( $errno ) ."( - ".esc_html( $description )."\n";


Lint: spacing around string concatenation (.).

jom · 2018-05-23T10:57:27Z

wp-job-manager-functions.php

@@ -1388,10 +1387,9 @@ function job_manager_duplicate_listing( $post_id ) {
 	/*
 	 * Duplicate post meta, aside from some reserved fields.
 	 */
-	$post_meta = $wpdb->get_results( $wpdb->prepare( "SELECT meta_key, meta_value FROM {$wpdb->postmeta} WHERE post_id=%d", $post_id ) );
+	$post_meta = get_post_meta( $post_id );


Replaced by #1503; please remove from this PR.

(Also line change below)

jom · 2018-05-23T10:59:51Z

wp-job-manager-template.php

@@ -1066,11 +1072,10 @@ function the_company_twitter( $before = '', $after = '', $echo = true, $post = n
 	if ( strlen( $company_twitter ) == 0 )
 		return;

-	$company_twitter = esc_attr( strip_tags( $company_twitter ) );
-	$company_twitter = $before . '<a href="http://twitter.com/' . $company_twitter . '" class="company_twitter" target="_blank">' . $company_twitter . '</a>' . $after;
+	$company_twitter = $before . '<a href="'.esc_url( 'https://twitter.com/' . $company_twitter ) . '" class="company_twitter" target="_blank">' . esc_html( wp_strip_all_tags( $company_twitter ) ) . '</a>' . $after;


Lint: spacing around string concatenation (.).

jom · 2018-05-23T11:03:48Z

templates/account-signin.php

 				<?php endif; ?>

 			<?php elseif ( $account_required ) : ?>

-				<?php echo apply_filters( 'submit_job_form_login_required_message',  __('You must sign in to create a new listing.', 'wp-job-manager' ) ); ?>
+				<?php echo wp_kses_post( apply_filters( 'submit_job_form_login_required_message',  __('You must sign in to create a new listing.', 'wp-job-manager' ) ) ); ?>


This was an original lint issue, but while editing line, there is a spacing issue in the argument list for __().

jom · 2018-05-23T11:17:09Z

cc: @tripflex I don't think the escaping happening in the fields/*.php templates will affect your plugin, but just wanted to flag you.

issues resolved, needs re-reviewing

jom · 2018-06-19T16:06:41Z

assets/js/ajax-filters.js

@@ -122,7 +123,8 @@ jQuery( document ).ready( function ( $ ) {
 				if ( result ) {
 					try {
 						if ( result.showing ) {
-							$( showing ).show().html( '<span>' + result.showing + '</span>' + result.showing_links );
+							var showing_el = jQuery('<span>').text(result.showing)
+							$( showing ).show().html( '').text(result.showing_links).prepend(showing_el);


result.showing_links contains HTML and gets encoded using $.text().

jom · 2018-06-19T16:22:45Z

I've force pushed a modified version of that commit with #1503's content removed. I want to test that change with WPML before merging and don't want it to hold up this commit. For further changes, you'll need to do a hard reset to the remote branch.... sorry, since I'm planning on squashing this PR anyway I could have just done a partial revert commit.

Can we revert the change to line 125 in ajax-filters.js? I don't mind doing it if you're fine with that. Once we do that, I'll update the template versions for 1.31.1.

There were a few more lint errors not resolved yet but I did them in 09fc708.

tomjn · 2018-06-20T22:12:51Z

@jom my bandwidth at the moment is quite tight, and we've a client waiting on these changes for quite a while now, anything you can do to expedite things would be super helpful

jom · 2018-06-20T22:15:15Z

@tomjn I'll revert the change mentioned in ajax-filters.js so the raw HTML doesn't show up (unless you have another preferred method) and then we should be good for this PR.

tomjn added 6 commits May 16, 2018 16:50

escaped an email variable

fc242fe

added escaping to a job title

b8e1c7d

added escaping

b1f05b6

fixed a php error and escaping in an error outputter

0d19e9c

fixed missing ecaping in the category walker class

99a0c23

escaping fixes and simplified some javascript generation

974c282

tomjn added [Type] Bug [Status] In Progress [Type] Cleanup and Documentation [Type] Maintenance Changes to non-production files labels May 16, 2018

tomjn self-assigned this May 16, 2018

tomjn added 10 commits May 17, 2018 20:45

sets a minimum posts per page on the ajax endpoints so that -1 cant b…

0f58c5d

…e passed

adds escaping

6f95b08

escapes the html license views

aba6d5c

fixes escaping issues

9365fe2

fixes some html string construction issues in the tip tip libbrary

ca65f50

adds escaping and fixes the use of strip tags, it doesnt strip css or…

95ee074

… script tags

escaping fixes

8d55576

adds escaping

fde8623

escaping fixes

00e4931

escaping fixes

93cd132

jom reviewed May 17, 2018

View reviewed changes

tomjn added 3 commits May 19, 2018 00:19

escaping fixes

432cfe7

escaping fixes

aeba75f

fixes html string construction by changing to dom node construction i…

a2761bd

…n the ajax filters

jom mentioned this pull request May 23, 2018

Add missing domains and untranslated strings in license view file #1505

Closed

jom previously requested changes May 23, 2018

View reviewed changes

jom added this to the 1.31.1 milestone May 23, 2018

Tom J Nowell and others added 11 commits June 19, 2018 13:32

fixes missing spacing lint issue

fed67b7

fixes missing spacing lint issue

c707ef7

fixes missing spacing lint issue

00b9eed

fixes missing spacing lint issue

02d0de0

fully expanded expansions

f607218

fixes spacing linting issues

dc48068

fixes spacing linting around concatenators

a0b76c7

PHP 5.2 compat

5e74f62

linting fixes

723704d

coding standards fixes

1d05acc

Undo optimisation that is now in a separate PR

874b306

jom force-pushed the vip_escaping branch 2 times, most recently from 1198a46 to d72e567 Compare June 19, 2018 14:58

Add formatting fixes for escaping changes

09fc708

jom force-pushed the vip_escaping branch from 1c02883 to 09fc708 Compare June 19, 2018 16:04

jom reviewed Jun 19, 2018

View reviewed changes

jom added 3 commits June 22, 2018 10:20

Fix HTML encoding for links in the [jobs] filter

795e84f

Rebuild modified JS assets

a915942

Update template version numbers

0a073de

jom force-pushed the vip_escaping branch from 63d49fa to 0a073de Compare June 22, 2018 08:22

jom changed the title ~~Vip escaping~~ Adds additional escaping throughout plugin (VIP) Jun 22, 2018

jom merged commit 55197d5 into master Jun 22, 2018

jom deleted the vip_escaping branch June 22, 2018 08:34

This was referenced Jun 27, 2018

Don't escape sign in template message entirely of HTML #1532

Merged

PHPCS code formatting fixes #1534

Merged

jom mentioned this pull request Jul 4, 2018

1.31.1 Release #1539

Closed

6 tasks

jom mentioned this pull request Apr 24, 2019

Allow embed <iframe> in job description content #1722

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds additional escaping throughout plugin (VIP) #1495

Adds additional escaping throughout plugin (VIP) #1495

tomjn commented May 16, 2018

jom May 17, 2018

tomjn May 17, 2018

jom May 17, 2018 •

edited

Loading

tomjn May 19, 2018

jom May 22, 2018

jom left a comment •

edited

Loading

jom May 23, 2018

tomjn Jun 4, 2018

jom May 23, 2018

tomjn Jun 4, 2018

jom May 23, 2018

jom May 23, 2018

jom May 23, 2018

tomjn Jun 4, 2018

jom May 23, 2018

tomjn Jun 4, 2018

jom May 23, 2018

tomjn Jun 4, 2018

jom May 23, 2018

jom May 23, 2018

tomjn Jun 4, 2018

jom May 23, 2018

tomjn Jun 4, 2018

jom commented May 23, 2018

jom Jun 19, 2018 •

edited

Loading

jom commented Jun 19, 2018 •

edited

Loading

tomjn commented Jun 20, 2018

jom commented Jun 20, 2018

Adds additional escaping throughout plugin (VIP) #1495

Adds additional escaping throughout plugin (VIP) #1495

Conversation

tomjn commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jom May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jom left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jom commented May 23, 2018

jom Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

jom commented Jun 19, 2018 • edited Loading

tomjn commented Jun 20, 2018

jom commented Jun 20, 2018

jom May 17, 2018 •

edited

Loading

jom left a comment •

edited

Loading

jom Jun 19, 2018 •

edited

Loading

jom commented Jun 19, 2018 •

edited

Loading