-
Notifications
You must be signed in to change notification settings - Fork 995
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
changed the title
Adds
Adds Nov 2, 2023
allowedUserFields
option to allow-list fields that can be returned by dbAuth functionsallowedUserFields
option to allow-list fields that can be returned to client by dbAuth functions
cannikin
changed the title
Adds
Adds Nov 3, 2023
allowedUserFields
option to allow-list fields that can be returned to client by dbAuth functionsallowedUserFields
option to define the only data that can be returned to client by dbAuth functions
jtoar
approved these changes
Nov 3, 2023
There was a problem hiding this 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!
@cannikin i'll leave it to you if you want to include those docs updates in this PR; if not feel free to merge. |
Documented! |
Tobbe
pushed a commit
that referenced
this pull request
Dec 22, 2023
…eturned to client by dbAuth functions (#9374)
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 likelogin
orforgotPassword
. It removes fields likehashedPassword
andsalt
. 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:
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.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):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 SignatureThis PR includes an argument change to the
forgotPassword
handler. Previously, a singleuser
object was passed which would contain theresetToken
to be emailed to the user. Since we're now removing everything that isn't in theallowedUserFields
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 theuser
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 theallowedUserFields: ["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 ofuser.resetToken
and just change it toresetToken
, 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