volatiletech / authboss

The boss of http auth.
MIT License
3.85k stars 207 forks source link

v1.2.0 with register and confirm modules passes plain-text password to storer. #237

Closed antonovvk closed 5 years ago

antonovvk commented 5 years ago

Hi! If v1 is still used by anyone, this requires fixing. Here's the diff which reproduces it in tests:

--- a/register/register_test.go
+++ b/register/register_test.go
@@ -138,6 +138,12 @@ func TestRegisterPostSuccess(t *testing.T) {
                t.Error(err)
        }

+       // As per v1.2.0 confirm.afterRegister() callback calls ctx.SaveUser() on directly passed ctx.User
+       // Which results in exposing "confirm_password" field with plaintext password to Storer
+       if pass, ok := ctx.User.String(authboss.ConfirmPrefix + authboss.StorePassword); ok {
+               t.Errorf("Confirm password was not removed: %s", pass)
+       }
+
        if w.Code != http.StatusFound {
                t.Error("It should have written a redirect:", w.Code)
        }
aarondl commented 5 years ago

Hi @antonovvk, thanks for reporting this. Just trying to understand exactly what's going on.

Are you saying that confirm.AfterRegister() gets called, which then calls ctx.SaveUser but the User has the plaintext password inside him at that point?

antonovvk commented 5 years ago

Hi @aarondl, yes, register module uses AttributesFromRequest() at register.go:113

  attr, err := authboss.AttributesFromRequest(r) // Attributes from overriden forms

to get forms values, and automatically sets 'confirm_password' field (with the plaintext password from the confirm form input) to the attrs (which is user). Which is later passed to confirm, and is saved to DB with SaveUser().

aarondl commented 5 years ago

Should be fixed in v1.2.1. I've also added a full deprecation notice for v1 and it should no longer be used as outlined in the upgrade plan to v2 in the README. Thanks for the report and the useful test.

antonovvk commented 5 years ago

Thank you! Will be migrating to v2 then, and switch to v1.2.1 until migration is done.