handle 401 when checking cookie session #300
Conversation
| @@ -115,10 +115,12 @@ func forwardRequestToSessionStore(r *http.Request, checkSessionURL string) (json | |||
| if res.StatusCode == 200 { | |||
| body, err := ioutil.ReadAll(res.Body) | |||
| if err != nil { | |||
| return json.RawMessage{}, err | |||
| return json.RawMessage{}, helper.ErrForbidden.WithReason(err.Error()).WithTrace(err) | |||
There was a problem hiding this comment.
- Why would you consider the inability to read the response body a 403 error? Wouldn't it make more sense to return more error details here? I believe this will be incredibly hard to handle. I understand that this was the case before, but since we're working on this patch we can address it straight away.
.WithTrace()adds the stack trace from the upstream error to the error context. stdlib errors do not implement stack trace functionality, thereforeWithTrace()on a stdlib doesn't have an effect.- Without
errors.WithStack()it's not possible to get the error trace. What was your thinking behind omitting that?
There was a problem hiding this comment.
I added that here because the function that called forwardRequestToSessionStore was previously wrapping all the errors in a 403, so I added this to maintain the existing behavior for all of the other error cases, though I would say this should probably be a 500 error instead
There was a problem hiding this comment.
Ok, so let's return instead:
| return json.RawMessage{}, helper.ErrForbidden.WithReason(err.Error()).WithTrace(err) | |
| return json.RawMessage{}, errors.WithStack(helper.ErrForbidden.WithReasonf("Unable to fetch cookie session context from remote: %+v", err)) |
| } | ||
|
|
||
| return json.RawMessage{}, errors.WithStack(helper.ErrUnauthorized) |
There was a problem hiding this comment.
These changes do not do a lot except changing some errors.WithStack() behavior, which should not have any effect.
|
Are you still up for these changes? :) |
|
I think the change makes sense in general, but I'm not using the cookie auth anymore since the patch to use cookies for oauth was merged, so it's not a priority for me anymore. |
Related issue
#298
Proposed changes
Handles 401 errors when attempting to check a cookie session rather than masking it as a 403 error
Checklist
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
developer guide (if appropriate)