[Iir0R]

Brief description

Added Session to handle refresh token rotation.

Change list

Session

• Added new collection "Session"
• Session is used to track users login / refresh / logout separately between devices
• Sessions belonging to same session share session_id
• Previously a global tokenVersion was used to invalidate refresh tokens, which would force signing in on all devices

Sign in

• A new session is created on sing in
• Refresh tokens token_id matches Session doc _id

Refresh

• On successful refresh, new Session doc is created, previous token is revoked by updating Sessions revokedAt
• Refresh doesn't extend Session past original expiration
• Inactive tokens (token not used for X days) also require new sign in
• Attempting refresh with used token revokes whole session

Logout

• Logout revokes Session in DB
• Works for mobile
• Fixed time format for cookies

Other

• Added JWT_REFRESH_EXPIRES and JWT_INACTIVITY to envVars
• Expiration times are primarily from env
• Updated other sign ins to also use Session
• Added Session Factory + Builder for tests
• Updated tests, added new one for logout

[CapoMK25]

These test failures seem to relate to the vuln fixes on the Dockerfile most likely, and will show up on every single subsequent PR, blocking each one, until addressed further @Iir0R @tickBit

Disclaimer: I have no further knowledge on this as I just read about the vuln fixes on discord.

[tickBit]

I asked AI. The answer seems to make sense:
""
The CI logs show that required variables are missing/undefined in the environment, including:

PORT
JWT_REFRESH_EXPIRES
JWT_REFRESH_INACTIVITY

This causes the method to log errors and throw, which makes the tests fail. The failing assertions are the ones that expect either:

a thrown ServiceError when a variable is missing, and
no throw when all variables are present

But in CI, those required variables are effectively absent, so the “all vars are found” case still throws, which is why the job exits with code 1.
""

I'll try to add those missing env variables, though I'm not sure about PORT variable.

[tickBit]

I have added now the missing environment variables to physical server onto dev instance.

Now the build might succeed.

@Iir0R

[tickBit]

I added first those environment variables onto prod instance only, now they're on main and dev, too.

[Iir0R]

Still fails, PORT seems to be the problem. Does server not have PORT in env?

[tickBit]

I'll check, PORT might be missing

[tickBit]

The teacher API was missing new environment variables, my bad. Now they're added there, too.
PORT is also present on every instance.

[tickBit]

According to AI: "

GitHub Actions tests do not use the Docker Compose environment variables from the physical server. The new refresh variables must also be added to the Jest test environment in the file:

src/__tests__/test_utils/env.ts

For example, in the context of JWT configurations:

process.env.JWT_EXPIRES = ‘15m’;
process.env.JWT_REFRESH_EXPIRES = ‘<test value>’;
process.env.JWT_REFRESH_INACTIVITY = ‘<test value>’;

Use the same valid format for the refresh values as in the application’s environment settings.

The PORT error is intentional in this test because the test sets the envVars.PORT value to undefined. The assertion likely fails because the validation detects uninitialized JWT_REFRESH_EXPIRES and JWT_REFRESH_INACTIVITY variables in addition to PORT."

[tickBit]

Into the PR should be added to src/__tests__/test_utils/env.ts:

process.env.JWT_REFRESH_EXPIRES = '<test_value>';
process.env.JWT_REFRESH_INACTIVITY = '<test_value>';

[codecov-alt]

Codecov Report

❌ Patch coverage is 92.23301% with 8 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/auth/auth.service.ts	92.85%	5 Missing ⚠️
src/auth/box/BoxAuthService.ts	92.30%	1 Missing ⚠️
src/box/accountClaimer/accountClaimer.service.ts	88.88%	1 Missing ⚠️
src/box/auth/BoxAuthHandler.ts	90.90%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/common/service/envHandler/envVars.ts	100.00% <ø> (ø)
src/auth/box/BoxAuthService.ts	95.00% <92.30%> (-1.08%)	⬇️
src/box/accountClaimer/accountClaimer.service.ts	93.33% <88.88%> (-1.41%)	⬇️
src/box/auth/BoxAuthHandler.ts	95.12% <90.90%> (-1.85%)	⬇️
src/auth/auth.service.ts	95.27% <92.85%> (-1.58%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[tickBit]

Nice work! It passed all the tests 👍
I'll wait until tomorrow before I review this.

[tickBit]

Overall excellent work 👍 !

I asked also AI about this PR since it concerns security.

Two small changes to db schema (those are AI's suggestions):

1. profile_id should use one consistent type across the schema and payloads. The schema currently uses ObjectId, while several payloads treat it as string, which makes the type contract harder to reason about and can lead to subtle runtime bugs.

2. Add a compound index on { session_id: 1, revokedAt: 1 } (and likely profile_id as well if it’s used in those lookups). The rotation/logout flow does repeated lookups and updates against the same session family, and without this index the collection will become more expensive as it grows.

Those changes keep the overall design intact while reducing type risk and query overhead.

[tickBit]

The compound indexes are correct now.

I would like to see still the following changes:
// schema

@Prop({ type: MongooseSchema.Types.ObjectId, ref: ModelName.PROFILE, required: true })
profile_id: Types.ObjectId;

// dto

export class SessionPayload {
  _id: ObjectId;
  session_id: string;
  profile_id: ObjectId;
  refreshToken: string;
  refreshTokenExpires: number;
}

[tickBit]

Hopefully my previous comment gives the idea, I'm after : )

[Iir0R]

Is profile_id supposed to be ObjectId or string? Previous change request to string, new one ObjectId?

[tickBit]

Is profile_id supposed to be ObjectId or string? Previous change request to string, new one ObjectId?

I've been confused about those things myself time to time.

profile_id should be an ObjectId in the MongoDB schema because it references Profile, but a string in JWT/API payloads. The previous request for string referred to its serialized representation, not the database field type. Please use Schema.Types.ObjectId with ref: ModelName.PROFILE.

I'm doing many things at once, so sorry for confusion : )

[tickBit]

I think this is good to go. Good work 👍

Fingers crossed, that it gets into the physical server next : )