Opened 8 years ago

Closed 5 years ago

#34 closed defect (fixed)

SQL cleanup - use DBI ? substitutions, move all SQL into IPDB.pm

Reported by: kdeugau Owned by:
Priority: major Milestone: 2.8
Version: Keywords:
Cc:

Description

Much of the SQL currently uses:

 $sth = $dbh->prepare("INSERT INTO table (cols) values ('$literal1','$literal2')");
 $sth->execute;

instead of:

 $sth = $dbh->prepare("INSERT INTO table (cols) values (?,?)");
 $sth->execute($literal1, $literal2);

Switching to CGI::Simple (#15) is exposing several places where this is problematic.

This is also something of an SQL-injection security issue - using DBI's parameter replacement means that user data goes right into the table, instead of munging the SQL (deliberately or otherwise).

Change History (28)

comment:1 Changed 8 years ago by kdeugau

(In [448]) /branches/htmlform

Escape forwarded form data with $q->escapeHTML on most forms in
main.cgi (see #15)
Fix up most subs in IPDB.pm that deal with form data that needs
escaping (see #34)

comment:2 Changed 8 years ago by kdeugau

(In [485]) /trunk

Convert pool-IP inserts to use DBI ? substitution on city. See #34.

comment:3 Changed 8 years ago by kdeugau

(In [492]) /branches/htmlform

Fix allocation update execution to use DBI ? substitutions. See #34.

comment:4 Changed 7 years ago by kdeugau

(In [508]) /branches/stable

Prep-for-release cleanup of buglets found making sure the demo
install works

  • Clean up instructions for creating the database. Apparently the PL/pgSQL "language" module required for the last-modified triggers can't be installed by a regular user, and isn't available by default. O_o
  • Fix a missed $IPDB::webpath-in-single-quotes
  • Add a quick hack to allow automagical allocation from private net ranges. See #38.
  • Partially convert some critical bits to use bound parameters in SQL for new allocations. See #34, mostly cleaned up already on /trunk or /branches/htmlform
  • Set $privdata = internally so that an allocation via admin tools doesn't error out

comment:5 Changed 6 years ago by kdeugau

(In [518]) /trunk

Update addMaster() to use ?-substitution in SQL. See #34.

comment:6 Changed 6 years ago by kdeugau

  • Priority changed from minor to major
  • Summary changed from SQL cleanup - use DBI ? substitutions to SQL cleanup - use DBI ? substitutions, move all SQL into IPDB.pm

comment:7 Changed 6 years ago by kdeugau

(In [519]) /trunk

Move SQL for node search page into IPDB.pm. See #34.
Also tweak some handling for the lingering global statement handle
variable, since it may be undefined instead of a statement handle.

comment:8 Changed 6 years ago by kdeugau

(In [523]) /trunk

Move SQL for index/summary page into IPDB.pm. See #34.
Tweak initialization of page templates to set loop_context_vars so
we don't have to manually maintain the row0/row1 entries
Commentstub subs for master and routed list pages.

comment:9 Changed 6 years ago by kdeugau

(In [524]) /trunk

Clean up and move SQL for showmaster to IPDB.pm. See #34.
Tweak template to use odd for row colors

comment:10 Changed 6 years ago by kdeugau

(In [525]) /trunk

Found the HTML::Template knob to twist to allow listMaster() etc to
pass back '<NONE>' instead of '&lt;NONE&gt;'. See #34, #3 (sort of).

comment:11 Changed 6 years ago by kdeugau

(In [527]) /trunk

Clean up and move SQL for showRBlock to IPDB.pm
Tweak listFree() since the segments in showMaster and showRBlock it
replaced weren't quite as identical as I thought
Convert template to use odd and squeeze a bit more HTML out of
main.cgi and IPDB.pm

See #34.

comment:12 Changed 6 years ago by kdeugau

(In [528]) /trunk

Clean up and move SQL for static IP pool list to IPDB.pm. See #34.

  • Rename listPool in main.cgi to showPool, so that we can:
  • Add listPool sub in IPDB.pm
  • Convert getBlockData to return a hashref instead of an array, and update the one extant call

Tweak template to use odd for row colors

comment:13 Changed 6 years ago by kdeugau

(In [529]) /trunk

Clean up and move SQL behind block assignment page to IPDB.pm. See #34.

comment:14 Changed 6 years ago by kdeugau

(In [530]) /trunk

Clean up and move most SQL for node handling into IPDB.pm. See #34.
Tweak "new assignment" and "edit block" templates a little to match
and for consistency.

comment:15 Changed 6 years ago by kdeugau

(In [531]) /trunk

Clean up and move most SQL for allocation update into IPDB.pm. See #34.

comment:16 Changed 6 years ago by kdeugau

(In [532]) /trunk

Clean up and move SQL to IPDB.pm for pool-selector for static IPs
on allocation confirmation page. See #34.

comment:17 Changed 6 years ago by kdeugau

(In [533]) /trunk

Clean up and move SQL for finding a free block to allocate from
into IPDB.pm. See #34.
Also fix a minor "Use of uninitialized value...".

comment:18 Changed 6 years ago by kdeugau

(In [534]) /trunk

Clean up and merge SQL for block-edit page into getBlockData.
Fortunately, the enhancement does not affect previous uses of
that sub. See #34.
Also tweak the template with a whitespace nitpick and to escape
HTML-funky characters in the circuit ID, description, notes, or
restricted data. Still need to confirm these can be reversed
on submission. See The Ticket That Won't Die, #3.

comment:19 Changed 6 years ago by kdeugau

(In [536]) /trunk

Clean up and move SQL for post-update backlink to IPDB.pm. See #34.
Also rename getParent() to subParent() to fit in with ipParent() and
blockParent().

Fix a couple "Use of uninitialized..." log-noise bugs. See #31.

Move some HTML-entity-escaping into the template, and shuffle lines
munging the notes and restricted data on post-update value display
so we can properly munge in <br> for \n. Doesn't seem to be a way
to plug that into HTML::Template. :( See #3.

comment:20 Changed 6 years ago by kdeugau

(In [538]) /trunk

Remove SQL in favour of calls to existing subs on delete confirmation
page. See #34.
Tweak template to remove a stale form variable and tighten
HTML-entities escaping on delete confirmation page. See #3.

comment:21 Changed 6 years ago by kdeugau

(In [539]) /trunk

Remove global $sth, since nothing uses it any more. See #34.

comment:22 Changed 6 years ago by kdeugau

(In [541]) /trunk

Start on SQL in admin.cgi. See #34.

  • Convert Q-n-D allocation list on main page to use existing getTypeList()
  • Convert timestamp-update master block list to use new getMasterList(), with a flag set to return the last-modified time. Also convert main.cgi new assignment page to use this, with the flag set to not return the lastmod.
  • Tweak admin main template to match

While following the code for the master block list, I also removed
several useless globals (@masterblocks, %allocated, %free, and
%routed) since they were only used originally in one place (index
page from main.cgi), obsoleted by changes in r523, and in fact got
overridden locally before that anyway.

comment:23 Changed 6 years ago by kdeugau

(In [542]) /trunk

Trim dangling database op that should have gone away in r541. See #34.

comment:24 Changed 6 years ago by kdeugau

(In [544]) /trunk

Extend findAllocateFrom() to allow passing a target block to allocate
to restrict the freeblock selection
Remove local SQL for confirmation page for "assign this block" in
admin.cgi in favour of existing data and subs. See #34.
For admin.cgi "assign this block", if the requested IP type does not
match the pool, the type will be adjusted and a warning pushed out.
Add space on "assign this block" template for a warning.

comment:25 Changed 6 years ago by kdeugau

(In [545]) /trunk

Tweak allocateBlock() to usefully handle passing an IP as an
override on the automatic IP-chooser.
Convert allocation of a pool IP in admin.cgi to use updated
allocateBlock(). See #34.

comment:26 Changed 6 years ago by kdeugau

(In [547]) /trunk

Clean up and move last-modified-change for master blocks to
IPDB.pm. See #34.

comment:27 Changed 6 years ago by kdeugau

  • Milestone changed from 3.0 to 2.8

comment:28 Changed 5 years ago by kdeugau

  • Resolution set to fixed
  • Status changed from new to closed

Resolving; with two exceptions all direct SQL has been moved to IPDB.pm.

Exceptions:

  • main.cgi, when browse-assigning a static IP, pool data is retrieved with a dbh->selectrow_array(). This is already obsoleted in /trunk due to the new allocation structures; the code uses a second call to getBlockData().
  • ipdb-rpc.cgi, getAvailableStatics uses dbh->selectall_arrayref for an operation not used in the web UI.
Note: See TracTickets for help on using tickets.