Fix/secuirty issues by syedgalib · Pull Request #1030 · sovware/directorist

syedgalib · 2022-07-19T09:55:04Z

No description provided.

obiPlabon

There are more issues similar to the feedback I shared. Please check them

obiPlabon · 2022-07-19T12:09:43Z

directorist-base.php

 		?>
 		<div class="atbd_rated_stars">
-			<?php echo ATBDP()->review->print_static_rating($average); ?>
+			<?php echo esc_html( ATBDP()->review->print_static_rating( $average ) ); ?>


this method returns html and doesn't need to be escaped.

obiPlabon · 2022-07-19T12:14:40Z

includes/asset-loader/localized_data.php

 		$listing_id  = 0;
-		$current_url = $current_url="//".$_SERVER['HTTP_HOST'].$_SERVER['REQUEST_URI'];
-		$current_listing_type = isset( $_GET['directory_type'] ) ? $_GET['directory_type'] : get_post_meta( $listing_id, '_directory_type', true );
+		$current_url = $current_url="//". sanitize_text_field( $_SERVER['HTTP_HOST'] ) . sanitize_text_field( $_SERVER['REQUEST_URI'] );


I think there's a typo $current_url = $current_url="//"

Looks like we don't need the HTTP_HOST here only the REQUEST_URI is enough. Please recheck...

obiPlabon · 2022-07-19T12:21:27Z

includes/asset-loader/localized_data.php

 				'please_login'          => __( 'Please login first', 'directorist' ),
 				'select_listing_map' => $select_listing_map,
-				'Miles'              => !empty( $_GET['miles'] ) ? $_GET['miles'] : $miles,
+				'Miles'              => ! empty( $_GET['miles'] ) ? sanitize_text_field( $_GET['miles'] ) : $miles,


