-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
greg@svn.wikimedia.org wrote:
- /**
* Returns true if this database uses timestamps rather than integers- */
- function realTimestamps() {
return $this->mRealTimestamps;- }
If something like this is necessary at all, why bother with a member variable? The function should simply be overridden appropriately.
- /* Offset must be an integral. */
- if ( !strlen( $options['offset'] ) || !preg_match( '/^[0-9]+$/', $options['offset'] ) )
$options['offset'] = '';
- /* Offset must be an integral, unless the db is using timestamps */
- $dbr =& wfGetDB( DB_SLAVE );
- if ( !strlen( $options['offset'] ) ||
( !$dbr->realTimestamps() && !preg_match( '/^[0-9]+$/', $options['offset'] ) ) )- $options['offset'] = '';
This looks like it may be unsafe; the code here is pretty hairy and I didn't see any _other_ validation of input.
1) Confirm you didn't introduce an SQL injection attack vector on PG systems.
2) Consider simply fixing the code to properly validate input and produce portable output, the way it's supposed to.
There should be no need to check or care what internal format the database uses when dealing with form parameters.
- -- brion vibber (brion @ pobox.com)
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Brion Vibber asks:
If something like this is necessary at all, why bother with a member variable? The function should simply be overridden appropriately.
A member variable avoids putting in any ugly "if db == postgres" lines, and allows easy future handling of other date issues. I'm not sure what function you are saying should be overridden?
This looks like it may be unsafe; the code here is pretty hairy and I didn't see any _other_ validation of input.
- Confirm you didn't introduce an SQL injection attack vector on PG
systems.
Excellent point, I'll fix that up.
- Consider simply fixing the code to properly validate input and
produce portable output, the way it's supposed to.
There should be no need to check or care what internal format the database uses when dealing with form parameters.
Not clear what you mean here. Since the internal format determines the range of valid form input, the only other way to do it is to convert the timestamp to epoch, output that, and then make sure we convert it back after the validation but before it is passed to any queries (or make alternative queries).
- -- Greg Sabino Mullane greg@turnstep.com PGP Key: 0x14964AC8 200611290849 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
Greg Sabino Mullane wrote:
A member variable avoids putting in any ugly "if db == postgres" lines, and allows easy future handling of other date issues. I'm not sure what function you are saying should be overridden?
I believe the idea is to have separate _classes_ for each DB system, so "db == postgres" would not be necessary because you'd have a separate class for PostgreSQL. In that class, you should simply override the function realTimestamps() and plainly return true. That would be proper object-oriented design, and it's a very common practice.
Timwi
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Timwi wrote:
Greg Sabino Mullane wrote:
A member variable avoids putting in any ugly "if db == postgres" lines, and allows easy future handling of other date issues. I'm not sure what function you are saying should be overridden?
I believe the idea is to have separate _classes_ for each DB system, so "db == postgres" would not be necessary because you'd have a separate class for PostgreSQL. In that class, you should simply override the function realTimestamps() and plainly return true. That would be proper object-oriented design, and it's a very common practice.
Yep. :)
Since we've already got those separate classes, the sensible and readable thing is for the method to simply return the correct value for its own class, rather than schlepping around a member variable that never changes.
Greg Sabino Mullane wrote:
- Consider simply fixing the code to properly validate input and
produce portable output, the way it's supposed to.
There should be no need to check or care what internal format the database uses when dealing with form parameters.
Not clear what you mean here. Since the internal format determines the range of valid form input, the only other way to do it is to convert the timestamp to epoch, output that, and then make sure we convert it back after the validation but before it is passed to any queries (or make alternative queries).
Yes, exactly. This is how all other web I/O is done, including other uses of timestamps.
The wfTimestamp() performs normalization of all known input formats to a requested format for either internal processing or output.
The TS_MW format is used in most cases for web I/O; it's also used for timestamp fields with the current MySQL classes, but you never need to know that since it's encapsulated in the Database->timestamp() method.
Sometimes other formats are used for particular protocols that prefer some normalized date format.
- -- brion vibber (brion @ pobox.com)
wikitech-l@lists.wikimedia.org