Add auth update endpoint #48

Mathieuka · 2024-10-28T21:54:41Z

Description

Fixes #36

Key Changes

Allows authenticated users to change their passwords using a PUT request with schema validation.
Enforces a strong password policy requiring uppercase, lowercase, numeric, and special characters.
Rate Limiting: Limits password update attempts to 3 per minute to enhance security.
Includes tests for successful updates, validation errors, and rate limiting.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

src/routes/api/auth/index.ts

jean-michelet

Thanks for the PR!

Always add a link to the issue when you push a PR, e.g. Fixes #36
It allows people following the issue to be notified and readers of the issue to know that a PR has been pushed.

Your branch is out of date, you need to sync your fork with this repo, pull the changes and resolve conflicts.

/auth is related to authentication, it must be used for login, logout, reset password, etc.

Regarding the CRUD of user details, you need to create a new folder routes/api/users (the autoloader will automatically prefix all the routes defined in this folder with path /api/users).

I wouldn't mix password update with other user details update, as password update require specific logic and is more security sensible. Most of the time, updating user details only require payload validation.

src/routes/api/auth/index.ts

src/schemas/auth.ts

Mathieuka · 2024-10-29T20:25:46Z

This is my first PR, I'm waiting for your review before committing to writing a test

jean-michelet

You need to write tests that prove that the feature work as expected and reach 100% code coverage: https://en.wikipedia.org/wiki/Code_coverage

jean-michelet · 2024-10-29T20:24:24Z

src/routes/api/user/index.ts

+          .where({ username })
+          .first()
+
+        if (user) {


Suggested change

if (user) {

if (!user) {

Prefer early return, if there is no user, the program can stop.

jean-michelet · 2024-10-29T20:25:26Z

src/routes/api/user/index.ts

+      const username = request.session.user.username
+
+      try {
+        const user = await fastify.knex<Auth>('users')


All these operations should be wrapped in a transaction to avoid race conditions.

jean-michelet · 2024-10-29T20:27:09Z

src/schemas/users.ts

+export const UpdateCredentialsSchema = Type.Object({
+  currentPassword: Type.String(),
+  newPassword: Type.String()


I would force to use a strong password and check that the two values are different.

jean-michelet · 2024-10-29T20:34:32Z

src/routes/api/user/index.ts

+import { UpdateCredentialsSchema } from '../../../schemas/users.js'
+
+const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
+  fastify.put(


I think we should use a rate limiter to mitigate brute force attack, there is an example here:

demo/src/app.ts

Line 74 in 4470023

preHandler: fastify.rateLimit({

jean-michelet · 2024-10-29T20:35:57Z

src/routes/api/user/index.ts

+        }
+
+        reply.status(401)
+        return { message: 'Invalid username or password.' }


This error message is wrong.

This error message is wrong.

I think we should not return to the client if the user or password is wrong, it is not a good security design to prevent potential attackers from getting information about the part of the login information that is incorrect.

Or i have missed something

I think we should not return to the client if the user or password is wrong

For a auth endpoint yes, for a setting endpoint, no. Because we know this is the user/admin that do this operation (unless hacked, this is why we have rate limiter). We must inform why the request for password update failed.

Regarding the line 52, if there is no user session, the error should be detected ahead of time in this hook:

demo/src/routes/api/autohooks.ts

Line 9 in 4470023

if (!request.session.user) {

But you can still return: reply.unauthorized(), it will set status code to 401 automatically.

jean-michelet · 2024-10-29T20:42:29Z

This is my first PR, I'm waiting for your review before committing to writing a test

Congrats for your first PR 👍
Keep in mind that writing tests does help you to produce robust and relevant code and then save code review time.

climba03003 · 2024-10-30T09:02:33Z

src/schemas/users.ts

+export const UpdateCredentialsSchema = Type.Object({
+  currentPassword: Type.String(),
+  newPassword: Type.String()
+})


Suggested change

export const UpdateCredentialsSchema = Type.Object({

currentPassword: Type.String(),

newPassword: Type.String()

})

const Password = Type.String({

// at least 1 upper case, 1 lower case, 1 numeric, 1 special characters with minimal 8 characters.

pattern: "^(?=.*?[A-Z])(?=.*?[a-z])(?=.*?[0-9])(?=.*?[#?!@$%^&*-]).{8,}$",

minLength: 8

})

export const UpdateCredentialsSchema = Type.Object({

currentPassword: Password,

newPassword: Password

})

const Password = Type.String({
// at least 1 upper case, 1 lower case, 1 numeric, 1 special characters with minimal 8 characters.
pattern: "^(?=.?[A-Z])(?=.?[a-z])(?=.?[0-9])(?=.?[#?!@$%^&*-]).{8,}$",
minLength: 8
})

Thanks for the suggestion, also the proposed pattern is draft to give me an idea because that it only checks for the presence of at least one uppercase, one lowercase, one digit and one special char, but it does not allow for any additional characters beyond these, finally the pattern defined only match strings that are exactly 4 characters long

The following i think is more accurate

const atLeastOneUpperCasePattern = '(?=.*?[A-Z])' const atLeastOneLowerCasePattern = '(?=.*?[a-z])' const atLeastOneNumericPattern = '(?=.*?[0-9])' const atLeastOneSpecialCharPattern = '(?=.*?[#?!@$%^&*-])' const passwordPattern = `^${atLeastOneUpperCasePattern}${atLeastOneLowerCasePattern}${atLeastOneNumericPattern}${atLeastOneSpecialCharPattern}.*$`

Also, the minimum length rule is duplicated in both the regex pattern and the JSON Schema definition, i'll keep the rules from the schema.

May be we have specific reason to keep both ?

the pattern defined only match strings that are exactly 4 characters long

No, it matches any string that is 8 characters or longer. See the last part .{8,}.

the minimum length rule is duplicated in both the regex pattern and the JSON Schema definition, i'll keep the rules from the schema.

By looking at the pattern, it is very hard to understand how it works and does it limit the length.
The minLength is used here for ensure the string is at least 8 characters and faster understanding on it.
I don't knows which one take higher priority to check in ajv but more details is better.

"The minLength is used here for ensure the string is at least 8 characters and faster understanding on it."

True, thanks for your answer

Mathieuka · 2024-10-31T22:16:18Z

test/routes/api/user/user.test.ts

+    })
+
+    assert.strictEqual(res.statusCode, 400)
+    assert.deepStrictEqual(JSON.parse(res.payload), { message: 'body/newPassword must match pattern "^(?=.*?[A-Z])(?=.*?[a-z])(?=.*?[0-9])(?=.*?[#?!@$%^&*-]).*$"' })


I should find a way to return a more explicit error message

Password must contain at least one uppercase letter, one lowercase letter, one digit, and one special character (#?!@$%^&*-).

jean-michelet · 2024-10-31T22:36:22Z

Code coverage is not reached, meaning some part of the code was not executed during test.

jean-michelet · 2024-11-05T20:20:29Z

src/routes/api/user/index.ts

+          errorResponseBuilder: function (_, context) {
+            return {
+              statusCode: 429,
+              error: 'Too Many Requests',
+              message: `You have reached the request limit. Please try again in ${Math.floor(context.ttl / 1000)} seconds.`,
+              date: new Date().toISOString(),
+              retryAfter: context.ttl
+            }


You leverage errorResponseBuilder but the message looks like the default one.
You can write a more explicit message regarding password update or you could just pass the needed configuration and let the plugin build the error for you.

No need to override the built in error message, I'll delete it

jean-michelet · 2024-11-05T20:23:58Z

src/schemas/users.ts

+const atLeastOneUpperCasePattern = '(?=.*?[A-Z])'
+const atLeastOneLowerCasePattern = '(?=.*?[a-z])'
+const atLeastOneNumericPattern = '(?=.*?[0-9])'
+const atLeastOneSpecialCharPattern = '(?=.*?[#?!@$%^&*-])'
+
+const passwordPattern = `^${atLeastOneUpperCasePattern}${atLeastOneLowerCasePattern}${atLeastOneNumericPattern}${atLeastOneSpecialCharPattern}.*$`


Not fundamentally opposed, but imo the initial regex is very simple to read, the concatenation is disturbing.

jean-michelet · 2024-11-05T20:24:17Z

src/schemas/users.ts

+const atLeastOneSpecialCharPattern = '(?=.*?[#?!@$%^&*-])'
+
+const passwordPattern = `^${atLeastOneUpperCasePattern}${atLeastOneLowerCasePattern}${atLeastOneNumericPattern}${atLeastOneSpecialCharPattern}.*$`
+const Password = Type.String({


Suggested change

const Password = Type.String({

const PasswordSchema = Type.String({

jean-michelet · 2024-11-05T20:28:10Z

test/routes/api/user/user.test.ts

+
+describe('User API', () => {
+  // Hashed value of `Password123$`
+  const Password123$ = 'ff57faf149a2bcab41bf7ecbbc8ce491.3ce6b34ea3edb3f0a09f811440885bfeda612832c04bfddc9d4b906019d97fa0'


Suggested change

const Password123$ = 'ff57faf149a2bcab41bf7ecbbc8ce491.3ce6b34ea3edb3f0a09f811440885bfeda612832c04bfddc9d4b906019d97fa0'

const hash = 'ff57faf149a2bcab41bf7ecbbc8ce491.3ce6b34ea3edb3f0a09f811440885bfeda612832c04bfddc9d4b906019d97fa0'

Can we just hash it with scryptHash function? If we change the algorithm implementation, this hash will become obsolete, this is not needed.

src/routes/api/user/index.ts

jean-michelet · 2024-11-05T21:05:54Z

test/routes/api/user/user.test.ts

+    const users = ['random-user-0', 'random-user-1', 'random-user-2', 'random-user-3', 'random-user-4', 'random-user-5']
+
+    for (const user of users) {
+      await createUser(app, {
+        username: user,
+        password: Password123$
+      })
+    }


I don't understand why you create all the users here, than recreate them individually in all the tests.

This is an oversight, I will delete it and keep the preparation at the unit test level

jean-michelet · 2024-11-05T21:07:06Z

test/routes/api/user/user.test.ts

+    assert.strictEqual(res.statusCode, 200)
+    assert.deepStrictEqual(JSON.parse(res.payload), { message: 'Password updated successfully' })
+
+    await deleteUser(app, 'random-user-0')


Why do you delete the users at the end of each test?
Just clean all of them in after hook when you are done.

Tests should be run in isolation as much as possible, shared dependencies across preparation hooks are hell.

But if that's a style you prefer, I can use hooks

I told you that because you were creating all the users in the before hook, but I agree with you.

test/routes/api/user/user.test.ts

Mathieuka · 2024-11-06T13:34:41Z

src/schemas/users.ts

+const passwordPattern = '^(?=.*?[A-Z])(?=.*?[a-z])(?=.*?[0-9])(?=.*?[#?!@$%^&*-]).*$'
+
+const PasswordSchema = Type.String({
+  pattern: passwordPattern,


@climba03003 @jean-michelet

Currently I don't think there is a way to customize the error message in case the payload doesn't match the pattern directly via the Type Builder Typebox.

I managed to do this via implementing fastify.setErrorHandler in the password update endpoint configuration, have you ever encountered this problem ?

Is it correct to manage the error message in a custom way at the endpoint ?

Is it correct to manage the error message in a custom way at the endpoint ?

You can using ajv-errors, so the message can be customize per schema.
https://www.npmjs.com/package/ajv-errors

@climba03003

config:
locally Node v20.12.2
ajv-errors 3.0.0

I have installed and attempted to integrate ajv-errors with Fastify's AJV plugin, but it seems there is an issue when we add the errorMessage keyword in the TypeBox or AJV schema.

My research is based on the Fastify documentation

And I think an obsolete implementation example in the documentation, more than 3 years example

// Excerpt from the documentation const fastify = Fastify({ ajv: { customOptions: { jsonPointers: true, // `jsonPointers` is not still part of `AjvOptions` in Fastify is replaced by `jsPropertySyntax` allErrors: true }, plugins: [ require('ajv-errors') ] } })

@jean-michelet If this is the case, I suggest creating an issue to update this part of documentation.

I have create a specific commit as an example of implementation a304da3

// Error thrown by the server FastifyError [Error]: Failed building the validation schema for PUT: /api/user/update-password, due to error strict mode: unknown keyword: "errorMessage"

@jean-michelet

Regarding the custom error message for AJV formatting, I haven't found a solution, and it seems that @climba03003 hasn't either ? as they approved the PR following my message #48 (comment)

As for SonarQube's warning about duplication, that's code I don't want to abstract and SonarQube doesn't know the difference

Is there a way to comment the lines with false positives to avoid warnings?
https://sonarcloud.io/project/security_hotspots?id=fastify_demo&pullRequest=48&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true

We can review on the dashboard and claim it is fixed or safe.

@Eomm
Is it possible to give these rights to the Fastify members that collaborates on this repo?
Besides me, I am aware of @climba03003 and @Fdawgs.

I believe it can identify the Github account and give the correct permission?
Because I have updated the sonarcloud issue when I using the Github OAuth.

jean-michelet · 2024-11-18T21:15:43Z

In RESTful architecture, resource naming conventions recommend using plural nouns for resource names.
Plz, change user for users, with s.

- Removed the errorResponseBuilder function from the user route configuration.

- Simplify the password pattern regex to require at least one uppercase, one lowercase, one numeric, and one special character

- Added rate limiting configuration to limit requests to 3 per minute.

- Rename `user` to `users` in file paths - Update describe block from 'User API' to 'Users API' - Update tags from 'User' to 'Users' in API endpoints definition

- Remove rate limiting configuration from the home route to simplify the code and improve performance.

- Updated loop condition from `< 3` to `< 4` to match new rate limit of 4 requests

github-advanced-security · 2024-12-08T18:03:50Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

jean-michelet · 2024-12-09T11:09:56Z

src/routes/api/users/index.ts

+          400: Type.Object({
+            message: Type.String()
+          }),
          401: Type.Object({
            message: Type.String()
+          }),
+          429: Type.Object({
+            message: Type.String()


Applying schema on response helps to avoid to return sensitive information and limit the output. We should not handle non endpoint-specific status code here.

sonarqubecloud · 2024-12-25T15:20:26Z

Quality Gate failed

Failed conditions
12.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Mathieuka force-pushed the add-auth-update-endpoint branch 2 times, most recently from 5096495 to 659692e Compare October 28, 2024 21:58

climba03003 requested changes Oct 29, 2024

View reviewed changes

src/routes/api/auth/index.ts Outdated Show resolved Hide resolved

src/routes/api/auth/index.ts Outdated Show resolved Hide resolved

jean-michelet reviewed Oct 29, 2024

View reviewed changes

src/routes/api/auth/index.ts Outdated Show resolved Hide resolved

src/schemas/auth.ts Outdated Show resolved Hide resolved

jean-michelet mentioned this pull request Oct 29, 2024

Registration with email verification process #49

Open

Mathieuka force-pushed the add-auth-update-endpoint branch 3 times, most recently from f4045d0 to b87b39e Compare October 29, 2024 20:09

Mathieuka requested review from jean-michelet and climba03003 October 29, 2024 20:23

Mathieuka marked this pull request as ready for review October 29, 2024 20:23

Mathieuka marked this pull request as draft October 29, 2024 20:24

jean-michelet reviewed Oct 29, 2024

View reviewed changes

climba03003 reviewed Oct 30, 2024

View reviewed changes

Mathieuka force-pushed the add-auth-update-endpoint branch from faa87b6 to b396657 Compare October 31, 2024 22:14

Mathieuka commented Oct 31, 2024

View reviewed changes

Mathieuka force-pushed the add-auth-update-endpoint branch from b396657 to cf565be Compare October 31, 2024 22:17

Mathieuka force-pushed the add-auth-update-endpoint branch 3 times, most recently from 68c3bb2 to bac1131 Compare November 3, 2024 16:23

jean-michelet reviewed Nov 5, 2024

View reviewed changes

Mathieuka commented Nov 6, 2024

View reviewed changes

Mathieuka force-pushed the add-auth-update-endpoint branch from a304da3 to 821c631 Compare November 9, 2024 15:32

climba03003 approved these changes Nov 11, 2024

View reviewed changes

Mathieuka marked this pull request as ready for review November 16, 2024 18:21

Mathieuka force-pushed the add-auth-update-endpoint branch from 914466a to 9326a67 Compare November 20, 2024 19:58

Mathieuka added 20 commits December 8, 2024 16:21

test: isolate test by seeding users

423bfa9

test: add unit test to verify rate limiting for password update requests

c684ada

refactor(test): refactor user creation loop

e423674

test: improve variable naming

3c7d52c

fix(api): refactor user.test.ts

5708cbe

refactor: rename helper function for updating password injection

51c6006

refactor: remove errorResponseBuilder from user route

9844207

- Removed the errorResponseBuilder function from the user route configuration.

refactor: simplify password pattern regex

aeb5414

- Simplify the password pattern regex to require at least one uppercase, one lowercase, one numeric, and one special character

refactor: rename Password to PasswordSchema

6e0c778

test: use scryptHash

e79857b

test: create and delete user at the test level

39dbe32

test: add abstraction to rate limiting testing

91bcb93

chore: ajv-errors integration code example

f9a2f49

feat: Remove ajv-errors and related implementation

170a24c

feat(routes): add rate limiting to home route

8ce3b83

- Added rate limiting configuration to limit requests to 3 per minute.

refactor(api): remove redundant return statement

d81991f

refactor(api): rename user to users

6faa9bb

- Rename `user` to `users` in file paths - Update describe block from 'User API' to 'Users API' - Update tags from 'User' to 'Users' in API endpoints definition

refactor(routes): remove rate limiting configuration

789d203

- Remove rate limiting configuration from the home route to simplify the code and improve performance.

fix(test): update rate limit test to match new rate limit

151ff29

- Updated loop condition from `< 3` to `< 4` to match new rate limit of 4 requests

test: Add error handling test

31c74db

Mathieuka force-pushed the add-auth-update-endpoint branch from 6059bae to 31c74db Compare December 8, 2024 18:02

Mathieuka added 2 commits December 9, 2024 11:26

feat: Add additional error responses to user API routes

e9e5e3a

test: Remove unnecessary await in user password update tests

15082be

jean-michelet reviewed Dec 9, 2024

View reviewed changes

Mathieuka added 3 commits December 10, 2024 16:40

feat: Remove HTTP sensitive status codes from user API schema

1e52e4b

test: isolate rate limit test

be7877b

test: refactor rate limit

f6c988f

jean-michelet merged commit 72956f3 into fastify:main Dec 27, 2024
4 of 5 checks passed

	const Password = Type.String({
	const PasswordSchema = Type.String({

	const Password123$ = 'ff57faf149a2bcab41bf7ecbbc8ce491.3ce6b34ea3edb3f0a09f811440885bfeda612832c04bfddc9d4b906019d97fa0'
	const hash = 'ff57faf149a2bcab41bf7ecbbc8ce491.3ce6b34ea3edb3f0a09f811440885bfeda612832c04bfddc9d4b906019d97fa0'

Add auth update endpoint #48

Add auth update endpoint #48

Conversation

Mathieuka commented Oct 28, 2024 • edited Loading

Description

Checklist

jean-michelet left a comment • edited Loading

Choose a reason for hiding this comment

Mathieuka commented Oct 29, 2024

jean-michelet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mathieuka Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

jean-michelet Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

jean-michelet commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Mathieuka Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jean-michelet commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mathieuka Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mathieuka Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mathieuka Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

Mathieuka Nov 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jean-michelet commented Nov 18, 2024 • edited Loading

github-advanced-security bot commented Dec 8, 2024

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 25, 2024

Quality Gate failed

Mathieuka commented Oct 28, 2024 •

edited

Loading

jean-michelet left a comment •

edited

Loading

Mathieuka Oct 30, 2024 •

edited

Loading

jean-michelet Oct 30, 2024 •

edited

Loading

jean-michelet commented Oct 29, 2024 •

edited

Loading

Mathieuka Oct 30, 2024 •

edited

Loading

Mathieuka Nov 6, 2024 •

edited

Loading

Mathieuka Nov 6, 2024 •

edited

Loading

Mathieuka Nov 9, 2024 •

edited

Loading

Mathieuka Nov 16, 2024 •

edited

Loading

jean-michelet commented Nov 18, 2024 •

edited

Loading