setQuery() method in JDatabase

[Mark Dexter]

In the setQuery() method we have the following code:

$this->limit = (int) $limit;

$this->offset = (int) $offset;

This code ensures we get integers, but we can still get SQL errors if one of these values is negative.

If we prevented negative numbers for these values (with something like the following), we would prevent this type of SQL error and I think still support all valid use cases:

$this->limit = ((int) $limit >= 0) ? (int) $limit : 0;

$this->offset = ((int) $offset >= 0) ? (int) $offset : 0;

This would prevent possible information disclosures caused by someone deliberately trying to cause a SQL error. 

What do people think? Thanks. Mark

[Andrew Eddie]

Sounds fine.

Regards,
Andrew Eddie
http://learn.theartofjoomla.com - training videos for Joomla developers

[Chris Davenport]

$this->limit = abs( (int) $limit );

Avoiding the conditional?

Chris.

--
Chris Davenport
Joomla Leadership Team - Production Working Group
Joomla Documentation Coordinator

[84.le0n]

Il giorno 02/mag/2012, alle ore 06:51, Chris Davenport <chris.d...@joomla.org> ha scritto:

> $this->limit = abs( (int) $limit );
>
> Avoiding the conditional?
>
> Chris.

It's a better solution.

Just a proposal: each driver has to manage and format these values in different manner inside it's own query, so why don't add "limit" and "offset" inside JDatabaseElement, as I already did inside Postgresql's driver, for 3.0 or 3.5 release ?

Eng. Gabriele Pongelli

AVVERTENZE AI SENSI DEL D.LGS. 196/2003
Le informazioni contenute in questo messaggio di posta elettronica e negli eventuali files allegati, sono da considerarsi strettamente riservati. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceveste per errore questo messaggio, Vi preghiamo cortesemente di darcene notizia all'indirizzo e-mail di cui sopra e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema; costituisce comportamento contrario ai principi dettati dal D.lgs. 196/2003 il trattenere il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse.
This electronic transmission is strictly confidential and intended solely for the addresses. It may contain information which is covered by legal, professional or other privilege. If you are not the intended addressee, you must not disclose, copy or take any action in reliance of this transmission. If you have received this transmission in error, please notify us and delete the received data as soon as possible

[Will Daniels]

On 02/05/12 09:36, 84.le0n wrote:
> Just a proposal: each driver has to manage and format these values in different manner inside it's own query, so why don't add "limit" and "offset" inside JDatabaseElement, as I already did inside Postgresql's driver, for 3.0 or 3.5 release ?

+1

It's fine for SELECT ... LIMIT OFFSET syntax to just append to the SQL, but for
other syntax it all gets a bit hacky[1] trying to rewrite the query, which
naturally involves making some assumptions.

-Will


[1]
https://github.com/wdaniels/joomla-cms/blob/odbc-driver/libraries/joomla/database/database/virtuoso.php#L360

[Ian]

I'm inclined to think that if $limit or $offset is less than 0 you throw an InvalidArgument exception.

Ian

[Amy Stephen]

Agree with Mark to prevent the disclosure due to a fatal error.

Agree with Ian on the approach.

[Niels Braczek]

Am 02.05.2012 16:06, schrieb Ian:

> I'm inclined to think that if $limit or $offset is less than 0 you throw an
> InvalidArgument exception.

That's much cleaner than producing unpredictable[tm] results by
manipulating the input data.

Regards,
Niels

--
| http://barcamp-wk.de · 1. Barcamp Westküste 30./31. März 2012 |
| http://www.bsds.de · BSDS Braczek Software- und DatenSysteme |
| Webdesign · Webhosting · e-Commerce · Joomla! Content Management |
------------------------------------------------------------------

[Mark Dexter]

What if the input is not an integer? Currently we are converting to integer. Should we throw an exception also if either value is a non-integer? Thanks. Mark

[Niels Braczek]

Am 02.05.2012 17:08, schrieb Mark Dexter:

> What if the input is not an integer? Currently we are converting to
> integer. Should we throw an exception also if either value is a
> non-integer? Thanks. Mark

Although I think that casting is acceptable, an Exception would be the
cleaner way, which I would prefer.

[Amy Stephen]

Good point, Mark. I guess the question is - what is the API in this case? If it's not clear, then maybe deciding what input is required is the first step. Certainly makes it easier to code with those decisions in place.

[CirTap]

Am 02.05.2012 06:51, schrieb Chris Davenport:
> $this->limit = abs( (int) $limit );
>
> Avoiding the conditional?
>

using abs() will create a very different result than what Mark suggested
$limit = -5;

$this->limit = abs( (int) $limit )

= 5

this will clamp negative numbers to Zero which seems more safe to me
$limit = -5;

$this->limit = ((int) $limit >= 0) ? (int) $limit : 0;

= 0;

To avoid the explicit condition, the repetitive type conversion and
clamp negative numbers to zero:
$limit = -5;
$this->limit = (int) max(0, $limit);
= 0

CirTap