Comment On Discount calculation

Either your second hint has the wrong formula, or your article does.

Fixed - ed.

the hint does.

-100 + (x * 100)/x

The block where division takes place is CONDITIONED OUT :)

if(a != 0)
b = 100 * a/a - 100;
else
b = 100;

would fix those pesky problems about representing rational numbers in floating point.

s.:

The block where division takes place is CONDITIONED OUT :)

if(a != 0)
b = 100 * a/a - 100;
else
b = 100;

but that's not what the code above does... the code above is :

if (b != 0)
b = 100 * a / a - 100


blah blah blah

s.:

The block where division takes place is CONDITIONED OUT :)

if(a != 0)
b = 100 * a/a - 100;
else
b = 100;

Except its more like:

if (c != 0)
b = a/a * 100 - 100;
else
b = 100;

So infact, checking for 0 doesn't in any way protect you. If I did my math right, if the discount_price is anything but 0, you get $discount = 0; otherwise you get $discount = 100 ?? That's what I'm coming up with.

(x * 100) / x = 100 so the discount will always be 0 or 100.

But why - in the first place - would you calculate discount based on the price? If their plan was that a $20 product would have a 20% discount, and a $40 product a 40% discount... I'd always buy the most expensive product (with a minimum price of $100).

The biggest WTF (I won't say real, because they are all real) is that you have no idea of knowing where the $_REQUEST value originated from. One should always use $_POST, $_GET, $_COOKIE, etc.

Yes, yes. Definitely get the list price and discount from one of $_POST, $_GET, $_COOKIE... and let me know as soon as your online store is open for business! I'd love a $0.01 shopping-spree. ;)

Coditor:

The biggest WTF (I won't say real, because they are all real) is that you have no idea of knowing where the $_REQUEST value originated from. One should always use $_POST, $_GET, $_COOKIE, etc.

Better yet, store the discounts in the database, take the selected discount ID from the request and validate against the selected item(s) server-side. That way no amount of client-side data manipulation can screw you up.

Coditor:

The biggest WTF (I won't say real, because they are all real) is that you have no idea of knowing where the $_REQUEST value originated from. One should always use $_POST, $_GET, $_COOKIE, etc.

$_POST/$_GET is no more secure than $_REQUEST.

Welcome to anything written in PHP!

Does the OP know what the code was meant to do? Was it really meant to be 0 or 100 discount value?

Obviously the security is a bigger WTF, because if it worked differetly then people could rob the store blind.

If I were to guess then I would put the discount calculation to be ((x-1)*100)/x

I mean if I were to guess what the code was intended to do, not what it actually does.

It seems the code is intended to figure out the percentage of discount the customer is receiving (assumedly for display purposes), given the discounted price and the original list price. Aside from all the problems with $_REQUEST and stuff, the equation should be:

(discounted_price / list_price) * 100

Unless the discounted price is $0.00 (and the list price isn't), in which case the discount should be 100%...

Addendum (2008-04-03 14:58):
My mistake: ((list_price - discounted_price) / list_price) * 100

$_POST is a BIT more secure, since you can't just type random crap in the URL to mess it up.

e.g. http://www.argos.co.uk/static/Product/partNumber/7300624/Trail/C%24cip%3D1500012028.Baby%2Band%2Bnursery%3EC%24cip%3D1500012068.Clearance%2Bbaby%2Band%2Bnursery.htm

I don't have anything to say about this post, just that it's my first one since I registered and I'm happy, now I have something to put on my curriculum :) And watch! No CAPTCHA!

$_POST is a BIT more secure, since you can't just type random crap in the URL to mess it up.

e.g. http://www.argos.co.uk/static/Product/partNumber/7300624/Trail/C%24cip%3D1500012028.Baby%2Band%2Bnursery%3EC%24cip%3D1500012068.Clearance%2Bbaby%2Band%2Bnursery.htm

Well, that's only really security by obscurity. Simply don't pass data that could cause harm if changed by the user (i.e. prices) through the client and back again -- store them on the server, and only display them to the client.

Wrong Request Syntax

Correct way:
$value = $_REQUEST['key'];

Request is simular to $_POST and $_GET, Depends on a forum submit type. The way accsessing is as a Single Dimension, not multiple.

And the other is that discound formula will be always zero

Dude:

Wrong Request Syntax

Correct way:
$value = $_REQUEST['key'];

Request is simular to $_POST and $_GET, Depends on a forum submit type. The way accsessing is as a Single Dimension, not multiple.

That's nonsense, $_REQUEST is an "array" like any other "array" in PHP (they're actually more like hashes, but whatever). It can be multidimensional no problem.

In fact, posting the following:
<input name="foo[0][1][2]" value="bar" type="hidden"/>
will give you a nice
$_POST['foo'][0][1][2] == 'bar'
var in your receiving page.

Of course, it doesn't validate against any DOCTYPE I know of (square brackets aren't allowed in name-attributes) but that's another story altogether.

Validates fine against XHTML 1.1

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><title>Test</title></head>
<body>
<form action="/">
<p><input name="test['field'][0][1]"/></p>
</form>
</body>
</html>

elias:

the equation should be:

(discounted_price / list_price) * 100

Unless the discounted price is $0.00 (and the list price isn't), in which case the discount should be 100%...

Right. So a $40 item for $30 is a 75% discount, while getting the same item for $20 ($10 less) is a 50% discount. Getting it for $10 is a 25% discount. But getting it for $0... 100% discount.

Errrm...

Please inform your coworkers about this site; I look forward to some good postings!

There are at least 2 REAL problems with this code
1) trusting that the value in the post/querystring is a number; it's easy to spoof the data entry form.
2) Mixing a business rule (calculating the discount) with code that processes the values posted to the webpage. These should be in separate layes (and perhaps even written in different languages).

NeoMojo:

Does the OP know what the code was meant to do?

Meant, yes.

NeoMojo:

Was it really meant to be 0 or 100 discount value?

No, not really. It's what it did though.

NeoMojo:

Obviously the security is a bigger WTF, because if it worked differetly then people could rob the store blind.

Not really. This isn't from a store website. It's from a backend where store managers set directly the discount prices directly for their resellers.
If they what to set 100% discount (or higher, for that matter) it's their prorrogative.

anonymous:

$_POST is a BIT more secure, since you can't just type random crap in the URL to mess it up.

e.g. http://www.argos.co.uk/static/Product/partNumber/7300624/Trail/C%24cip%3D1500012028.Baby%2Band%2Bnursery%3EC%24cip%3D1500012068.Clearance%2Bbaby%2Band%2Bnursery.htm

Well, that's only really security by obscurity. Simply don't pass data that could cause harm if changed by the user (i.e. prices) through the client and back again -- store them on the server, and only display them to the client.

A smack to the back of the head for both of you. POST is not security by obscurity, it's not security of any type. Using a password is an example of security by obscurity. Switching from GET to POST is like switching from 2 bullets in the gun to 1 when playing Russian Roulet with yourself.

elias:

It seems the code is intended to figure out the percentage of discount the customer is receiving (assumedly for display purposes)

You get the cake!

elias:

(discounted_price / list_price) * 100

You forgot to subtract from 1 (or 100). I'll have that cake back!

I walked the dinosaur:

Welcome to anything written in PHP!

So it's php's fault the programmer is an idiot? By that logic every language blows.

ben:

Validates fine against XHTML 1.1

I stand corrected. However did I get that idea? Or maybe it were the IDs then...

Oh well. In any case: they can be multidimensional arrays in PHP anyway. :-)

