Differential D17325
Provide a better error message when an invalid ID is given to arc patch Authored by jcox on Feb 8 2017, 2:02 PM. Tags None Referenced Files
Subscribers
Details
Fixes T8937. Previously when running arc patch D9999999999 or arc export --revision 99999999 with a non-existent diff or revision ID you would get a rather unhelpful error message. Now you'll get a slightly more helpful error message: $ arc patch D99999999 Exception Couldn't find a revision or diff that matches the given ID (Run with `--trace` for a full exception trace.) Ran arc patch with a valid revision and saw it patch successfully. Ran again with an invalid revision, saw the error message.
Diff Detail
Event TimelineComment Actions In theory, $params might not have anything like an ID in it so this error message might not make sense, but in practice it currently always does and I expect all this code to get a Very Stern Look At before that assumption changes.
Comment Actions I just ran into this again but for a completely different reason. The user assured me multiple times that they were entering a valid diff ID and I repeatedly explained, "no, you don't understand. I Know What I'm Doing™". Anyway... it turns out that this same error also occurs if you have a cli token installed but don't have permissions to view the diff in question. I _think_ the "Couldn't find a revision..." message still makes sense in that context but I may want to expound a bit more to explain that it could also be a permissions issue. Comment Actions Yeah, something along the lines of "it's invalid or you don't have permission to see it" should be correct and slightly more clear. More broadly, there are two sub-cases here:
We deal with the first case terribly in master but significantly better in experimental. We don't do any better about the second case in experimental. And there's currently no way to distinguish between invalid and restricted objects with *.search methods. This is partly by design. If we let you see restricted matches, you can query tasks for "yelirekim's credit card number is 1", "yelirekim's credit card number is 2", etc., until you get a restricted match instead of 0 matches. Then you can try "41", "42", etc. Or search for tasks tagged "security" which match "xss" in the title or whatever to learn when XSS is discovered, then refine the search to narrow down where it is. There's a broader discussion of this class of information disclosure in T11773, particularly T11773#197922. A subset of queries (basically, queries with IDs or PHIDs only, with a neutral ordering and no grouping) are safe to return "restricted" vs "invalid" on, but we don't currently have a way to identify safe queries. Since fixing this is complicated and the utility is narrow (mostly just improving error messages, I think?), I'd expect these errors to remain "invalid or you don't have permission to see it" for a while rather than be able to distinguish between those cases, at least in general. In specific narrow cases like this, it might be OK to call phid.lookup or something similar to distinguish between "invalid" and "restricted", but today there's no way to pass a Diff ID to that call and it's fairly ancient and probably has other limitations. Comment Actions Yeah I think it probably makes sense to not distinguish between a permission error and invalid resource for the reasons you said. I know amazon S3 does a similar thing and just returns a 403 in both cases. | ||||||||||||||||||||||||||||||||||||||