Page MenuHomePhabricator

Fix a bug where "View as Query" could replace a saved query row by ID, causing workboard 404s
ClosedPublic

Authored by epriestley on Nov 1 2018, 12:28 AM.

Details

Summary

Fixes T13208. See that task for details.

The clone $query line is safe if $query is a builtin query (like "open").

However, if it's a saved query we clone not only the query parameters but the ID, too. Then when we save() the query later, we overwrite the original query.

So this would happen in the database. First, you run a query and save it as the workboard default (query key "abc123"):

123abc123{"...xxx..."}

Then we clone it and change the parameters, and save() it. But that causes an UPDATE ... WHERE id = 123 and the table now looks like this:

123def456{"...yyy..."}

What we want is to create a new query instead, with an INSERT ...:

123abc123{"...xxx..."}
124def456{"...yyy..."}
Test Plan
  • Followed reproduction steps from above.
    • With just the new save() guard, hit the guard error.
    • With the newCopy(), got a new copy of the query and "View as Query" remained functional without overwriting the original query row.
  • Ran migration, saw an affected board get fixed.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Nov 1 2018, 12:28 AM
Owners added a subscriber: Restricted Owners Package.Nov 1 2018, 12:28 AM
epriestley requested review of this revision.Nov 1 2018, 12:29 AM
epriestley marked an inline comment as done.Nov 1 2018, 12:37 AM
epriestley added inline comments.
src/applications/search/engine/PhabricatorApplicationSearchEngine.php
106–112

This should prevent this class of bug from ever occurring again. I made an effort to grep for other misuses of clone but didn't turn any up.

joshuaspence accepted this revision.Nov 1 2018, 1:58 AM
This revision is now accepted and ready to land.Nov 1 2018, 1:58 AM
This revision was automatically updated to reflect the committed changes.