(Thanks for pointing that out to me!)

Do you not see
$value = floatval($_REQUEST['list_price'][$key]);

derobert:

elias:

the equation should be:

(discounted_price / list_price) * 100

Unless the discounted price is $0.00 (and the list price isn't), in which case the discount should be 100%...

Right. So a $40 item for $30 is a 75% discount, while getting the same item for $20 ($10 less) is a 50% discount. Getting it for $10 is a 25% discount. But getting it for $0... 100% discount.

Errrm...

Please inform your coworkers about this site; I look forward to some good postings!

Ah, right, sorry.

((list_price - discounted_price) / list_price) * 100

Everyone knows it should be:

if (c != 0)
b = (a * 100)*(7/a)*(6/100);
else
b = 42;

Ok, some other WTFs about this that people seem to be missing:

if($_REQUEST['discount_price'][$key] != 0) //avoid division by 0

{

    $value = floatval($_REQUEST['list_price'][$key]);

    $discount = - 100 + ($value * 100) / $value;

    $discount = round($discount, 2);

}

... So the value being tested for zero isn't used in the equation regardless.

Also rather striking:

else

    $discount = 100;

So if the discount_price is zero, ie. there is no discount, then there is a discount of... 100? Also, discounts computed in the above equation are negative, so does this mean if there's no discount_price then the price is inflated by 100 of whatever unit? WTF...?!

Dan.

dillybar1:

So it's php's fault the programmer is an idiot? By that logic every language blows.

You must be new here!

thinking that using $_GET, $_POST, $_REQUEST, $_COOKIE are security risks DIRECTLY shows you know absolutely nothing about the PHP language.

register_globals = on; is a security risk.

$query = "SELECT * FROM something WHERE name='" . $_REQUEST['foo'] . "'"; is a security risk

$query = "SELECT * FROM something WHERE name='" . mysql_real_escape_string($_REQUEST['foo']) . "'"; is NOT a security risk

it's not the $_-globals that are a security risk. it's MORON PROGRAMMERS who don't know when they need to do something to user input to prevent injection attacks that are a security risk.

Vombatus:

dillybar1:

So it's php's fault the programmer is an idiot? By that logic every language blows.

You must be new here!

Lot of ASP/C#/.NET/SQL Server loving trolls around. I blame Alex for writing this site in ASP (that's a WTF right there).

ASP/C#/.NET aren't so bad.. it's just they're platform-locked.

SQL Server on the other hand is a pile... *cough*lacking*hack*offset*cough*keyword*cough*

PHP-Coder:

$query = "SELECT * FROM something WHERE name='" . $_REQUEST['foo'] . "'"; is a security risk

$query = "SELECT * FROM something WHERE name='" . mysql_real_escape_string($_REQUEST['foo']) . "'"; is NOT a security risk

But a language that makes it far easier to write code that is a security risk than not (mysql_real_escape_string? Yowch! Mnemonic or what?) has got to be at least an attractive nuisance when it comes to the security-risk department. Alas, most languages don't make doing the right thing easy enough...

in my math universe -100 + (x * 100)/x = -100 + 100 = 0. Huge discount.

dkf:

PHP-Coder:

$query = "SELECT * FROM something WHERE name='" . $_REQUEST['foo'] . "'"; is a security risk

$query = "SELECT * FROM something WHERE name='" . mysql_real_escape_string($_REQUEST['foo']) . "'"; is NOT a security risk

But a language that makes it far easier to write code that is a security risk than not (mysql_real_escape_string? Yowch! Mnemonic or what?) has got to be at least an attractive nuisance when it comes to the security-risk department. Alas, most languages don't make doing the right thing easy enough...

I don't really agree with that statement totally. mysql_real_escape_string() is not that hard to remember.

The part I do agree on though is that PHP is far to relaxed (same argument for VB) and so it allows people to write code that belongs on this website.

And I'm a guy who likes PHP. I just hate how lenient it is.

PHP-Coder:

register_globals = on; is a security risk.

No it's not, unless you're one of *those* PHP coders...

This page will explain how one can write insecure code with this directive but keep in mind that the directive itself isn't insecure but rather it's the misuse of it.

dillybar1:
register_globals = on; is a security risk.

Register globals is only a security risk if you're not careful. It is possible to write secure code with it on.

dillybar1:
So it's php's fault the programmer is an idiot? By that logic every language blows.

No, but it is is PHP's fault for making it easy for idiots to write bad code, rather than it being secure by default.

The math was supposed to be something to the effect of

$discount = - 100 + ($_REQUEST['discount_price'][$key] * 100) / $_REQUEST['list_price'][$key];

but got messed up...
It even would be no surprise if it was a mistake made by the submitter.

The fact that they blindly trust the user-supplied $_REQUEST might be a WTF of its own if it happens in other parts of the code, but here it likely isn't as this $discount is probably only used for display purposes.

Either way, this isn't quite worthy to be on this site.

code monkey:

So infact, checking for 0 doesn't in any way protect you. If I did my math right, if the discount_price is anything but 0, you get $discount = 0; otherwise you get $discount = 100 ?? That's what I'm coming up with.

If you don't do any division further on, then yes it doesn't.

But note that $discount = $value/value; always equals 1 except for $value equal 0 when it triggers a division/0 error. This is the error the if() protects against.

lets lose all but the last line...

I bet he or she meant:

$discount = ($discount > 100)?$list_price/$discount_price*100:100;

PHP-Coder:

thinking that using $_GET, $_POST, $_REQUEST, $_COOKIE are security risks DIRECTLY shows you know absolutely nothing about the PHP language.

I believe that the original poster's point was not that $_GET et al are ALWAYS security risks -- clearly you have to get data from the user somehow -- but rather that getting something like a product price from the submitted web page is a security risk, because no matter where you think that value is supposed to come from, someone could always create his own form to submit to your site where he put whatever values he wanted in there.

(In this case the poster of the original story has explained that this was an internal web site where the users are authorized to enter whatever prices they want to anyway, so the criticism is inapplicable.)

gabba:

How does checking the discount price for zero prevent a divide by zero when dividing by the list price?

Because they converted it to a float first....DUH. :)

I think it was supposed to be:

if($_REQUEST['discount_price'][$key] != 0) //avoid division by 0
{
$value = floatval($_REQUEST['list_price'][$key]);
$discount = - 100 + ($value * 100) / $_REQUEST['discount_price'][$key];
$discount = round($discount, 2);
}
else
$discount = 100;

So $discount is rather an additional amount of goods you get for free, than the amount of price you don't have to pay.
As in commercials, when they say that you'll get 25% more of chocolate for the same money.
Actually it should be $discount=INFINITY; instead of $discount=100; but normal people do not get it.

obediah:

anonymous:

$_POST is a BIT more secure, since you can't just type random crap in the URL to mess it up.

e.g. http://www.argos.co.uk/static/Product/partNumber/7300624/Trail/C%24cip%3D1500012028.Baby%2Band%2Bnursery%3EC%24cip%3D1500012068.Clearance%2Bbaby%2Band%2Bnursery.htm

Well, that's only really security by obscurity. Simply don't pass data that could cause harm if changed by the user (i.e. prices) through the client and back again -- store them on the server, and only display them to the client.

A smack to the back of the head for both of you. POST is not security by obscurity, it's not security of any type.

One valid definition of "security by obscurity" means something that makes it more difficult for casual users to break in, while not adding any real security.

Using a password is an example of security by obscurity.

No. There is a difference between security that uses a secret key as part of the mechanism, and "security by obscurity".

Granted, there is a sliding scale involved - rot13 is "security by obscurity" now, but thousands of years ago, it was a state-of-the-art cipher, using a secret key of "N".

Say $_REQUEST['discount_price'][$key] == 0

then

$value = floatval(0) // still zero
$discount = - 100 + (0 * 100 ) / 0;

// translation: -100 + 0 / 0;

And of course if the curious math was fixed users gain the opportunity to pick any price they want by modifying the form/request string