Thread: SQL failures, new navigation system and other things
Brought to you by:
iridium
Menu
▾
▴
phpweather-devel
|
From: Ondrej J. <ne...@po...> - 2002-08-27 17:37:28
Attachments:
nepto-phpweather-patches.tgz
|
Maxim, 19:19:39
27. august 2002 (utorok)
Greetings again!
Today I implemented things I was talking about yesterday. As it was
written, I wanted them and I'm usign them on my website (I will pass URL
tomorrow evening). But I also suppose that you will be interested in them
too.
This e-mail contains three problems described, with solution
written. Patches are also included.
SQL failures
------------
PROBLEM: I discovered several SQL query failures when someone nasty pass
strange parameters into CGI variables (ie. quoting chars).
SOLUTION: Patch SQL queries and do appropriate slashes escaping. Also
patch main program to strip slashes from variables recieved from
CGI interface. Annoying thing is, that icao is stored
add-slashed in data_retrieval object; keep this in mind and
strip slashes when returning icao instead of station name!
PATCH: File db/pw_db_mysql.php was patched. It contains:
- proper query escaping (they look nicer now however)
- striping slashes when returning icao instead name of station
Decreased pw_utilities.php usage
--------------------------------
PROBLEM: Functions in pw_utilities.php are less usuable due to containing
hidden "old" variables. We will not need them anymore also
on test page.
SOLUTION: Remove these hidden "old" variables from pw_utilities.php.
PATCH: File pw_utilities.php was patched. It was posted yesterday
to phpWeather development mailinglist.
Navigation system (demo page) problems
--------------------------------------
PROBLEM: Navigation system on demopage is too complicated. It contains
redunant data. Not only redunant "old" variables, also
redunand cc/icao ones; note that icao contains itself
country code.
SOLUTION: Completely rewrite navigation system use only three types of
page (1 - pure country selection, 2 - location selection,
3 - weather print; detailed information for this was posted
yesterday). Use new DB method get_country() to get country
for particular icao.
This "new style" also allows visitor to direct link weather
page for location (airport). He should from scratch write
http://example.com/weather.php?icao=lztt if he or she knows
particular icao. No other passed CGI variables are needed.
PATCHES: 1. data_retrieval.php was patched (get_country() method
implemented)
2. db/pw_db_mysql.php was patched (get_country($icao) method
was implemented)
3. index.php was rewritten (it's easier to understand,
contains less code, and maybe also faster; I'm sending
whole file, not patch)
All shoud work well for MySQL. There is a need for implementation of
the same thing in other DB modules as it is in MySQL one. Once again, they
are these three:
1. Proper query escaping.
2. Slashes stripping when returning icao instead name of station.
3. Method get_country($icao), which returns country code for
partcular icao.
I will implement it today night for PostgreSQL, but what about DBA?
Maybe you don't understand or not undestand absolutely what I'm
speaking about, or simply don't trust me. Than please ask anything about it,
I will try to advocate my changes. :-) I want also ask you for testing it.
It should work without *NO* problems (for MySQL currently).
Thanks,
=Nepto=
____________________________________________________________________________
Ondrej 'Nepto' Jombik, http://www.nepto.sk/ ne...@at...
|
|
From: Ondrej J. <ne...@po...> - 2002-08-27 19:20:38
Attachments:
db-pw_db_pgsql.php.patch
|
Maxim, 20:37:32
27. august 2002 (utorok)
Greetings!
> All shoud work well for MySQL. There is a need for implementation of
> the same thing in other DB modules as it is in MySQL one. Once again, they
> are these three:
As I said, I implemented that stuff for PostgreSQL. I supposed that
it will take few hours to study PostgreSQL SQL language specials, but in
fact it took only few minutes. Patch in attachement.
> 1. Proper query escaping.
Quesries seems to be properly escaped. But at least insert will fail
if $cc or $location will contain apostroph character. But, well isit
possible?
> 2. Slashes stripping when returning icao instead name of station.
Done.
> 3. Method get_country($icao), which returns country code for
> partcular icao.
Done. Copy/paste from MySQL module did the work (but I was forced to
use ' instead of ", strange isn't it? :-)
> I will implement it today night for PostgreSQL, but what about DBA?
Maybe I will also learn DBA till tomorrow I will send patch also for
this, but now I really don't know how does it works because my glance at
sources tell that it is not like classical RDBM SQL systems.
Another issue for PostgreSQL is that when using function "Create or
Recreate Tables" it will fail if I give it empty database. It is due fact,
that in PostgreSQL it is impossible to DROP TABLE that does not exists. I
solve this problem with these two commands:
phpweather=> create table metars ( foo int );
CREATE
phpweather=> create table stations ( foo int );
CREATE
phpweather=>
But it needs more complex solution anyway.
=Nepto=
____________________________________________________________________________
Ondrej 'Nepto' Jombik, Open Source software developer, http://www.platon.sk/
|
|
From: Martin G. <gim...@gi...> - 2002-08-27 23:33:42
|
Ondrej Jombik <ne...@po...> writes: > Maxim, 19:19:39 > 27. august 2002 (utorok) > Greetings again! > > Today I implemented things I was talking about yesterday. As > it was written, I wanted them and I'm usign them on my website (I > will pass URL tomorrow evening). But I also suppose that you will be > interested in them too. I'm very interested - and I want to thank you for the time you put into PHP Weather. The patches and suggestions are coming faster than I can get time to read them :-) If you plan to continue in this pace, then why don't get yourself a CVS account? I can add nepto as a developer if you're interested... > This e-mail contains three problems described, with solution > written. Patches are also included. I've looked at the patches and I like them - especially the KISS principle :-) But why have you rewritten all the SQL statements to use sprintf() to embed the variables instead of using either "foo $var bar" or 'foo ' . $var . ' bar'? Apart from that, I'll probably commit them tomorrow (or to be correct: later today, it's getting late... :-) I'm sure that I can patch the null and dba database backends to handle the lookup for country code. -- Martin Geisler My GnuPG Key: 0xF7F6B57B See http://gimpster.com/ and http://phpweather.net/ for: PHP Weather => Shows the current weather on your webpage and PHP Shell => A telnet-connection (almost :-) in a PHP page. |
|
From: Ondrej J. <ne...@po...> - 2002-08-28 00:11:26
|
Maxim, 02:09:20
28. august 2002 (streda)
Greetings.
> If you plan to continue in this pace, then why don't get yourself a
> CVS account? I can add nepto as a developer if you're interested...
Yes, that's not bad idea. It should make 2.x development slight
flexible and faster. So I'm interested. Thanks.
> > This e-mail contains three problems described, with solution
> > written. Patches are also included.
> I've looked at the patches and I like them - especially the KISS
> principle :-) But why have you rewritten all the SQL statements to use
> sprintf() to embed the variables instead of using either "foo $var
> bar" or 'foo ' . $var . ' bar'?
I have answer for this. When I sterted with implementation of
described things, I didn't know that $icao is already escaped (addslashed).
So I glace at queries and see that I need to add addsleshes($icao) into
almost every query. But than I said, why not to rewrite all of them to be
more readable?
I must admin that finally it was unneccessary. Sorry for that, but I
think that buinding queries with sprintf() gets more readability. I like
this way.
> Apart from that, I'll probably commit them tomorrow (or to be correct:
> later today, it's getting late... :-) I'm sure that I can patch the
> null and dba database backends to handle the lookup for country code.
Thanks. I was affraid that I will study DBA interface. Without
get_country() method in pw_db_dba.php it will not works. That's fact.
Regards,
=Nepto=
____________________________________________________________________________
Ondrej 'Nepto' Jombik, Visit Atlantis talker now! http://atlantis.sk/
|
|
From: Martin G. <gim...@gi...> - 2002-08-28 10:31:42
|
Ondrej Jombik <ne...@po...> writes:
Hi Ondrej and others,
>> If you plan to continue in this pace, then why don't get yourself a
>> CVS account? I can add nepto as a developer if you're interested...
>
> Yes, that's not bad idea. It should make 2.x development
> slight flexible and faster. So I'm interested. Thanks.
Great - I've added you to the project.
> ... I glace at queries and see that I need to add addsleshes($icao)
> into almost every query.
I've left most of the addslashes in the code, but I also think that
some of them are unnecessary.
We have two cases:
1) We're looking something up in the database. If the data can come
directly from the user, we need to use addslashes().
2) We're returning something from the database. If it comes from the
stations and countries databases, then we don't need to
stripslashes(). The data in those databases comes from
stations.csv and should be safe.
If the data comes from the METARs database, then we have to
stripslashes() because slashes might have been added when the
data was inserted.
> I must admin that finally it was unneccessary. Sorry for that,
> but I think that buinding queries with sprintf() gets more
> readability. I like this way.
I see - it actually get's more readable when you use sprintf() on a
long SQL query.
>> Apart from that, I'll probably commit them tomorrow (or to be
>> correct: later today, it's getting late... :-) I'm sure that I can
>> patch the null and dba database backends to handle the lookup for
>> country code.
I've just commited the patches along with updated pw_db_null and
pw_db_dba classes. I've tested all four backends, but it would be nice
to have someone double check it on another system.
> Thanks. I was affraid that I will study DBA interface.
The DBA databases are just like an array on disk: you can lookup a
value by a key and that's it. So when I have to store data about ICAOs
I do it by combining the name, country, and country code and then the
whole thing using the ICAO as a key.
The same applies to the null database - both backends use two
constants (PW_FIELD_SEPERATOR and PW_FIELD_REPLACEMENT) to seperate
the fields.
> Without get_country() method in pw_db_dba.php it will not works.
> That's fact.
Yes. I've renamed the method to get_country_code() because we now also
have a get_name() and get_country() that returns the name of a station
and the name of the country. Is this OK?
--
Martin Geisler My GnuPG Key: 0xF7F6B57B
See http://gimpster.com/ and http://phpweather.net/ for:
PHP Weather => Shows the current weather on your webpage and
PHP Shell => A telnet-connection (almost :-) in a PHP page.
|
|
From: Ondrej J. <ne...@po...> - 2002-08-28 23:13:43
|
Maxim, 00:39:03
29. august 2002 (stvrtok)
Greetings.
> 2) We're returning something from the database. If it comes from the
> stations and countries databases, then we don't need to
> stripslashes(). The data in those databases comes from
> stations.csv and should be safe.
I was not doing anything on data retrieved from database. They are
always safe, IMHO.
> If the data comes from the METARs database, then we have to
> stripslashes() because slashes might have been added when the
> data was inserted.
I misunderstand this, but my ideas are based on the following fact:
every string that will be inserted into database should be addslashed before
insert, because it could contain quoting character(s). Than will query
suceed and string will not contains these slashes which was added by
addslashes() call.
If string is "safe" and it is not possible that it will contain
quoting characters, backslash or nul (\0) character, that it could be
addslashed too. It will remains unchanged.
So conclusion: addslasles on strings inserted into DB is *always*
good idea. Problem may occur only if string is addslashed twice or more
times.
> >> Apart from that, I'll probably commit them tomorrow (or to be
> >> correct: later today, it's getting late... :-) I'm sure that I can
> >> patch the null and dba database backends to handle the lookup for
> >> country code.
> I've just commited the patches along with updated pw_db_null and
> pw_db_dba classes. I've tested all four backends, but it would be nice
> to have someone double check it on another system.
I checkouted fresh copy of phpWeather, because there was lot of
merging problems. I tried it for MySQL and PostgreSQL and it worked well. I
also tried this "new" version of phpWeather on my site and again without
problems (I only change get_country() for get_country_code() call).
> > Without get_country() method in pw_db_dba.php it will not works.
> > That's fact.
> Yes. I've renamed the method to get_country_code() because we now also
> have a get_name() and get_country() that returns the name of a station and
> the name of the country. Is this OK?
Yes, absolutely. I see that amount of queries was integrated into
one and cached data are stored in $icao_data now.
=Nepto=
____________________________________________________________________________
Ondrej 'Nepto' Jombik, Open Source software developer, http://www.platon.sk/
|
|
From: Martin G. <gim...@gi...> - 2002-08-29 10:51:55
|
Ondrej Jombik <ne...@po...> writes: >> The data in those databases comes from stations.csv and should be >> safe. > > I was not doing anything on data retrieved from database. They > are always safe, IMHO. Yes, my mistake. >> If the data comes from the METARs database, then we have to >> stripslashes() because slashes might have been added when the data >> was inserted. Ups - when we add slashes to a string and insert it in the database, then we wont see the slashes again when we extract it from the database again... So the above makes no sence. > So conclusion: addslasles on strings inserted into DB is > *always* good idea. Problem may occur only if string is addslashed > twice or more times. Yes, exactly. I think the easiest way to handle this is to let the methods in the db backends be the only methods to add slashes. All other methods should not have to care about this. > I checkouted fresh copy of phpWeather, because there was lot > of merging problems. I didn't apply your patches directly (unmodified) so that's why you got merging problems. > I tried it for MySQL and PostgreSQL and it worked well. I also tried > this "new" version of phpWeather on my site and again without > problems (I only change get_country() for get_country_code() call). Good - that should be the only change necessary. Does this mean that we're ready to release the code? -- Martin Geisler My GnuPG Key: 0xF7F6B57B See http://gimpster.com/ and http://phpweather.net/ for: PHP Weather => Shows the current weather on your webpage and PHP Shell => A telnet-connection (almost :-) in a PHP page. |
|
From: Ondrej J. <ne...@po...> - 2002-08-30 01:19:18
|
Maxim, 02:14:38
30. august 2002 (piatok)
Greetings.
> > So conclusion: addslasles on strings inserted into DB is
> > *always* good idea. Problem may occur only if string is addslashed
> > twice or more times.
> Yes, exactly. I think the easiest way to handle this is to let the
> methods in the db backends be the only methods to add slashes. All
> other methods should not have to care about this.
So also icao should be stored as-is without starting addslashes()
call used? I will be for this. Ugly stripslashes() hack when using icao as
substitution for human readable station name could be than removed. But
IMHO this should be done after comming release. It is not majority problem.
> > I tried it for MySQL and PostgreSQL and it worked well. I also tried
> > this "new" version of phpWeather on my site and again without
> > problems (I only change get_country() for get_country_code() call).
> Good - that should be the only change necessary.
> Does this mean that we're ready to release the code?
From my point of view yes. That was all things I had actual for
phpWeather. Others after release. But maybe someone other has also
something...
I will try to find some time for submiting things (comments/bugs) on
SF, just for remembering them.
=Nepto=
____________________________________________________________________________
"Be conservative in what you do, be liberal in what you accept from others."
(RFC 793: Transmission control protocol; chapter 2.10. Robustness Principle)
|
|
From: Martin G. <gim...@gi...> - 2002-08-30 13:28:07
|
Ondrej Jombik <ne...@po...> writes: >> Yes, exactly. I think the easiest way to handle this is to let the >> methods in the db backends be the only methods to add slashes. All >> other methods should not have to care about this. > > So also icao should be stored as-is without starting > addslashes() call used? Do you mean when we store the ICAO and station name when initializing the databases? > I will be for this. Ugly stripslashes() hack when using icao as > substitution for human readable station name could be than removed. Where is this code you're talking about? I'm afraid that you've got me confused. The only code that uses stripslashes() is the code in make_config.php. >> Does this mean that we're ready to release the code? > > From my point of view yes. That was all things I had actual > for phpWeather. Others after release. But maybe someone other has > also something... In that case - they're too late. I've just made the release on SourceForge. I'll go and make an announcement on Freshmeat now. > I will try to find some time for submiting things > (comments/bugs) on SF, just for remembering them. Good idea! -- Martin Geisler My GnuPG Key: 0xF7F6B57B See http://gimpster.com/ and http://phpweather.net/ for: PHP Weather => Shows the current weather on your webpage and PHP Shell => A telnet-connection (almost :-) in a PHP page. |
|
From: Ondrej J. <ne...@po...> - 2002-08-30 23:57:20
|
Maxim, 01:28:09
31. august 2002 (sobota)
Greetings.
> > I will be for this. Ugly stripslashes() hack when using icao as
> > substitution for human readable station name could be than removed.
> Where is this code you're talking about? I'm afraid that you've got me
> confused. The only code that uses stripslashes() is the code in
> make_config.php.
I'm afraid, that we do not completely understand ourself, so I will
try to explain the issue. All I was talking about is related to line 78 in
data_retrieval.php file. It looks like the following one:
$new_icao = addslashes($new_icao);
Icao is stored in internal object structures addslashed. No matter
that it will probably never contain quoting or other problematic character.
In fact, when compiling query you don't need to call addslashes() on
$icao, because it is already addslashed. My suggestion was to store icao
as-is in data_retrieval class and call appropriate addslashes() on $icao
during query compiling.
Another issue is, that in old code lookup_icao() returns either
"Location, Country" or simply "$icao" when "Location, Country" was not found
for particular $icao in database. In that case, we need to do stripslashes()
on $icao, because it stored addslashed in internal data structures.
I hope, that now you understand what I was talking about. If not, no
matters. It is only precision issue, not real problem. Icao will probably
never contains any from problematic characters and if so, everything will
probably works well.
=Nepto=
____________________________________________________________________________
"Be conservative in what you do, be liberal in what you accept from others."
(RFC 793: Transmission control protocol; chapter 2.10. Robustness Principle)
|
|
From: Martin G. <gim...@gi...> - 2002-08-31 10:05:51
|
Ondrej Jombik <ne...@po...> writes: > [...] > > I hope, that now you understand what I was talking about. If > not, no matters. It is only precision issue, not real problem. Icao > will probably never contains any from problematic characters and if > so, everything will probably works well. Thanks for the explaination - I now see the problem. I agree that we shouldn't addslashes() in set_icao() but instead delay it to the generation of queries. I don't think we should worry about stripping slashes from the ICAO in get_location() - if someone has used a ICAO with things like quotes in it, then they deserve to get slashes back :-) But as you say, this will probably never happen. -- Martin Geisler My GnuPG Key: 0xF7F6B57B See http://gimpster.com/ and http://phpweather.net/ for: PHP Weather => Shows the current weather on your webpage and PHP Shell => A telnet-connection (almost :-) in a PHP page. |
|
From: Max H. <ma...@fl...> - 2002-08-30 14:47:33
|
> > > So conclusion: addslasles on strings inserted into DB is > > > *always* good idea. Problem may occur only if string is addslashed > > > twice or more times. > > Yes, exactly. I think the easiest way to handle this is to let the > > methods in the db backends be the only methods to add slashes. All > > other methods should not have to care about this. > > So also icao should be stored as-is without starting addslashes() > call used? I will be for this. Ugly stripslashes() hack when using icao as > substitution for human readable station name could be than removed. But > IMHO this should be done after comming release. It is not majority problem. I'm not sure what's being suggested here - is the suggestion that pw_db_mysql:query should perform the escaping? That's silly. addslashes() should be run at the point that the query SQL is generated... Yay for a release of 2.0.0 though! Max |
|
From: Ondrej J. <ne...@po...> - 2002-08-30 01:19:41
|
> Today I implemented things I was talking about yesterday. As it was > written, I wanted them and I'm usign them on my website (I will pass URL > tomorrow evening). So here it is: http://nepto.sk/specials/weather/ Navigation used there is now also included in phpWeather index.php demo page. =Nepto= ____________________________________________________________________________ Ondrej 'Nepto' Jombik, Visit Atlantis talker now! http://atlantis.sk/ |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X