On Apr 11, 2011 10:58 PM, "Hyrum K Wright" <hy...@hyrumwright.org> wrote: > > On Mon, Apr 11, 2011 at 9:41 PM, Greg Stein <gst...@gmail.com> wrote: > > Woah. When did svn_sqlite__prepare arrive? > > $ svnd blame subversion/libsvn_subr/sqlite.c | grep svn_sqlite__prepare > 875453 hwright > SVN_ERR(svn_sqlite__prepare(&db->prepared_stmts[stmt_idx], db, > 873188 gstein svn_sqlite__prepare(svn_sqlite__stmt_t **stmt, > svn_sqlite__db_t *db, > 875770 hwright SVN_ERR(svn_sqlite__prepare(&stmt, db, "PRAGMA > user_version;", scratch_pool)); > > It looks like you introduced (or resurrected it) in r873188.
Really, Hyrum? Is that the argument here? That I can't disagree with this function because I'm in the blame list? You use the term "resurrect" because you've probably see the log for r873188. That function came from an earlier implementation. It got (re)introduced by bringing that rev back. Then discussions between yourself and me determined that we wanted static text, rather than arbitrary preparation, so the current (static) system was devised, and we built it. So yeah: despite the implication that I should be OK with this... no. I'm not. > > > I'm basically -1 on that. > > > > The whole idea behind static statements was to avoid SQL injection attacks. > > Allowing the *code* to construct statements opens us up. > > > > This is Not Good. > > This is still a prepared statement, arguments are still bound using > the standard SQLite binding mechanism, so they will be properly > escaped. Arguments, yes. But this edits the statement itself, outside of binding. > Yes, one can put arbitrary text in the > statement-to-be-prepared, but the preparation step helps prevent > injection attacks. (You can't prepare multiple statements, which > vastly decreases the attack space.) No. You may be referring to *this* statement. What about the next one which is an INSERT or UPDATE? Slippery slope that I disagree with. We spoke about this, and I haven't changed my mind. Static text only. > > Plus, we're talking about local data here. While sql injection is a > Bad Thing, I suspect somebody wanting to hose a working copy will do > something much simpler: rm .svn/wc.db. :) Thanks for the strawman. Are you sure that *nothing* will cause an injection? Trojans? Bad server input? Inappropriate clicks in a UI? The static text design was to disable all such attacks. I did't even realize we had left the prepare function in there, since we had designed all callers to avoid it. > > On a higher-level, if we *don't* use this API, what are your > suggestions on how to create a IN clause with arbitrary numbers of > values? Not sure. Maybe we can work through some ideas. But "we have no other choice" is not a good enough reason to keep this. That is an even worse slope to slide down. Doing things simply because they are "convenient". Cheers, -g
