Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds allowedUserFields option to define the only data that can be returned to client by dbAuth functions #9374

Merged
merged 7 commits into from
Dec 19, 2023

Conversation

cannikin
Copy link
Member

@cannikin cannikin commented Nov 2, 2023

This is the result of a report to our security email. dbAuth has a function _sanitizeUser() which removes sensitive fields from being returned to the client when calling functions like login or forgotPassword. It removes fields like hashedPassword and salt. However, this is a deny list, not an allow list, so if you added your own potentially sensitive fields, they could be sent along in responses, depending on what data you chose to return from those functions.

This PR reverses the strategy and instead makes you list the only fields that are allowed to be sent back. This will be a breaking change in two ways:

  1. There is a fallback that if the new allowedUserFields option is not defined we'll assume ["id", "email"] by default, which is probably the smallest list that most people are using. So the majority of users can install this release, make no code changes, and be fine. But, if people were counting on additional fields being returned by default, they now won't be, thus the breaking-ness.
  2. The signature to the forgotPassword handler has changed, adding a second argument. See the forgotPassword Handler Signature section below.

The config set in api/src/functions/auth.js will now look something like this (comments removed for clarity):

const forgotPasswordOptions = {
- handler: async (user) => {
+ handler: async (user, resetToken) => {
    return user
  },
  expires: 60 * 60 * 24,
  errors: {
    usernameNotFound: 'Username not found',
    usernameRequired: 'Username is required',
  },

// more config...

const authHandler = new DbAuthHandler(event, context, {
  db: db,
+ allowedUserFields: ["id", "email"],
  authModelAccessor: 'user',
  authFields: {
    id: 'id',
    username: 'email',
    hashedPassword: 'hashedPassword',
    salt: 'salt',
    resetToken: 'resetToken',
    resetTokenExpiresAt: 'resetTokenExpiresAt',
  },
  cookie: {
    HttpOnly: true,
    Path: '/',
    SameSite: 'Strict',
    Secure: process.env.NODE_ENV !== 'development',
  },
  forgotPassword: forgotPasswordOptions,
  login: loginOptions,
  resetPassword: resetPasswordOptions,
  signup: signupOptions,
})

These new options have been added to the template for the file that's created when running yarn rw setup auth dbAuth so new apps will get these options automatically.

forgotPassword Handler Signature

This PR includes an argument change to the forgotPassword handler. Previously, a single user object was passed which would contain the resetToken to be emailed to the user. Since we're now removing everything that isn't in the allowedUserFields list, this value would have been stripped out and not made available without some hacky workarounds. Instead, it's now sent in a second argument to keep the user object "pure," and a comment has been added to the template explaining how to use it.

Codemod?

I don't think we can really codemod the new allowedUserFields option because it will depend on which fields you may or may not have decided to return in those functions. We could simply add the allowedUserFields: ["id", "email"], line, but it will now strip out any custom fields you may have been returning, and this would be unexpected: the spirit of codemods is "this will do all the work for you, don't worry" but in this case that's not really possible.

We could codemod the new resetToken argument, although what you did with it inside of that function could be harder. We could assume that any previous usage was of user.resetToken and just change it to resetToken, but if they did anything else special with it the codemod wouldn't know what to do there.

I propose we just make a thorough Release Notes entry that explains the minimal changes to make.

TODO

  • Docs update

@cannikin cannikin added topic/auth release:breaking This PR is a breaking change fixture-ok Override the test project fixture check topic/security labels Nov 2, 2023
@cannikin cannikin added this to the v7.0.0 milestone Nov 2, 2023
@cannikin cannikin changed the title Adds allowedUserFields option to allow-list fields that can be returned by dbAuth functions Adds allowedUserFields option to allow-list fields that can be returned to client by dbAuth functions Nov 2, 2023
@jtoar jtoar self-requested a review November 3, 2023 04:55
@cannikin cannikin changed the title Adds allowedUserFields option to allow-list fields that can be returned to client by dbAuth functions Adds allowedUserFields option to define the only data that can be returned to client by dbAuth functions Nov 3, 2023
Copy link
Contributor

@jtoar jtoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @cannikin!

@jtoar
Copy link
Contributor

jtoar commented Nov 3, 2023

@cannikin i'll leave it to you if you want to include those docs updates in this PR; if not feel free to merge.

@jtoar jtoar modified the milestones: SSR, v7.0.0 Dec 13, 2023
@cannikin
Copy link
Member Author

Documented!

@Tobbe Tobbe merged commit 208af67 into main Dec 19, 2023
32 checks passed
@Tobbe Tobbe deleted the rc-dbauth-user-fields branch December 19, 2023 04:59
Tobbe pushed a commit that referenced this pull request Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:breaking This PR is a breaking change topic/auth topic/security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants