Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Returning 403 when record not in scope? #76

Open
Justin-Maxwell opened this issue Aug 19, 2017 · 4 comments
Open

Returning 403 when record not in scope? #76

Justin-Maxwell opened this issue Aug 19, 2017 · 4 comments

Comments

@Justin-Maxwell
Copy link

Justin-Maxwell commented Aug 19, 2017

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...

@valscion
Copy link
Member

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 ☺️

@joshkinabrew
Copy link

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.

@blindMoe
Copy link

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'

@valscion
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants