Page MenuHomePhabricator

Support %P in qsprintf()
Closed, ResolvedPublic

Description

  • 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).

Revisions and Commits

Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision

Event Timeline

epriestley raised the priority of this task from to Low.
epriestley updated the task description. (Show Details)
epriestley added a subscriber: epriestley.
epriestley added a revision: Restricted Differential Revision.Nov 7 2018, 12:34 AM
epriestley added a revision: Restricted Differential Revision.Nov 7 2018, 12:45 AM
epriestley closed this task as Resolved by committing Restricted Diffusion Commit.Nov 13 2018, 4:48 PM
epriestley added a commit: Restricted Diffusion Commit.
epriestley added a commit: Restricted Diffusion Commit.
epriestley added a revision: Restricted Differential Revision.Nov 15 2018, 1:28 PM
epriestley added a revision: Restricted Differential Revision.Nov 15 2018, 1:31 PM
epriestley added a commit: Restricted Diffusion Commit.Nov 16 2018, 8:36 PM
epriestley added a revision: Restricted Differential Revision.Nov 17 2018, 1:21 AM
epriestley added a commit: Restricted Diffusion Commit.Nov 19 2018, 3:39 PM
epriestley added a commit: Restricted Diffusion Commit.Nov 21 2018, 3:55 PM