[HTML/PHP]Extra lijn blokkeert blockchain API

[Carharttguy]

Hallo iedereen

Ik probeer de Receive Payment API van blockchain.info te gebruiken (https://blockchain.info/nl/api/api_receive).
Op zich lukt dit wel. Alleen heb ik een klein probleem met het callback script. Het wordt juist aangeroepen, en ik hoor "*ok*" terug te geven als alles goed is uitgevoerd. Dat doe ik, maar de logs van blockchain.info geven terug dat er HTML in mijn pagina zit, en dat mag niet.

Door in Firefox de bronpagina weer te geven merk ik dat er inderdaad eerst een blanke lijn staat, en dan pas de "*ok*". Ik denk dat daarom die API mijn pagina niet goed leest.

Ik gebruik onderstaande code in mijn callback script (Oude PHP, niet echt veilig, ik weet het)

PHP:

<?php
    include_once("database.php");
    $secret = $_GET['secret'];
    
    $query = "SELECT id, name, email, satoshi_price, secret FROM orders WHERE id = ".$_GET['id'];
    $query = mysql_real_escape_string($query);
    $result = mysql_query($query);

    $email = mysql_result($result, 0, 'email');
    $name = mysql_result($result, 0, 'name');
    
    if($secret != mysql_result($result, 0, 'secret')){
        echo "Fuck you.";
    }else{
        if($_GET['destination_address'] == "xxxxxxxxxxxxxxxxx"){
            if(intval($_GET['confirmations']) >= 6){
                $value = mysql_real_escape_string($_GET['value']);
                $transaction_hash = mysql_real_escape_string($_GET['transaction_hash']);
                $orderid = mysql_real_escape_string($_GET['id']);
                
                $query = "INSERT INTO bitcoin_payments(orderid, value, transaction_hash) VALUES(".$orderid.", ".$value.", '".$transaction_hash."')";
                $inserted = mysql_query($query);
                
                if($inserted){
                    echo "*ok*";
                }
            }
        }
    }
?>

Ik zie niet in hoe die echo '*ok*' een extra witte lijn kan genereren. Ik heb het trouwens ook bij die echo 'fuck you'. Terwijl er in dat script toch echt geen andere echo's staan.

Iemand een idee? Vind weinig terug op Google.

Dankjewel

[ Voor 0% gewijzigd door NMe op 17-07-2015 20:32 ]

[NMe]

Er staat vast een witregel voor je <?php. Of anders een BOM.

Je hebt jezelf trouwens wagenwijd open staan voor SQL-injectieaanvallen.

[ Voor 25% gewijzigd door NMe op 17-07-2015 20:44 ]

[Damic]

Trouwens je inkomende data een beetje escapen kan geen kwaad zie hier boven.

Euhm NMe heb jij die mysql_escape toegevoegd?

[ Voor 36% gewijzigd door Damic op 17-07-2015 20:35 ]

[NMe]

Damic schreef op vrijdag 17 juli 2015 @ 20:33:
Euhm NMe heb jij die mysql_escape toegevoegd?

Nee, die stond er blijkbaar al maar ik had hem ook gemist. Dat is trouwens wel een heel onverstandige plek om te escapen,

[Carharttguy]

Ik heb het gevonden, er stond een witregel onderaan mijn database.php die ik include!
Ja ik die query escapen nadat hij was samengesteld had ik gedaan omdat $orderid = mysql_real_escape_string($_GET['id']); niet onmiddellijk leek te werken bovenaan in dat script. Maar goed, is alweer aangepast, was maar een testscriptje

Maar gewoon uit nieuwsgierigheid. Is er dan enig verschil tussen eerst die ID escapen, of pas achteraf de query te escapen?

[krvabo]

Een echt verschil vast niet, maar je ziet dat twee andere programmeurs bij het bekijken van je code al meteen zoiets hadden van 'dit is onveilig', hoewel het uiteindelijk niet zo bleek te zijn. Het is altijd een best practice om dingen te escapen / beveiligen waar het nodig en logisch is, in deze gevallen zodra je de waarde in de string plakt (je plakt een 'onveilige' waarde aan een 'veilige'). Het zou kunnen dat je ergens die query kopieert omdat je hem later nodig hebt ($query2 = $query) voordat je hem escaped, maar dan denkt dat je hem al escaped hebt, terwijl dat niet zo is.

Je zou er voor kunnen kiezen om ergens in een bootloader je hele $_GET array te escapen (niet doen!), waardoor de waarde in principe veilig is, maar elke programmeur die je code ziet zal je vervloeken, omdat totaal onduidelijk is welke code veilig is, en welke niet (om het nog maar niet te hebben over als je de 'onveilige' waarde nodig hebt).

Het is dus gewoon een best practice om je waarde te escapen op het moment dat je de waarde in de string zet. Overigens zou een intval() of (int) cast hier waarschijnlijk nuttiger zijn, of zelfs een prepared statement.

[ Voor 8% gewijzigd door krvabo op 17-07-2015 23:08 ]

[orf]

In beide gevallen is het hier niet goed.

Stel dat $_GET['id'] het volgende bevat:

PHP:

<?php

$_GET['id'] = '1 OR 1=1 LIMIT 1';

Er staan geen "gevaarlijke tekens" in, dus er wordt niets ge-escapet. Toch levert deze query een ander resultaat op dan je verwacht.

Met quotes om de id heen zou het wel goed gaan. Die worden ge-escapet en dus levert de query geen resultaat op. Je kunt met intval() de waarde ook naar een integer omzetten.

Maar eigenlijk moet je gewoon Prepared statements gebruiken.

Edit: @hierboven: Een hele query escapen heeft meestal geen nut omdat daar je query juist van kapot gaat. Escapen is voor values, niet voor complete queries.

[ Voor 12% gewijzigd door orf op 17-07-2015 23:10 ]

[krvabo]

Je hebt gelijk, had inderdaad ook even niet gerekend op spaties. Daar kun je inderdaad wel wat dingen mee uithalen.

(Al zal, indien je de hele query escaped, niet snel je query kapot gaan, edge cases daargelaten ofc. Je krijgt waarschijnlijk wel een ander resultaat dan je zou willen.)

[Gomez12]

Carharttguy schreef op vrijdag 17 juli 2015 @ 22:53:
Maar gewoon uit nieuwsgierigheid. Is er dan enig verschil tussen eerst die ID escapen, of pas achteraf de query te escapen?

Voornamelijk leesbaarheid en onderhoudbaarheid.

In wezen zijn je value, transactionid en orderid nu "vervuild" want ze bevatten niet meer de invoer, maar enkel de voor mysql ge-escapete invoer. Heb je die variabelen later ergens nodig waar bijv een andere escaping nodig is dan moet je deze variabelen eerst mysql_unescapen en daarna weer voor die andere weer gaan escapen.

Zou je de variabelen nog een naam meegeven als mysql_orderid oid dan was het nog enigszins leesbaar (alhoewel ik het een overbodige variabele zou vinden maar ok) want dan was het duidelijk dat die variabele enkel aangemaakt was voor mysql en dus geescaped zou kunnen zijn.

Alhoewel ik bijv meestal ook niet zo vrolijk wordt van een orderid wat geen int is maar een string, Noem het dan een uid oid maar dat is mijn pet peeve...

orf schreef op vrijdag 17 juli 2015 @ 23:09:
Edit: @hierboven: Een hele query escapen heeft meestal geen nut omdat daar je query juist van kapot gaat. Escapen is voor values, niet voor complete queries.

Hij zegt imho expliciet dat je moet escapen als je de waarde in de string zet en hij heeft het niet over de totale string.

Dus zoiets :

PHP:

$query = "INSERT INTO bitcoin_payments(orderid, value, transaction_hash) VALUES(".mysql_real_escape_string($orderid).", ".mysql_real_escape_string($value).", '".mysql_real_escape_string($transaction_hash)."')";

Maar gewoon prepared statements gebruiken en je hebt de hele discussie niet...

krvabo schreef op vrijdag 17 juli 2015 @ 23:24:
(Al zal, indien je de hele query escaped, niet snel je query kapot gaan, edge cases daargelaten ofc. Je krijgt waarschijnlijk wel een ander resultaat dan je zou willen.)

Het probleem met string concatenation is dat je heel erg moet oppassen en dat er weinig dingen egde-cases zijn imho...
Zo werk ik bijv standaard met een aantal query's die vanuit de plek vanwaar ze worden aangeroepen bepaalde variabelen niet weten (in theorie heb ik gegevens ook niet nodig voor een juist resultaat) alleen weet ik die gegevens wel en helpen ze immens qua indexen etc, dus ik heb ze gewoon hard in de query opgenomen en moet ik ze dan wel of niet handmatig escapen.

Plus het hele simpele feit dat het op regel 6 wel doen, maar op regel 22 niet (daar zijn de variabelen weer ge-escaped) verwarring en fouten gaat brengen, probeer consistent te zijn.

[NMe]

krvabo schreef op vrijdag 17 juli 2015 @ 23:06:
Een echt verschil vast niet

Jawel, er is een verschil. Goed, dat verschil maakt het niet onveilig, maar wel naar om te debuggen. Stel, die query verandert over een tijdje en je moet bijvoorbeeld ook een status checken. Je kan dan "AND status = 'open'" toevoegen aan je WHERE-clause, maar ineens gaat dan je hele query stuk omdat de quotes ook escaped worden. Kun je dan alsnog je query herschrijven zodat je het op de goeie manier doet.

Oftewel: gebruik liever prepared statements, of als je dan al op deze manier wil escapen, doe dan de losse onderdelen die je invult in de query. En zelfs dan is het oppassen, zoals hierboven al omschreven.

[Damic]

Zoiets is al veel beter voor je waarden

PHP:

$value = (long) $_GET['value']; 
$orderid = (int) $_GET['id'];

trouwens die onderste query kan ook anders

PHP:

//van
 $query = "INSERT INTO bitcoin_payments(orderid, value, transaction_hash) VALUES(".$orderid.", ".$value.", '".$transaction_hash."')";
//naar
 $query = "INSERT INTO bitcoin_payments(orderid, value, transaction_hash) VALUES($orderid, $value, '$transaction_hash')";
//en anders doe je het zo
 $query = 'INSERT INTO bitcoin_payments(orderid, value, transaction_hash) VALUES('.$orderid.', '.$value.', \''.$transaction_hash.'\')';

en euhm een extra lege lijn in database.php????

[ Voor 81% gewijzigd door Damic op 18-07-2015 00:08 ]

[P.O. Box]

nog even ontopic:
in php is de sluittag ?> niet verplicht als hij aan het einde van je bestand komt te staan. Sterker nog , deze wordt afgeraden.
En precies om de reden die TS nu ondervonden heeft, de loze spatie/enter na zijn sluitingstag in z'n geinclude db file.

mijn advies: verwijder de ?>'s

[NMe]

Damic schreef op zaterdag 18 juli 2015 @ 00:00:
trouwens die onderste query kan ook anders

PHP:

//van
 $query = "INSERT INTO bitcoin_payments(orderid, value, transaction_hash) VALUES(".$orderid.", ".$value.", '".$transaction_hash."')";
//naar
 $query = "INSERT INTO bitcoin_payments(orderid, value, transaction_hash) VALUES($orderid, $value, '$transaction_hash')";
//en anders doe je het zo
 $query = 'INSERT INTO bitcoin_payments(orderid, value, transaction_hash) VALUES('.$orderid.', '.$value.', \''.$transaction_hash.'\')';

De tweede optie van je is heel persoonlijk, heel veel mensen vinden dat helemaal niet beter. En áls je het dan al zo doet, doe dan jezelf een plezier en schrijf altijd {$variabele} in plaats van $variabele. You'll thank me later.

Die derde regel is bepaald geen verbetering trouwens, waarom zou je gaan escapen als dat met de huidige constructie helemaal niet nodig is?

P.O. Box schreef op zaterdag 18 juli 2015 @ 08:27:
mijn advies: verwijder de ?>'s

Inderdaad, die dingen leveren alleen dit soort problemen op.

[Voutloos]

Even geen quote want er wordt heel veel onzin gepraat: Een mysql_real_escape_string() over een hele query voegt NIETS NADA NOPPES toe. Die functie heeft werkelijk alleen nut voor waarden die 1 (lees: één) enkel string in een query worden, inclusief bijbehorende quotes.

Escapen moet je gewoon goed doen. En niet lui (raar, want het is juist extra moeite t.o.v. consistent toepassen) doen als het voor een query - in de huidige vorm - wel los loopt.

[Diecke]

Een id checken met ctype_digit lijkt me beter. Hoef je ook niet meer te escapen en kun je een nette foutmelding geven.

[Voutloos]

Het een sluit het ander niet uit.
Geef gerust een zinnige melding bij invalid input AND laat ter plekke @ query zien dat je goed zit.

Dan blijft het ook werken als je inputvalidatie ooit verandert. En voor meer argumenten, zie het magic quotes fiasco.

[NMe]

Diecke schreef op maandag 20 juli 2015 @ 15:23:
Een id checken met ctype_digit lijkt me beter. Hoef je ook niet meer te escapen en kun je een nette foutmelding geven.

In veel gevallen hoef je niks te checken. Een "WHERE id = $id" levert bij 0 gewoon geen records op, dus in zo'n geval kun je blind je input naar int casten. Als er troep in je input zit krijg je dan gewoon geen resultaat zonder verdere extra check. Kan, zeker als de input alleen door tampering niet zou kunnen kloppen, een prima oplossing zijn. Maar niet altijd natuurlijk.

[ Voor 11% gewijzigd door NMe op 20-07-2015 18:01 ]