feat: Return more 404s vs 403s #2194
Conversation
There was a problem hiding this comment.
So, just generally speaking, this PR returns 404 correctly, but is not comprehensive in the sense that there are still cases where we return 403s when we shouldn't---and that's a security leak.
I'm uncomfortable, but conflicted, about whether to support a PR that makes correct changes, but leaves us in a state where we leak info. Would it be worth pairing on chasing down the other places that we should return 404?
coderd/files.go
Outdated
| @@ -86,7 +86,9 @@ func (api *API) fileByHash(rw http.ResponseWriter, r *http.Request) { | |||
| } | |||
| file, err := api.Database.GetFileByHash(r.Context(), hash) | |||
| if errors.Is(err, sql.ErrNoRows) { | |||
| httpapi.Forbidden(rw) | |||
| httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ | |||
There was a problem hiding this comment.
this reintroduces the security concern that now you can distinguish between whether it exists or not even if you have no access. We should also return 404 on line 102 of this file.
There was a problem hiding this comment.
I see what you mean. Before we were returning 403 to be consistent, but that is not a good UX.
If authorize returns a 404 in some cases, can that be a bad UX?
My question is mainly, are these security concerns ones we should take as important right now. If we return a 403 vs 404 we do leak the info so the user knows if the resource exists or not. Is that a security issue we should take seriously right now? (Honest question)
There was a problem hiding this comment.
Security impact is minor in a real sense, but paints us in a "these people don't know what they're doing" light to security minded people, so IMO the impact is basically on par with the confusion we had before.
There was a problem hiding this comment.
So it look like I need to make Authorize return a 404 message consistent with the existing 404 message
There was a problem hiding this comment.
Yeah. It would be good to have one 404 code path that we use everywhere so we can standardize messaging.
There was a problem hiding this comment.
@spikecurtis Will fix this now.
|
A high level problem is that The high level change we should make is ensuring that all 404s look the same: same message etc. Easy to leak information by sending one message if the object exists and a different one if it really doesn't. |
| @@ -411,7 +418,8 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { | |||
|
|
|||
| // admins can change passwords without sending old_password | |||
| if params.OldPassword == "" { | |||
| if !api.Authorize(rw, r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { | |||
| if !api.Authorize(r, rbac.ActionUpdate, rbac.ResourceUser.WithID(user.ID.String())) { | |||
There was a problem hiding this comment.
as an aside, it is very unintuitive to me that this check should fail for my own user.
There was a problem hiding this comment.
Why not? There is ResourceUser and ResourceUserData. The former cannot be edited by "me"
There was a problem hiding this comment.
I guess that's just it. Why can't I edit my own "ResourceUser" resource? The code says
// ResourceUser is the user in the 'users' table.
// ResourceUser never has any owners or in an org, as it's site wide.
// create/delete = make or delete a new user.
// read = view all 'user' table data
// update = update all 'user' table data
ResourceUser = Object{
Type: "user",
}
Things like email and password are in the users table, and users can change them, no?
There was a problem hiding this comment.
Yea, I need to fix that comment. The comment is incorrect as I since placed email as a ResourceUserData. There was some thrashing there and now the comment is stale.
My mistake
Issue: #1899
403 -> 404 changes