- In the Phacility deployment workflow, we have a couple of cases where we GRANT with a password. It would be cleaner to use %P to keep this out of --trace, but qsprintf() does not currently support %P.
- Supporting %P requires returning an object from qsprintf().
- In both the qsprintf() and csprintf() cases, we could implement more phutil_tag()-like semantics and make these objects respond to conversion with %s correctly (without escaping). This would let us dramatically reduce the amount of %Q and %C we use, which is highly desirable.
- However, we do a lot of manual string mangling with qsprintf() in Query classes right now, e.g. implode(') OR (', $clauses). This won't work if we make the semantics safer.
- Fix is probably something similar to phutil_implode_html(), maybe qsprintf_join($conn_r, $pattern, $query_parts, $between) or something:
qsprintf_join($conn_r, '(%s)', $clauses, ') OR (');
Maybe cleaner as:
qsprintf_join(conn, clauses, before, between, after); qsprintf_join($conn_r, $clauses, '(', ') OR (', ')');
Or maybe that's overthinking it, this seems OK:
$clauses = qsprintf_implode($conn_r, $clauses, ') OR ('); $clauses = qsprintf($conn_r, '(%s)', $clauses);
In 95% of cases, we should be able to bury that in a buildJoinClause() or buildWhereClause() call anyway, so the simpler method is probably cleaner.
So overall approach is probably:
- Implement qsprintf_implode() with current semantics.
- Go through Phabricator and convert all manual mangling of queries to use qsprintf() or qsprintf_implode(). Most of this can be accomplished by lifting clauses construction into the base query class, probably.
- Change qsprintf() semantics so it returns an object.
- Allow %s (or maybe a new conversion, since %s has some level of existing semantics -- but the important part is that this would be a safe conversion, not %Q) to convert this object without double-escaping.
- Convert as many %Q to %s (or the new conversion) as possible.
- Implement %P for queries.
Beyond giving us %P, this would significantly improve our resistance to SQL injection (by reducing manual query manipulation to near-zero and making it very difficult to get wrong).