fix: Auth middleware params should not be mutated #261
Conversation
Applying authentication options should happen inside the handler to avoid the authorization parameters from getting mutated on each handler execution.
| if paramsErr != nil { | ||
| w.WriteHeader(http.StatusUnauthorized) | ||
| return | ||
| params := &AuthorizationParams{} |
There was a problem hiding this comment.
💡 Should we maybe also avoid passing pointers to params fields such as in line 70?
claims, err := jwt.Verify(r.Context(), ¶ms.VerifyParams)There was a problem hiding this comment.
I thought about it as well, but in this case at least I believe it's not "our" fault and the call chain won't modify something surprisingly?
There was a problem hiding this comment.
Agreed but on the other hand is there any point in passing a pointer? 🤔 Raising this now since we won't be able to change this later, it's a breaking change.
There was a problem hiding this comment.
Do you mean changing all the SDK's methods to accept parameters as non-pointer types?
There was a problem hiding this comment.
Unless the nil check is necessary or convenient, generally I don't see benefit in using pointers but I may be missing something. I was mostly referring to jwt.Verify though since it can encounter conditions where parameter objects are propagated through multiple layers. I can't imagine other methods having the same kind of flow, parameters will be probably constructed just near the call!
There was a problem hiding this comment.
I see, thanks for the explanation. I understand the concern, but I'm not sure what's best from a design perspective.
So you're suggesting that all SDK methods accept pointers except jwt.Verify? What about jwt.Decode? Does it fall in the same category?
What's the general heuristic for deciding whether we should accept a pointer or non-pointer?
Is it ok to have all SDK methods accept pointers to structs (what we do now) except for one or two methods?
There was a problem hiding this comment.
Is it ok to have all SDK methods accept pointers to structs (what we do now) except for one or two methods?
If you need to detect nil (no parameters) I think it's fine. jwt.Verify seems to receive options through this object and not request parameters. That might be a key difference.
Applying authentication options should happen inside the handler to avoid the authorization parameters from getting mutated on each handler execution.
This was caught by a test in an upcoming PR. Sadly, it's hard to write a test to catch the bug here.