Page MenuHomePhabricator

Allow setup checks to perform writes
ClosedPublic

Authored by epriestley on May 14 2015, 5:21 PM.
Tags
None
Referenced Files
F14354233: D12842.id30890.diff
Thu, Dec 19, 5:05 PM
F14352601: D12842.diff
Thu, Dec 19, 2:15 PM
Unknown Object (File)
Mon, Dec 9, 3:32 AM
Unknown Object (File)
Sat, Dec 7, 12:56 PM
Unknown Object (File)
Sat, Dec 7, 10:44 AM
Unknown Object (File)
Fri, Dec 6, 1:15 PM
Unknown Object (File)
Mon, Dec 2, 6:24 PM
Unknown Object (File)
Nov 12 2024, 7:54 AM
Subscribers

Details

Summary

Fixes T8198. Currently, if the policy.locked configuration setting includes a value which is a user PHID, we may perform a cache fill during setup as a side effect of validating it.

Right now, there is no WriteGuard active during setup, because we don't have a Request object yet so we can't actually perform CSRF validation.

Two possible approaches are:

  1. Prevent the write from occuring.
  2. Change the code to allow the write.

In the past, I think we've hit similar cases and done (1). However, IIRC those writes were sketchier, more isolated, and easy to remove (I think there was one with PKCS8 keys). This one is pretty legit and not very easy to remove without making a bit of a mess.

There's no techncial reason we can't do (2), we just have to create a no-op WriteGuard for the setup phase.

Test Plan
  • To reproduce this issue: set some value in policy.locked to a user PHID, then wipe out profile caches in the database, then restart the webserver.
  • Reproduced the issue.
  • Added the new dummy write guard, fixed a minor issue with disposal semantics (see D12841).
  • Verified this fixed the issue.
  • Added a throw to the real CSRF validator and performed a real write. Verified I got CSRF-blocked.
  • Removed a CSRF token from a form and double-checked that CSRF protection still works.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Allow setup checks to perform writes.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
  • "actually checks for errors" -> "actually checks CSRF tokens".
btrahan edited edge metadata.
This revision is now accepted and ready to land.May 14 2015, 5:37 PM

(An alternative would be to let you just setCallback() on the WriteGuard instead of having to destroy it and create a new one, but sort of conceptually like that they're immutable and it's very hard to accidentally break CSRF protection.)

This revision was automatically updated to reflect the committed changes.