jsonapi-authorization icon indicating copy to clipboard operation
jsonapi-authorization copied to clipboard

Returning 403 when record not in scope?

Open Justin-Maxwell opened this issue 6 years ago • 4 comments

Hi.

How do we return a 403 instead of 404 on Update, when a record exists, but the Policy Scope excludes the record for the current user.

From what I can tell, PunditScopedResource is scoping the update request (fine, makes sense) and so JSONAPI::Resource#find_by_key raises RecordNotFound.

Am I missing something? Does it make sense for single record operations to have an additional .exists? check to distinguish between unauthorized and non-existent (and perhaps trigger Pundit::NotAuthorizedError handling?)

NB: I tried including JSONAPI::Exceptions::RecordNotFound in config.exception_class_whitelist, but it still doesn't seem to get passed back to the controller...

Justin-Maxwell avatar Aug 19 '17 19:08 Justin-Maxwell

Hi,

thanks for the question! Sorry for my response being delayed, as I was on vacation.

Returning 404 is actually the intended behavior currently. I agree that it can be surprising, though. We don't have a strong opinion on this subject.

I think the initial reasoning behind this decision was to disallow querying for resource presences. Kind of the way if you go check out https://github.com/venuu/venuu it will 404 for you instead of 403 — it is a private repository that others are not even authorized to know whether it exists or not.

Do you think this makes sense? I'd love to hear your opinions on this matter, and if at all possible, help us clarify this in the documentation somewhere :relaxed:

valscion avatar Aug 28 '17 07:08 valscion

Personally I agree on it needing to be a 404 and not 403. The less the user knows about authorization and why they aren't able to view something the better. A 404 means that it just doesn't exist. If a malicious person was trying to find a record and got a 403, they would then think that it exists and that there's some way to access it and potentially keep probing for holes. This is security by obscurity and by no means a comprehensive policy but the 404 generally helps to deter malicious intent.

I actually ran into this today wondering why a record wasn't there when I could query in the database and realized my Pundit scope was doing its job giving a 404 because it was out of the scope of my permissions.

I think a 403 is appropriate for a PUT, POST, DELETE etc, not necessarily always for a GET.

joshkinabrew avatar Nov 07 '17 19:11 joshkinabrew

So in 0.8 this was a 403 and now it is a 404 right? I am updating to 1.0.0.alpha6 and am noticing my specs failing with 'expected 403 got 404'

blindMoe avatar Jul 26 '18 23:07 blindMoe

Could be so @blindMoe. I just came back from vacation and I can't remember off the top of my head.

Would you be able to check this by looking at the current specs in this repository, and report back your findings? It could be that this issue is closeable.

valscion avatar Jul 31 '18 08:07 valscion