Page MenuHomePhabricator

if phabricator users are blocked due to IP range blocks send a 4xx rather than 500
Closed, ResolvedPublic


see T229575

A Phabricator user is blocked because of his IP range and a security incident but he is served a 500 Internal Server Error.

That leads to ticket creation and debugging from some people what's wrong with Phab.

Let's send a status code that actually fits the real reason, 401 or 403 (Unauthorized / Forbidden) instead and avoid the 5xx ?

Event Timeline

Dzahn created this task.Aug 1 2019, 8:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 1 2019, 8:36 PM

I am neither sure where this code is located nor if it is public... Also see non-public T218589 etc.

sbassett triaged this task as Medium priority.Aug 1 2019, 9:55 PM
sbassett set Security to Software security bug.
sbassett added a project: acl*security.
sbassett changed the visibility from "Public (No Login Required)" to "Custom Policy".
sbassett added a subscriber: sbassett.

Related to: T218784

Protecting for now as this is directly related to a specific mitigation for an ongoing security incident. The related private task is: T218784.

As long as nothing secret is discussed or posted in this task I do not see any reason to protect this task? (Mere Phab IDs are not secret.)

@Aklapper - if you want to revert, feel free. My default instinct is to protect/obfuscate mitigations of this nature as much as possible, even Phab IDs for protected tasks.

I think it makes sense to have this public. The fact user is blocked because a security incident can be public, and probably would be mentioned to anyone asking why they sees 500. This is motivated by said incident, but it's really not a mitigation of an incident, it's a mitigation of no good block at Phab instead. However, I'm not comfortable with republishing when it was protected by a security team staff member, unless explicitly invited to do so.

Dzahn added a comment.Aug 30 2019, 7:12 AM

I agree this does not seem to be a mitigation of a security issue. It's just about not creating false positive reports for users getting 500 errors when in reality there is no server side error and stuff works as designed. This should save us time.

Concensus seems to exist, but no one is willing to actually publish? :-) Anyway, +1 to "this should save us time".

Dzahn added a comment.Aug 30 2019, 7:35 AM

I expect that keeping the ticket secret and sending a false positive 500 will mean more (initially public) tickets about it from confused users in the future. Then we would have to keep hiding those too and keep explaining to just one person at a time. And only a few users with access to secret tickets would even know and be able to handle them.

Even "those few people" can spend more time than needed on looking into that, cf T230521, where I actually debugged something that is a mitigation of a security incident. Anyway, agreed.

sbassett added a comment.EditedSep 3 2019, 4:32 PM

Discussed with the Security-Team this morning - we're fine making this task public as long as no sensitive issues from the related, protected tasks are discussed here.

sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 3 2019, 4:32 PM

Discussed with the Security-Team this morning - we're fine making this task public as long as no sensitive issues from the related, protected tasks are discussed here.

Acknowledged, thanks. Merely publishing IDs should be fine IMO. Can I say "cf T230304"?

@Urbanecm - Task IDs and even protected Phabricator project names should be fine, as those aren't published or accessible for unprivileged Phabricator/anon users. It's really just some of the sensitive details within certain protected tasks which should never be made public or discussed on related public tasks. Thanks.

alaa added a subscriber: alaa.Nov 4 2019, 7:01 PM
Meno25 added a subscriber: Meno25.Jun 6 2020, 7:20 AM
Dzahn closed this task as Resolved.Dec 16 2020, 4:45 PM
Dzahn claimed this task.

Apparently this has been fixed by someone somewhere (Nice, but would have been great to get an update here).

T270184#6695521 shows users are now getting 403