Provide a better error message when an invalid ID is given to arc patch
ClosedPublic

Authored by jcox on Wed, Feb 8, 2:02 PM.

Details

Summary

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.)
Test Plan

Ran arc patch with a valid revision and saw it patch successfully. Ran again with an invalid revision, saw the error message.

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jcox created this revision.Wed, Feb 8, 2:02 PM
epriestley accepted this revision.Wed, Feb 8, 5:16 PM

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.

src/workflow/ArcanistWorkflow.php
1201

For consistency of editorial voice, prefer a complete sentence with a period at the end.

This revision is now accepted and ready to land.Wed, Feb 8, 5:16 PM
jcox added a comment.Wed, Feb 8, 5:44 PM

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.

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:

  • You don't have permission to see something or it doesn't exist, but you ran a public command (like arc patch) in anonymous mode, and you might have permission to see it if you logged in, so the immediate fix might be to login (that is, the best guidance we could give the user is "invalid or you don't have permission, you may need to log in").
  • You used some valid set of credentials and the thing is invalid or you can't see it.

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.

jcox updated this revision to Diff 41667.Wed, Feb 8, 7:29 PM

Update message to account for permission error

jcox added a comment.Wed, Feb 8, 7:36 PM

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.

This revision was automatically updated to reflect the committed changes.