redwood icon indicating copy to clipboard operation
redwood copied to clipboard

[Bug?]: Default dbAuth workflow leaks resetToken

Open maddijoyce opened this issue 2 years ago • 10 comments

What's not working?

After following the tutorial to get an idea of how redwood works, I was testing out the dbAuth functionality with the console open. I was looking at the response from the forgotPassword lambda (without changing anything from the scaffolded code) and it's returning the reset token:

{
  "id": "b6e9b773-ecc1-4d27-bcdb-aa6729255bfe",
  "email": "[email protected]",
  "resetToken": "ZTEyNzI3YjdjMjc1", // <-- This token allows me to reset
  "resetTokenExpiresAt": "2022-09-06T11:35:07.169Z",
  "createdAt": "2022-09-05T10:06:06.648Z"
}

How do we reproduce the bug?

Just follow the instructions here - https://redwoodjs.com/docs/auth/dbauth

At it's most basic, on a brand new redwood repo run:

  • yarn rw setup auth dbAuth
  • yarn rw g dbAuth

What's your environment? (If it applies)

System:
    OS: macOS 12.5
    Shell: 3.3.1 - /opt/homebrew/bin/fish
  Binaries:
    Node: 16.13.0 - /private/var/folders/79/3b3vgxfs4633nxwvbjs0c0y00000gn/T/xfs-09c55961/node
    Yarn: 3.2.1 - /private/var/folders/79/3b3vgxfs4633nxwvbjs0c0y00000gn/T/xfs-09c55961/yarn
  Databases:
    SQLite: 3.37.0 - /usr/bin/sqlite3
  Browsers:
    Chrome: 96.0.4664.55
    Firefox: 104.0.1
    Safari: 15.6
  npmPackages:
    @redwoodjs/core: 2.2.3 => 2.2.3

Are you interested in working on this?

  • [X] I'm interested in working on this

maddijoyce avatar Sep 05 '22 11:09 maddijoyce

My feeling is that there are 2 possible options here.

  • One would be to update the documentation to make sure users understand they need to modify that handler
  • The other would be to modify the generated function, possibly as simple as:
handler: (user) => {
    // Ensure the reset token is not sent to the client
    delete user.resetToken;
    return user;
  },

maddijoyce avatar Sep 05 '22 11:09 maddijoyce

Thanks for the report @maddijoyce! @cannikin could I hand this one off to you cause dbAuth?

jtoar avatar Sep 05 '22 12:09 jtoar

Yup, I’m on it!

On Sep 5, 2022, at 8:30 AM, Dominic Saadi @.***> wrote:

 Thanks for the report @maddijoyce! @cannikin could I hand this one off to you cause dbAuth?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

cannikin avatar Sep 05 '22 15:09 cannikin

@dac09 Didn't we just update this function to only return a boolean, or throw an error? Was that in the TS strict stuff?

cannikin avatar Sep 07 '22 15:09 cannikin

@dac09 Didn't we just update this function to only return a boolean, or throw an error? Was that in the TS strict stuff?

Yeah we changed the template - but we mainly changed the templates though, but didn't change the dbAuthHandler underneath to throw an error, so I think it still needs a looking at.

dac09 avatar Sep 07 '22 16:09 dac09

I don't know that we need to throw an error, just update this function to not return the same thing that came from that forgotPassword handler: https://github.com/redwoodjs/redwood/blob/main/packages/api/src/functions/dbAuth/DbAuthHandler.ts#L500-L509 So even if someone DID return something sensitive, the actual function would return nothing. Just update it to return an empty string or something.

Then update ForgotPasswordPage to use the email from the form field on the page, instead of getting it from the response object: https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/generate/dbAuth/templates/forgotPassword.tsx.template#L33

cannikin avatar Sep 07 '22 16:09 cannikin

Did you want to work on this one @maddijoyce? How does my suggestion above sound?

cannikin avatar Sep 07 '22 16:09 cannikin

Ah yeah that makes sense to me. Don't just limit the chance for leakage in the tutorial/generated code. Instead eliminate it entirely in the DbAuthHandler.

I'd love to have a crack at this, so I'll check the contributor guide and push a PR. Cheers!

maddijoyce avatar Sep 08 '22 00:09 maddijoyce

~I marked it as a breaking change: if someone is doing something with the user object returned by that function then they're screwed. Unfortunately, that's everyone running dbAuth since the toast message it shows on the ForgotPasswordPage gets the email address from the response object. 😬 So we'd have to release this with 4.0 at the earliest.~

Turns out it was easier to just strip resetToken and resetTokenExpiresAt from the handler, which will NOT be breaking.

cannikin avatar Sep 08 '22 01:09 cannikin

We do have the ability to write codemods, so that your files are automatically updated by us during the yarn rw upgrade process, but it's no picnic writing them. And depending on how much someone has customized their forgotPassword hander or page, they might be impossible to run. In which case we'd have to have release notes anyway explaining how to make the changes. Personally, I'm inclined to just have Release Notes tell you what to do, but if you want to take a stab at a codemod @maddijoyce be my guest! :)

cannikin avatar Sep 08 '22 17:09 cannikin

Hey @maddijoyce any chance you still want to take this one? No problem if not, just say the word and I'll add to my own queue. :)

cannikin avatar Sep 28 '22 20:09 cannikin