$_GET['miles'] is an integer whereas $miles is a string. Looks like the translation string in incorrect. Supposed to be $miles_text = __( '%d Miles', 'directorist' ); $miles = ! empty( $_GET['miles'] ) ? absint( $_GET['miles'] ) : 0; and `'Miles' => sprintf( $miles_text, $miles )

obiPlabon · 2022-07-19T12:22:29Z

includes/data-store/class-atbdp-listing-store.php

-			$action    = ! empty( $_GET['atbdp_action'] ) ? $_GET['atbdp_action'] : '';
-			$listing_id  = ! empty( $_GET['atbdp_listing'] ) ? absint( $_GET['atbdp_listing'] ) : 0;
+			$action    = ! empty( $_GET['atbdp_action'] ) ? sanitize_text_field( $_GET['atbdp_action'] ) : '';
+			$listing_id  = ! empty( $_GET['atbdp_listing'] ) ? absint( sanitize_text_field( $_GET['atbdp_listing'] ) ) : 0;


sanitize_text_field unnecessary

obiPlabon · 2022-07-19T12:25:25Z

includes/modules/appsero/src/Insights.php

        echo '</p></div>';

-        echo "<script type='text/javascript'>jQuery('." . $this->client->slug . "-insights-data-we-collect').on('click', function(e) {
+        echo "<script type='text/javascript'>jQuery('." . esc_attr( $this->client->slug ) . "-insights-data-we-collect').on('click', function(e) {


use esc_js for all inline and internal js

obiPlabon · 2022-07-20T03:03:41Z

includes/review/class-comment-form-processor.php

 			Comment::clear_transients( $comment->comment_post_ID );

-			$cpage = isset( $_POST['cpage'] ) ? absint( $_POST['cpage'] ) : 0;
+			$cpage = isset( $_POST['cpage'] ) ? absint( sanitize_text_field( $_POST['cpage'] ) ) : 0;


absint( $_POST['cpage'] )

obiPlabon · 2022-07-20T03:10:47Z

includes/review/class-comment-form-processor.php

 			}

-			if ( empty( $_POST['comment'] ) || empty( trim( $_POST['comment'] ) ) ) {
+			if ( empty( $_POST['comment'] ) || empty( trim( sanitize_text_field( $_POST['comment'] ) ) ) ) {


It's a textarea field so using sanitize_text_field will remove newline characters.

$comment = isset( $_POST['comment'] ) ? trim( sanitize_textarea_field( $_POST['comment'] ) : ''; if ( empty( $comment ) ) { .... } // And online line 83-87 $comment_data = array( ... ... 'comment_content' => $comment );

obiPlabon · 2022-07-20T03:12:27Z

includes/review/class-comment-form-renderer.php

-			$nonce      = ! empty( $_REQUEST['nonce'] ) ? $_REQUEST['nonce'] : '';
-			$post_id    = ! empty( $_REQUEST['post_id'] ) ? absint( $_REQUEST['post_id'] ) : 0;
-			$comment_id = ! empty( $_REQUEST['comment_id'] ) ? absint( $_REQUEST['comment_id'] ) : 0;
+			$nonce      = ! empty( $_REQUEST['nonce'] ) ? sanitize_text_field( $_REQUEST['nonce'] ) : '';


wp_unslash( $_REQUEST['nonce'] )

obiPlabon · 2022-07-20T03:12:46Z

includes/review/class-comment-form-renderer.php

-			$post_id    = ! empty( $_REQUEST['post_id'] ) ? absint( $_REQUEST['post_id'] ) : 0;
-			$comment_id = ! empty( $_REQUEST['comment_id'] ) ? absint( $_REQUEST['comment_id'] ) : 0;
+			$nonce      = ! empty( $_REQUEST['nonce'] ) ? sanitize_text_field( $_REQUEST['nonce'] ) : '';
+			$post_id    = ! empty( $_REQUEST['post_id'] ) ? absint( sanitize_text_field( $_REQUEST['post_id'] ) ) : 0;


absint( $_REQUEST['post_id'] )

obiPlabon · 2022-07-20T03:13:08Z

includes/review/class-comment-form-renderer.php

-			$comment_id = ! empty( $_REQUEST['comment_id'] ) ? absint( $_REQUEST['comment_id'] ) : 0;
+			$nonce      = ! empty( $_REQUEST['nonce'] ) ? sanitize_text_field( $_REQUEST['nonce'] ) : '';
+			$post_id    = ! empty( $_REQUEST['post_id'] ) ? absint( sanitize_text_field( $_REQUEST['post_id'] ) ) : 0;
+			$comment_id = ! empty( $_REQUEST['comment_id'] ) ? absint( sanitize_text_field( $_REQUEST['comment_id'] ) ) : 0;


absint( $_REQUEST['comment_id'] )

kowsar89

phpcs issue found

kowsar89 · 2022-07-24T08:34:28Z

Get help from:
https://github.com/WordPress/WordPress-Coding-Standards/wiki/Fixing-errors-for-input-data
https://github.com/WordPress/WordPress-Coding-Standards/wiki/Sanitizing-array-input-data
https://github.com/WordPress/WordPress-Coding-Standards/wiki/Escaping-a-WP_Error-object

kowsar89

phpcs issue found

kowsar89

phpcs issue found

kowsar89 · 2022-07-26T09:23:23Z

includes/asset-loader/localized_data.php

 		$radius_search_unit      = get_directorist_option( 'radius_search_unit', 'miles' );
 		$default_radius_distance = get_directorist_option( 'search_default_radius_distance', 0 );

+		$miles = ! empty( $_GET['miles'] ) ? absint( $_GET['miles'] ) : 0;


improvements should go into different PR, only security fixes here

vairafiq

Looks good

vairafiq

Review broke in single listing

vairafiq · 2022-08-01T12:25:07Z

https://www.loom.com/share/7f0bbb8b1ddb497eb97632fe92e352b8

vairafiq

Tested, works fine

kowsar89

phpcs ok

kowsar89

PHP error found

[02-Aug-2022 06:46:52 UTC] PHP Parse error:  syntax error, unexpected ')' in /media/k/SSD/xampp/htdocs/wpwax/directorist/wp-content/plugins/directorist/includes/helper-functions.php on line 9009
[02-Aug-2022 06:46:52 UTC] PHP Fatal error:  Exception thrown without a stack frame in Unknown on line 0

obiPlabon

ok

syedgalib added 4 commits July 17, 2022 16:38

added escaping

fefd89c

applied escaping and sanitizing

8c4918c

applied escaping and sanitizing

951fc06

applied escaping and sanitizing

770dc6c

syedgalib requested review from obiPlabon and vairafiq as code owners July 19, 2022 09:55

obiPlabon suggested changes Jul 20, 2022

View reviewed changes

kowsar89 suggested changes Jul 20, 2022

View reviewed changes

kowsar89 added the Coding label Jul 20, 2022

kowsar89 added this to the v7.3.1 milestone Jul 20, 2022

syedgalib added 3 commits July 25, 2022 18:53

improved sanitization

30a71fe

added wpcs config file

9a93f38

improved sanitization

36e6d9c

kowsar89 suggested changes Jul 26, 2022

View reviewed changes

syedgalib added 7 commits July 26, 2022 10:27

improved sanitization

f6b12fd

removed phpcs config file

a5c8163

improved sanitization

3923781

code optimised

f9027fd

added script version

e31cce3

updated gitignore

a426841

improved sanitization

85ebcdc

kowsar89 added the Type: Security label Jul 26, 2022

kowsar89 suggested changes Jul 26, 2022

View reviewed changes

syedgalib added 6 commits July 26, 2022 18:54

improved sanitization

e37fc26

improved sanitization

b68d576

improved sanitization

96f3a26

improved sanitization

c9ef213

improved sanitization

111c887

updated gitignore

da93a85

syedgalib added 7 commits July 27, 2022 15:54

fixed typo

c29e524

improved sanitization

50fc9a8

fixed typo

abb02cc

fixed merge conflict

1a5177f

reverted to old state

b0541e3

reverted to old state

d929c44

reverted to old state

dc22189

vairafiq approved these changes Jul 31, 2022

View reviewed changes

vairafiq requested changes Jul 31, 2022

View reviewed changes

syedgalib added 6 commits July 31, 2022 13:19

improved sanitization

f11e3bc

fixed merge conflict

e9beb93

improved sanitization

3754781

improved sanitization

6647b8a

updated phpcs config file

9761056

improved sanitization

6e004f5

improved sanitization

3b6e4bd

vairafiq previously approved these changes Aug 2, 2022

View reviewed changes

kowsar89 approved these changes Aug 2, 2022

View reviewed changes

kowsar89 suggested changes Aug 2, 2022

View reviewed changes

fixed php error

de43c77

syedgalib dismissed vairafiq’s stale review via de43c77 August 2, 2022 07:12

kowsar89 approved these changes Aug 2, 2022

View reviewed changes

obiPlabon approved these changes Aug 2, 2022

View reviewed changes

obiPlabon merged commit 677f59f into sovware:alpha Aug 2, 2022

Conversation

syedgalib commented Jul 19, 2022

Uh oh!

obiPlabon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kowsar89 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kowsar89 commented Jul 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kowsar89 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kowsar89 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vairafiq left a comment

Choose a reason for hiding this comment

Uh oh!

vairafiq left a comment

Choose a reason for hiding this comment

Uh oh!

vairafiq commented Aug 1, 2022

Uh oh!

vairafiq left a comment

Choose a reason for hiding this comment

Uh oh!

kowsar89 left a comment

Choose a reason for hiding this comment

Uh oh!

kowsar89 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obiPlabon left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kowsar89 left a comment •

edited

Loading

kowsar89 commented Jul 24, 2022 •

edited

Loading

kowsar89 left a comment •

edited

Loading

kowsar89 left a comment •

edited

Loading

kowsar89 left a comment •

edited

Loading