Skip to content

Commit

Permalink
Fixed low risk XSS issue with user account address edit. Thanks to @r…
Browse files Browse the repository at this point in the history 
…obert81 for the find.

Close #3721
  • Loading branch information
@jamesallsup
jamesallsup committed Dec 17, 2015
1 parent 3319a61 commit 303fa88
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions upload/catalog/controller/account/address.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,15 +425,15 @@ protected function getForm() {
}

if (isset($this->request->post['country_id'])) {
$data['country_id'] = $this->request->post['country_id'];
$data['country_id'] = (int)$this->request->post['country_id'];
} elseif (!empty($address_info)) {
$data['country_id'] = $address_info['country_id'];
} else {
$data['country_id'] = $this->config->get('config_country_id');
}

if (isset($this->request->post['zone_id'])) {
$data['zone_id'] = $this->request->post['zone_id'];
$data['zone_id'] = (int)$this->request->post['zone_id'];
} elseif (!empty($address_info)) {
$data['zone_id'] = $address_info['zone_id'];
} else {
Expand Down Expand Up @@ -503,11 +503,11 @@ protected function validateForm() {
$this->error['postcode'] = $this->language->get('error_postcode');
}

if ($this->request->post['country_id'] == '') {
if ($this->request->post['country_id'] == '' || !is_int($this->request->post['country_id'])) {
$this->error['country'] = $this->language->get('error_country');
}

if (!isset($this->request->post['zone_id']) || $this->request->post['zone_id'] == '') {
if (!isset($this->request->post['zone_id']) || $this->request->post['zone_id'] == '' || !is_int($this->request->post['zone_id'])) {
$this->error['zone'] = $this->language->get('error_zone');
}

Expand Down

3 comments on commit 303fa88

@ADDCreative
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamesallsup is_int will always return false as $this->request->post['zone_id'] is a string. is_numeric could be used instead.

There are also a few other places where the zone_id is output without validation. See links below for the places in 1.5.x.
opencart-ce/opencart-ce@7cb8a38
opencart-ce/opencart-ce@dbb7d13
opencart-ce/opencart-ce@9ae8661

The test could also maybe written as below without the test for an empty string, for a small optimisation.

if (!isset($this->request->post['zone_id']) || !is_numeric($this->request->post['zone_id'])) {

@chienhoang
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you use intval function?
http://php.net/manual/en/function.intval.php

@jamesallsup
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ADDCreative - New commit based on feedback 33642ba

Please sign in to comment.