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.
Tags
None
Referenced Files
F13050486: D19768.id47222.diff
Fri, Apr 19, 2:56 AM
F13050485: D19768.id47219.diff
Fri, Apr 19, 2:56 AM
F13050484: D19768.id.diff
Fri, Apr 19, 2:56 AM
F13049668: D19768.diff
Fri, Apr 19, 2:22 AM
Unknown Object (File)
Thu, Apr 11, 9:13 AM
Unknown Object (File)
Thu, Apr 11, 4:10 AM
Unknown Object (File)
Wed, Mar 20, 10:32 PM
Unknown Object (File)
Mar 16 2024, 10:33 AM
Subscribers
Restricted Owners Package

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a subscriber: Restricted Owners Package.Nov 1 2018, 12:28 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.

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.