This repository was archived by the owner on May 12, 2026. It is now read-only.
feat(auth): implement user registration with session creation, Redis caching, and refresh token - #11
Merged
Conversation
…, and T&C validation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
📝 WalkthroughWalkthroughThis PR enforces terms acceptance at registration (DB column, DTO, controller change), refactors registration to create user/session/auth-metadata inside a DB transaction and set a refresh cookie with Redis mapping, updates related tests and system messages, and additionally introduces mailer config, email consumer/queue changes, a mailer smoke-test script, and a reusable CI pipeline replacing legacy workflows. ChangesUser Terms Acceptance Registration
Mailer, Email Queue & CI Pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/modules/auth/tests/auth.service.spec.ts (1)
92-114: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider verifying the refresh token cookie is set.
The test mocks
response.cookie()but doesn't assert that it was called with the correct parameters. Consider adding an assertion to verify the refresh token cookie is set correctly during registration.🧪 Suggested assertion
After line 113, add:
expect(responseMock.cookie).toHaveBeenCalledWith( 'refresh_token', expect.any(String), expect.objectContaining({ httpOnly: true, sameSite: 'strict', maxAge: 7 * 24 * 60 * 60 * 1000, }) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/auth/tests/auth.service.spec.ts` around lines 92 - 114, Add an assertion to the test that verifies the refresh token cookie is set when calling service.createNewUser by asserting responseMock.cookie was called with the 'refresh_token' name, a string value, and cookie options containing httpOnly: true, sameSite: 'strict', and maxAge of 7 * 24 * 60 * 60 * 1000; locate this in the test around the it('creates a user when none exists with that email') block and add the expect for responseMock.cookie after the existing result assertions so the registration flow in createNewUser is confirmed to set the cookie.src/modules/auth/auth.service.ts (1)
133-159:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMajor inconsistency: Login doesn't create sessions or refresh tokens.
The registration flow (lines 39-131) creates a
UserSession, writes to Redis, and sets a refresh token cookie, but theloginUsermethod (lines 133-159) does none of these. This creates an architectural inconsistency where:
- Users who register have sessions and refresh tokens.
- Users who log in only get access tokens without sessions.
- The JWT structure differs (register includes
session_idon line 113, login doesn't on line 144).For a consistent authentication model,
loginUsershould also create or refresh sessions and set the refresh token cookie.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/auth/auth.service.ts` around lines 133 - 159, loginUser currently only returns an access token and skips session creation and refresh-token handling; update the loginUser function to mirror the register flow by creating a UserSession (use the same UserSession creation logic/class), persist the session to Redis (same key/schema your register flow uses), generate a refresh token and set it as an HTTP-only cookie on the response, and include session_id in the payload passed to this.jwtService.sign (matching the register JWT structure); ensure you reuse the same helpers/utilities used in registration for session creation, redis write and cookie-setting so both flows remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/auth/auth.service.ts`:
- Around line 88-92: The AuthMetadata creation in auth.service.ts incorrectly
sets last_login_at during registration; update this to reflect registration time
or defer last_login_at to login flows—specifically change the property used in
queryRunner.manager.create(AuthMetadata, { user_id: saved.id, last_login_at: new
Date() }) to a registration timestamp name (e.g. registered_at or created_at)
that matches the AuthMetadata entity, or remove last_login_at here and only set
AuthMetadata.last_login_at inside the actual login routine; ensure corresponding
entity property (AuthMetadata) is renamed/added and migrations/uses updated to
keep names consistent.
- Around line 95-98: The catch block after queryRunner.rollbackTransaction
currently throws a generic CustomHttpException with
SYS_MSG.SESSION_CREATION_FAILED; change it to inspect the caught error (error)
and map it to more specific categories (e.g., DB_ERROR, REDIS_ERROR,
VALIDATION_ERROR) based on error.name, instanceof checks, or error codes, log
the full error via this.logger.error, and throw a CustomHttpException that
includes a distinct error code or category and a concise client-safe message
(while preserving internal details in logs); update the throw site (the catch in
register or the function where queryRunner.rollbackTransaction is used) to use
the new mapped category and include the original error.message or an errorId
token for support lookup so callers can differentiate database vs redis vs
validation failures.
- Line 71: In the user creation flow in AuthService (e.g., the method handling
registration such as createUser/registerUser), remove the hardcoded
terms_accepted: true and use the value from the incoming DTO (the
register/create DTO variable, e.g., createUserDto or registerDto) instead;
ensure the code references dto.terms_accepted when building the user payload so
the DTO validation is respected and no hardcoded value overrides it.
- Line 86: Remove the dead Redis session caching or implement refresh flow:
either delete the call to redisService.set(redisKey, userSession.id, 900) and
remove any unused user_sessions TTL/Redis assumptions plus associated DB schema
changes, or implement a /refresh endpoint that reads the refresh token cookie,
validates it against the user_sessions table (matching token and expiry for the
stored userSession record), issues a new access token, and returns it (and
optionally rotates the refresh token in the DB). Locate usage around
auth.service.ts (redisService.set and userSession creation) and the
user_sessions table handling to add the endpoint logic and validation or to
remove the Redis set and related unused schema. Ensure the refresh endpoint
checks expiry, user id, and token integrity before issuing the new access token.
In `@src/modules/auth/dto/create-user.dto.ts`:
- Line 54: The property declaration for terms_accepted in the CreateUserDto DTO
is missing a trailing semicolon; open the CreateUserDto (create-user.dto.ts),
locate the terms_accepted: boolean property and add a semicolon at the end to
match the other property declarations and TypeScript style.
In `@src/modules/auth/tests/auth.service.spec.ts`:
- Line 15: Remove the unused imports `AnyAaaaRecord` and `AnyCaaRecord` from the
import statement that pulls from 'node:dns' in the auth service test; update the
import line in src/modules/auth/tests/auth.service.spec.ts so it no longer
references these two symbols (`AnyAaaaRecord`, `AnyCaaRecord`) and only imports
what the test actually uses.
In `@src/modules/user/entities/user.entity.ts`:
- Line 41: The property declaration terms_accepted in the User entity is missing
a terminating semicolon; update the property declaration for terms_accepted in
src/modules/user/entities/user.entity.ts (the User entity class) to include a
semicolon at the end so it matches the other property declarations and
TypeScript syntax conventions.
---
Outside diff comments:
In `@src/modules/auth/auth.service.ts`:
- Around line 133-159: loginUser currently only returns an access token and
skips session creation and refresh-token handling; update the loginUser function
to mirror the register flow by creating a UserSession (use the same UserSession
creation logic/class), persist the session to Redis (same key/schema your
register flow uses), generate a refresh token and set it as an HTTP-only cookie
on the response, and include session_id in the payload passed to
this.jwtService.sign (matching the register JWT structure); ensure you reuse the
same helpers/utilities used in registration for session creation, redis write
and cookie-setting so both flows remain consistent.
In `@src/modules/auth/tests/auth.service.spec.ts`:
- Around line 92-114: Add an assertion to the test that verifies the refresh
token cookie is set when calling service.createNewUser by asserting
responseMock.cookie was called with the 'refresh_token' name, a string value,
and cookie options containing httpOnly: true, sameSite: 'strict', and maxAge of
7 * 24 * 60 * 60 * 1000; locate this in the test around the it('creates a user
when none exists with that email') block and add the expect for
responseMock.cookie after the existing result assertions so the registration
flow in createNewUser is confirmed to set the cookie.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d79a769f-6bfb-4508-b397-c21b91ff51fa
📒 Files selected for processing (7)
src/database/migrations/1778335054510-migration.tssrc/modules/auth/auth.controller.tssrc/modules/auth/auth.service.tssrc/modules/auth/dto/create-user.dto.tssrc/modules/auth/tests/auth.service.spec.tssrc/modules/user/entities/user.entity.tssrc/shared/constants/SystemMessages.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 7 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 7 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/auth/auth.service.ts`:
- Around line 77-80: The code currently assigns a plaintext refresh token into
the UserSession entity (see UserSession creation and generateRefreshToken());
instead, generate the secure random token as now but compute a one-way hash
(e.g., bcrypt or HMAC-SHA256 with a server secret) and store only that hash in
the UserSession.refresh_token column, persist the plaintext token only in the
outgoing cookie/response, and implement a verify path in your refresh/revoke
flows that hashes the presented token and compares it to the stored hash (add a
helper like verifyRefreshToken(token, hashed) and update the refresh/revoke
handlers to use it).
- Around line 104-106: The QueryFailedError handler in AuthService (where it
checks err.name === 'QueryFailedError') should detect Postgres unique-violation
errors ((err as any).code === '23505') and map that case to the same
USER_ACCOUNT_EXIST/BAD_REQUEST response used by the earlier findOne pre-check
(which is raceable outside the transaction that begins around the register
transaction block). Update the error branch to inspect (err as any).code ===
'23505' and return the appropriate USER_ACCOUNT_EXIST error and BAD_REQUEST
status instead of the generic "Database error occurred during registration"
message, otherwise keep existing logging behavior (including logging the full
err) for other QueryFailedError cases.
In `@src/modules/auth/tests/auth.service.spec.ts`:
- Around line 38-52: The current transaction mock in dataSourceMock
(createQueryRunner -> manager.save) always returns the same user-shaped object;
change manager.save to use sequential mockResolvedValueOnce calls to return
distinct entities in order (first the saved User, then the saved UserSession
with id and refresh_token, then the saved AuthMetadata) so the test exercises
session wiring; update the tests that call createQueryRunner/save (including the
other block around the later tests) to assert response.cookie(...) and the
decoded JWT payload include the actual saved UserSession.id and
UserSession.refresh_token values returned by those mockResolvedValueOnce calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c56744e2-7381-469b-a73a-196bc1073ced
📒 Files selected for processing (4)
src/modules/auth/auth.service.tssrc/modules/auth/dto/create-user.dto.tssrc/modules/auth/tests/auth.service.spec.tssrc/modules/user/entities/user.entity.ts
Contributor
|
Add a link to your clickup ticket in your related issue part of the PR details. |
… terms_accepted from DTO
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (6)
src/modules/auth/auth.service.ts (3)
105-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle unique email constraint violations in error handling.
The
findOnepre-check (line 44) runs outside the transaction and is raceable—concurrent registrations can both pass the check, then the second insert hits the unique constraint onQueryFailedErrorbut returns a generic "Database error occurred during registration" instead of mapping the constraint violation toUSER_ACCOUNT_EXISTwithBAD_REQUEST.Check for Postgres unique violations (
(err as any).code === '23505') and return the same error the pre-check would have thrown.🔧 Proposed fix
if (err.name === 'QueryFailedError') { + const pgError = err as any; + if (pgError.code === '23505') { + // Unique constraint violation (concurrent registration) + throw new CustomHttpException(SYS_MSG.USER_ACCOUNT_EXIST, HttpStatus.BAD_REQUEST); + } errorMessage = 'Database error occurred during registration'; this.logger.error('DB_ERROR during registration', err); } else if (err.message?.includes('Redis') || err.message?.includes('redis')) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/auth/auth.service.ts` around lines 105 - 107, The DB error handler in AuthService during registration should detect Postgres unique-constraint violations and map them to the existing USER_ACCOUNT_EXIST / BAD_REQUEST response rather than a generic message; update the block that currently checks err.name === 'QueryFailedError' (and logs via this.logger.error('DB_ERROR during registration', err)) to also check (err as any).code === '23505' and set the same errorMessage/status you use in the pre-check (USER_ACCOUNT_EXIST and HttpStatus.BAD_REQUEST) before logging and returning, otherwise keep the generic database error handling.
86-86:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRemove unused Redis session caching or implement the refresh token endpoint.
The Redis key
sess:{userId}:{sessionId}is written but never retrieved anywhere in the codebase—it's dead code. Additionally, the refresh token is generated and stored with a 7-day expiry, but there's no endpoint or logic to exchange the refresh token for a new access token.Either:
- Remove the orphaned
redisService.setcall if refresh tokens aren't needed yet, or- Implement a
/auth/refreshendpoint that validates therefresh_tokencookie againstuser_sessions, checks expiry/revocation, retrieves the session from Redis (or falls back to Postgres), and issues a new access token.💡 Guidance for implementing refresh endpoint
If you choose option 2, the endpoint should:
async refreshAccessToken(cookieToken: string) { // 1. Hash the incoming cookie token // 2. Find matching UserSession in DB where refresh_token hash matches & !is_revoked & expires_at > now // 3. Optionally check Redis cache for session validity // 4. Generate new access token with { id, sub, session_id, email } // 5. Return new access token }Then the Redis cache at line 86 would be checked during refresh to quickly invalidate sessions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/auth/auth.service.ts` at line 86, The Redis write at redisService.set(redisKey, userSession.id, 900) is dead unless you add refresh logic—either remove that call and any related Redis session caching, or implement a /auth/refresh flow: add an endpoint (e.g., POST /auth/refresh) that accepts the refresh_token cookie, hash it and query UserSession (user_sessions) for a non-revoked, unexpired refresh_token, optionally check Redis for the session id cached by redisService.set, then generate and return a new access token (payload: id, sub, session_id, email); if you implement refresh, keep the redisService.set call and ensure refreshAccessToken validates Redis first and falls back to Postgres.
77-83:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid storing refresh tokens in plaintext.
refresh_tokenis stored directly in the database without hashing. If theuser_sessionstable is compromised, every active session secret is immediately reusable by an attacker.Hash the token (e.g., with
bcrypt.hash()or a keyed HMAC) before storing it inUserSession.refresh_token, send only the plaintext token in the cookie, then compare hashes during refresh/revoke flows.🔒 Proposed fix to hash refresh tokens
userSession = queryRunner.manager.create(UserSession, { user_id: saved.id, - refresh_token: this.generateRefreshToken(), + refresh_token: await bcrypt.hash(this.generateRefreshToken(), 10), expires_at: refreshTokenExpiry, is_revoked: false, });Important: You'll also need to store the plaintext token temporarily to set the cookie:
+ const plaintextToken = this.generateRefreshToken(); + const hashedToken = await bcrypt.hash(plaintextToken, 10); + userSession = queryRunner.manager.create(UserSession, { user_id: saved.id, - refresh_token: this.generateRefreshToken(), + refresh_token: hashedToken, expires_at: refreshTokenExpiry, is_revoked: false, }); await queryRunner.manager.save(userSession);Then update the cookie setting:
- response.cookie('refresh_token', userSession.refresh_token, { + response.cookie('refresh_token', plaintextToken, { httpOnly: true,When you implement the refresh endpoint, verify with:
const isValid = await bcrypt.compare(cookieToken, storedSession.refresh_token);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/auth/auth.service.ts` around lines 77 - 83, The refresh token is being stored in plaintext (UserSession.refresh_token) — change the flow in the method that calls generateRefreshToken() so you generate the plaintext token, set the cookie with that plaintext, then hash the token (e.g., bcrypt.hash(...) or a keyed HMAC) and save only the hashed value to UserSession.refresh_token before calling queryRunner.manager.save(userSession); also update the refresh and revoke flows to validate tokens by comparing the cookie plaintext to the stored hash using bcrypt.compare (or HMAC verify), ensure you handle hashing errors and choose appropriate salt/rounds.src/modules/auth/tests/auth.service.spec.ts (1)
39-53: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffMake the transaction mock return entity-specific saves.
queryRunner.manager.savealways resolves to the same user-shaped object, so when the service savesUserSessionandAuthMetadata, they incorrectly receive user properties. The test at lines 114-122 can't verify the actualrefresh_tokenvalue orsession_idbecause the mock doesn't return proper session data.Use sequential
mockResolvedValueOnce(...)to return distinct entities in order (User, then UserSession withidandrefresh_token, then AuthMetadata), and update the cookie assertion to verify the actual token value instead ofexpect.any(String).♻️ Proposed fix
const dataSourceMock = { createQueryRunner: jest.fn().mockReturnValue({ connect: jest.fn(), startTransaction: jest.fn(), commitTransaction: jest.fn(), rollbackTransaction: jest.fn(), release: jest.fn(), manager: { create: jest.fn().mockImplementation((entity, data) => data), - save: jest - .fn() - .mockResolvedValue({ id: 'user-1', email: 'jane@example.com', full_name: 'Jane Doe', avatar_url: null }), + save: jest.fn() + .mockResolvedValueOnce({ id: 'user-1', email: 'jane@example.com', full_name: 'Jane Doe', avatar_url: null }) + .mockResolvedValueOnce({ id: 'session-1', user_id: 'user-1', refresh_token: 'mock-refresh-token', expires_at: new Date(), is_revoked: false }) + .mockResolvedValueOnce({ user_id: 'user-1', last_login_at: null }), }, }), };Then update the cookie assertion to verify the actual token:
expect(responseMock.cookie).toHaveBeenCalledWith( 'refresh_token', - expect.any(String), + 'mock-refresh-token', expect.objectContaining({ httpOnly: true, sameSite: 'strict', maxAge: 7 * 24 * 60 * 60 * 1000, }) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/auth/tests/auth.service.spec.ts` around lines 39 - 53, The queryRunner.manager.save mock in dataSourceMock currently always resolves to the same user-shaped object, so update dataSourceMock.createQueryRunner().manager.save to use sequential mockResolvedValueOnce(...) calls: first return the User object, second return a UserSession object with distinct id and refresh_token values, and third return the AuthMetadata object; keep other methods on the mock unchanged. Then update the test's cookie assertion to assert the actual refresh_token value returned by the second mockResolvedValueOnce (the session's refresh_token) instead of using expect.any(String), and assert the session_id matches the id from that same mocked UserSession..github/workflows/_build.yml (1)
42-43: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse
npm cifor reproducible installs (same as_test.yml/_lint.yml).Builds in particular benefit from a deterministic
node_modulesderived strictly frompackage-lock.json. Withcache: npmalready configured above,npm ciis the idiomatic choice.♻️ Proposed change
- - name: Install dependencies - run: npm install + - name: Install dependencies + run: npm ci🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/_build.yml around lines 42 - 43, The "Install dependencies" step currently runs `npm install`; change it to use `npm ci` so the workflow step (named "Install dependencies") performs a reproducible install from package-lock.json (matching the other workflows like `_test.yml`/`_lint.yml`) and leverages the configured `cache: npm` for deterministic builds..github/workflows/_lint.yml (1)
23-24: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSame
npm install→npm ciconsideration as_test.yml.Apply the same fix here for reproducibility and lockfile-cache integrity. See the comment on
_test.ymlfor the rationale and diff.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/_lint.yml around lines 23 - 24, Replace the "Install dependencies" step's usage of npm install with npm ci to ensure deterministic installs and lockfile integrity; locate the workflow step named "Install dependencies" that currently runs "npm install --include=dev" and change it to run "npm ci --include=dev" so the job uses the lockfile and produces reproducible builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 17-19: Remove the unused environment key DB_DATABASE from
.env.example and keep only DB_NAME (which is the actual env variable referenced
by the codebase), so delete the DB_DATABASE line to avoid confusion between
DB_DATABASE and DB_NAME.
In @.github/workflows/_build.yml:
- Around line 3-22: The workflow's workflow_dispatch trigger is missing the
environment input so env vars TARBALL, ECOSYSTEM_CONFIG, and ARTIFACT_NAME that
rely on inputs.environment resolve to empty; either add a matching
inputs.environment declaration under workflow_dispatch (mirror the workflow_call
input: description, required: true, type: string) so manual dispatch supplies
inputs.environment, or remove the workflow_dispatch trigger entirely so the
reusable workflow only runs via workflow_call and the env expressions remain
valid; update either the workflow_dispatch block or delete it and ensure
inputs.environment is referenced only where provided.
In @.github/workflows/_deploy.yml:
- Around line 26-29: The workflow defines REMOTE_TMP, DEPLOY_DIR and PM2_ENV in
the job env and then re-defines identical local aliases inside the remote
heredoc; remove the duplicated re-assignments by using the exported workflow env
values directly inside the heredoc (referencing ${ env.DEPLOY_DIR }, ${
env.PM2_ENV } and ${ env.REMOTE_TMP } there) and eliminate the local variable
re-definitions (the redundant lines that set local DEPLOY_DIR/PM2_ENV/REMOTE_TMP
in the heredoc), or alternatively keep only the remote-side definitions and
delete the job-level env entries—pick one approach and remove the other to avoid
drift (symbols: REMOTE_TMP, DEPLOY_DIR, PM2_ENV, the heredoc block).
- Around line 55-74: Replace the insecure "-o StrictHostKeyChecking=no" usage in
the sshpass/scp/ssh commands by adding a one-time "Trust deploy host" step that
takes a HOST_SSH_KEY (e.g. secrets.HOST_SSH_KEY or an ssh-keyscan output) and
appends it to ~/.ssh/known_hosts with strict permissions, then remove the
StrictHostKeyChecking option from all sshpass/scp/ssh invocations; reference the
ssh/scp commands and the TARBALL/REMOTE_TMP deploy step to locate where to drop
the option and add the new setup step that writes HOST_KEY into known_hosts and
sets 700/600 permissions.
- Around line 43-51: Replace the sshpass-based steps ("Install sshpass" and
"Prepare SSH helper") with an SSH key-based deploy flow: read the private key
from secrets.DEPLOY_SSH_KEY (instead of secrets.PASSWORD/SSHPASS), write it to
~/.ssh/id_ed25519 with strict file permissions, load it into ssh-agent (or use
the official actions/ssh-agent) and add known_hosts or use ssh-keyscan to trust
the target host; remove any usage of sshpass and SSHPASS env var. Ensure the
workflow steps reference the same job names ("Install sshpass", "Prepare SSH
helper") if you keep names, update them to reflect the new key setup, and make
sure the key file permissions and agent-add are performed before any
ssh/scp/rsync steps.
In @.github/workflows/_security.yml:
- Around line 21-25: Remove the unnecessary "Install dependencies" step and have
the "Run npm audit" step run immediately after checkout; specifically delete the
step named "Install dependencies" (the npm install --include=dev command) and
keep/ensure the step named "Run npm audit" executes directly after checkout so
npm audit reads from package-lock.json without installing dev dependencies.
- Around line 36-39: The gitleaks GitHub Action step "Run Gitleaks" uses
gitleaks/gitleaks-action@v2 which requires a GITLEAKS_LICENSE env var for
org-owned repos; update the workflow to either (A) pass the license: add
GITLEAKS_LICENSE to the env for the "Run Gitleaks" step and declare it as a
secret in the workflow_call inputs so it can be forwarded from pipeline.yml, or
(B) replace the action with a license-free gitleaks CLI invocation in the job
instead; reference the "Run Gitleaks" step, the gitleaks/gitleaks-action@v2
usage, env GITHUB_TOKEN and the new GITLEAKS_LICENSE secret when making the
change.
In @.github/workflows/_test.yml:
- Around line 21-22: Replace the workflow step currently named "Install
dependencies" that runs `npm install --include=dev` with `npm ci`; specifically,
update the run command to `npm ci` so CI uses the lockfile for reproducible
installs, fails on drift, and pairs with the `cache: npm` strategy (remove the
`--include=dev` flag as dev deps are installed by default in CI).
In @.github/workflows/pipeline.yml:
- Around line 60-66: This PR adds a workflow file `_test.yml` but leaves the
`test-pr` and `test` jobs commented out; either enable those jobs or remove
`_test.yml`. Fix by uncommenting the `test-pr` (and `test` if intended) job
definitions in the pipeline and add the corresponding `needs: test` (or add
`test` into existing `needs:` lists where `lint-pr` is referenced) so the new
test jobs are actually run, or alternatively delete `_test.yml` if you don't
want to enable CI yet; look for the job names `test-pr`, `test`, and the
referenced workflow file `_test.yml` to make the change.
- Around line 38-40: The pipeline-wide concurrency block uses group:
pipeline-${{ github.ref }} with cancel-in-progress: true which will cancel
in-flight deploys (including the deploy-prod job); update the workflow so
production deploys are not cancelled by either (a) changing the global
concurrency to allow cancels only for non-prod jobs or (b) adding a per-job
concurrency override on the deploy-prod job (symbol: deploy-prod) to set
concurrency.group to a unique name (e.g., deploy-prod) and cancel-in-progress:
false; locate the global concurrency entry (group: pipeline-${{ github.ref }})
and/or the deploy-prod job and apply the override accordingly.
In `@config/mailer.config.ts`:
- Line 17: The mailer config currently sets port using
Number(process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? 587) without
validation; change this to explicitly parse the env value (prefer
RESEND_SMTP_PORT then SMTP_PORT), ensure it is an integer and within 1–65535,
and throw a clear startup error if invalid so the app fails fast; update the
"port" field in the mailer config (the Number(...) expression) to perform
parsing and validation and raise an Error with the offending env value when out
of range or non-numeric.
In `@scripts/test-resend.ts`:
- Around line 6-15: The script currently only checks pass; add fail-fast
validation for host and port after computing host, port, user, pass, from:
verify host is a non-empty string (host) and port is a valid integer within
1–65535 (port), and if either check fails call console.error with a clear
message and process.exit(1) so misconfiguration fails before creating the SMTP
transport; keep messages specific (e.g., "No SMTP host found" and "Invalid SMTP
port: <value>") to aid debugging.
- Around line 1-2: The project is missing package.json entries for the modules
imported in scripts/test-resend.ts (nodemailer and dotenv); add them to
package.json as dependencies or devDependencies (e.g., "nodemailer" and
"dotenv") and run npm install (or yarn) so imports in test-resend.ts resolve;
ensure versions are compatible with your Node/TypeScript setup and update any
lockfile.
In `@src/modules/email/email.consumer.ts`:
- Around line 19-25: In handleFailure, avoid logging raw error.message which may
leak recipient PII; instead sanitize the error text before passing to
this.logger.error by redacting any email-like patterns (e.g., regex replace of
\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}\b) or only log non-sensitive fields such
as error.name/error.code; update the call in handleFailure (which currently uses
error instanceof Error ? error.message : String(error)) to use a sanitizedError
variable and continue to use this.maskEmail(job.data?.mail?.to ?? '') for the
recipient field so provider errors cannot contain unmasked email addresses.
In `@src/modules/email/queue.service.ts`:
- Around line 14-18: The queued email job currently uses retries but has no
deduplication: when adding jobs via this.emailQueue.add (mailJob) include a
stable idempotency key (e.g., jobId option derived from mail.id or a hash of
recipient+template+nonce) so Bull/Bee-Queue can dedupe retries, and make the
consumer handler that actually sends mails idempotent by checking/updating a
persistent sent-state (e.g., EmailRepository.markSent / outbox row) before
sending; ensure the consumer's send logic checks the sent flag and marks it
atomically (or uses an upsert/transaction) to avoid duplicate outbound emails on
retries or partial failures.
---
Duplicate comments:
In @.github/workflows/_build.yml:
- Around line 42-43: The "Install dependencies" step currently runs `npm
install`; change it to use `npm ci` so the workflow step (named "Install
dependencies") performs a reproducible install from package-lock.json (matching
the other workflows like `_test.yml`/`_lint.yml`) and leverages the configured
`cache: npm` for deterministic builds.
In @.github/workflows/_lint.yml:
- Around line 23-24: Replace the "Install dependencies" step's usage of npm
install with npm ci to ensure deterministic installs and lockfile integrity;
locate the workflow step named "Install dependencies" that currently runs "npm
install --include=dev" and change it to run "npm ci --include=dev" so the job
uses the lockfile and produces reproducible builds.
In `@src/modules/auth/auth.service.ts`:
- Around line 105-107: The DB error handler in AuthService during registration
should detect Postgres unique-constraint violations and map them to the existing
USER_ACCOUNT_EXIST / BAD_REQUEST response rather than a generic message; update
the block that currently checks err.name === 'QueryFailedError' (and logs via
this.logger.error('DB_ERROR during registration', err)) to also check (err as
any).code === '23505' and set the same errorMessage/status you use in the
pre-check (USER_ACCOUNT_EXIST and HttpStatus.BAD_REQUEST) before logging and
returning, otherwise keep the generic database error handling.
- Line 86: The Redis write at redisService.set(redisKey, userSession.id, 900) is
dead unless you add refresh logic—either remove that call and any related Redis
session caching, or implement a /auth/refresh flow: add an endpoint (e.g., POST
/auth/refresh) that accepts the refresh_token cookie, hash it and query
UserSession (user_sessions) for a non-revoked, unexpired refresh_token,
optionally check Redis for the session id cached by redisService.set, then
generate and return a new access token (payload: id, sub, session_id, email); if
you implement refresh, keep the redisService.set call and ensure
refreshAccessToken validates Redis first and falls back to Postgres.
- Around line 77-83: The refresh token is being stored in plaintext
(UserSession.refresh_token) — change the flow in the method that calls
generateRefreshToken() so you generate the plaintext token, set the cookie with
that plaintext, then hash the token (e.g., bcrypt.hash(...) or a keyed HMAC) and
save only the hashed value to UserSession.refresh_token before calling
queryRunner.manager.save(userSession); also update the refresh and revoke flows
to validate tokens by comparing the cookie plaintext to the stored hash using
bcrypt.compare (or HMAC verify), ensure you handle hashing errors and choose
appropriate salt/rounds.
In `@src/modules/auth/tests/auth.service.spec.ts`:
- Around line 39-53: The queryRunner.manager.save mock in dataSourceMock
currently always resolves to the same user-shaped object, so update
dataSourceMock.createQueryRunner().manager.save to use sequential
mockResolvedValueOnce(...) calls: first return the User object, second return a
UserSession object with distinct id and refresh_token values, and third return
the AuthMetadata object; keep other methods on the mock unchanged. Then update
the test's cookie assertion to assert the actual refresh_token value returned by
the second mockResolvedValueOnce (the session's refresh_token) instead of using
expect.any(String), and assert the session_id matches the id from that same
mocked UserSession.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4867391-dc09-427e-be54-a0babb67ad15
📒 Files selected for processing (23)
.env.example.github/workflows/_build.yml.github/workflows/_deploy.yml.github/workflows/_lint.yml.github/workflows/_security.yml.github/workflows/_test.yml.github/workflows/dev-deployment.yaml.github/workflows/lint-build-test.yaml.github/workflows/main-deployment.yaml.github/workflows/pipeline.yml.github/workflows/staging-deployment.yamlconfig/mailer.config.tsscripts/test-resend.tssrc/app.module.tssrc/modules/auth/auth.service.tssrc/modules/auth/dto/create-user.dto.tssrc/modules/auth/tests/auth.service.spec.tssrc/modules/email/email.consumer.spec.tssrc/modules/email/email.consumer.tssrc/modules/email/email.module.tssrc/modules/email/email.service.spec.tssrc/modules/email/queue.service.spec.tssrc/modules/email/queue.service.ts
💤 Files with no reviewable changes (4)
- .github/workflows/main-deployment.yaml
- .github/workflows/dev-deployment.yaml
- .github/workflows/staging-deployment.yaml
- .github/workflows/lint-build-test.yaml
Comment on lines
+3
to
+22
| on: | ||
| workflow_dispatch: | ||
| workflow_call: | ||
| inputs: | ||
| environment: | ||
| description: 'Target environment — controls which ecosystem config is bundled (dev | staging | prod)' | ||
| required: true | ||
| type: string | ||
| outputs: | ||
| tarball: | ||
| description: 'Tarball filename produced by this job' | ||
| value: ${{ jobs.build.outputs.tarball }} | ||
| artifact_name: | ||
| description: 'GitHub Actions artifact name that holds the tarball' | ||
| value: ${{ jobs.build.outputs.artifact_name }} | ||
|
|
||
| env: | ||
| TARBALL: nestjs-${{ inputs.environment }}.tar.gz | ||
| ECOSYSTEM_CONFIG: ${{ inputs.environment == 'prod' && 'main' || inputs.environment }}-ecosystem-config.json | ||
| ARTIFACT_NAME: build-${{ inputs.environment }} |
Contributor
There was a problem hiding this comment.
workflow_dispatch is broken: it doesn’t declare an environment input, so all env-vars resolve to empty.
Lines 20–22 derive TARBALL, ECOSYSTEM_CONFIG, and ARTIFACT_NAME from inputs.environment, but only workflow_call declares that input. When triggered manually via workflow_dispatch on this reusable workflow, inputs.environment is empty, producing:
TARBALL=nestjs-.tar.gzECOSYSTEM_CONFIG=-ecosystem-config.json(so step at line 49 fails whencpcan’t find the file)ARTIFACT_NAME=build-
Either mirror the input under workflow_dispatch, or drop the trigger entirely (the orchestrator pipeline.yml already exposes manual dispatch).
🛠️ Option A — mirror the input on `workflow_dispatch`
on:
- workflow_dispatch:
+ workflow_dispatch:
+ inputs:
+ environment:
+ description: 'Target environment (dev | staging | prod)'
+ required: true
+ type: choice
+ options: [dev, staging, prod]
workflow_call:
inputs:
environment:🛠️ Option B — remove the unused trigger
on:
- workflow_dispatch:
workflow_call:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| on: | |
| workflow_dispatch: | |
| workflow_call: | |
| inputs: | |
| environment: | |
| description: 'Target environment — controls which ecosystem config is bundled (dev | staging | prod)' | |
| required: true | |
| type: string | |
| outputs: | |
| tarball: | |
| description: 'Tarball filename produced by this job' | |
| value: ${{ jobs.build.outputs.tarball }} | |
| artifact_name: | |
| description: 'GitHub Actions artifact name that holds the tarball' | |
| value: ${{ jobs.build.outputs.artifact_name }} | |
| env: | |
| TARBALL: nestjs-${{ inputs.environment }}.tar.gz | |
| ECOSYSTEM_CONFIG: ${{ inputs.environment == 'prod' && 'main' || inputs.environment }}-ecosystem-config.json | |
| ARTIFACT_NAME: build-${{ inputs.environment }} | |
| on: | |
| workflow_dispatch: | |
| inputs: | |
| environment: | |
| description: 'Target environment (dev | staging | prod)' | |
| required: true | |
| type: choice | |
| options: [dev, staging, prod] | |
| workflow_call: | |
| inputs: | |
| environment: | |
| description: 'Target environment — controls which ecosystem config is bundled (dev | staging | prod)' | |
| required: true | |
| type: string | |
| outputs: | |
| tarball: | |
| description: 'Tarball filename produced by this job' | |
| value: ${{ jobs.build.outputs.tarball }} | |
| artifact_name: | |
| description: 'GitHub Actions artifact name that holds the tarball' | |
| value: ${{ jobs.build.outputs.artifact_name }} | |
| env: | |
| TARBALL: nestjs-${{ inputs.environment }}.tar.gz | |
| ECOSYSTEM_CONFIG: ${{ inputs.environment == 'prod' && 'main' || inputs.environment }}-ecosystem-config.json | |
| ARTIFACT_NAME: build-${{ inputs.environment }} |
Suggested change
| on: | |
| workflow_dispatch: | |
| workflow_call: | |
| inputs: | |
| environment: | |
| description: 'Target environment — controls which ecosystem config is bundled (dev | staging | prod)' | |
| required: true | |
| type: string | |
| outputs: | |
| tarball: | |
| description: 'Tarball filename produced by this job' | |
| value: ${{ jobs.build.outputs.tarball }} | |
| artifact_name: | |
| description: 'GitHub Actions artifact name that holds the tarball' | |
| value: ${{ jobs.build.outputs.artifact_name }} | |
| env: | |
| TARBALL: nestjs-${{ inputs.environment }}.tar.gz | |
| ECOSYSTEM_CONFIG: ${{ inputs.environment == 'prod' && 'main' || inputs.environment }}-ecosystem-config.json | |
| ARTIFACT_NAME: build-${{ inputs.environment }} | |
| on: | |
| workflow_call: | |
| inputs: | |
| environment: | |
| description: 'Target environment — controls which ecosystem config is bundled (dev | staging | prod)' | |
| required: true | |
| type: string | |
| outputs: | |
| tarball: | |
| description: 'Tarball filename produced by this job' | |
| value: ${{ jobs.build.outputs.tarball }} | |
| artifact_name: | |
| description: 'GitHub Actions artifact name that holds the tarball' | |
| value: ${{ jobs.build.outputs.artifact_name }} | |
| env: | |
| TARBALL: nestjs-${{ inputs.environment }}.tar.gz | |
| ECOSYSTEM_CONFIG: ${{ inputs.environment == 'prod' && 'main' || inputs.environment }}-ecosystem-config.json | |
| ARTIFACT_NAME: build-${{ inputs.environment }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/_build.yml around lines 3 - 22, The workflow's
workflow_dispatch trigger is missing the environment input so env vars TARBALL,
ECOSYSTEM_CONFIG, and ARTIFACT_NAME that rely on inputs.environment resolve to
empty; either add a matching inputs.environment declaration under
workflow_dispatch (mirror the workflow_call input: description, required: true,
type: string) so manual dispatch supplies inputs.environment, or remove the
workflow_dispatch trigger entirely so the reusable workflow only runs via
workflow_call and the env expressions remain valid; update either the
workflow_dispatch block or delete it and ensure inputs.environment is referenced
only where provided.
Comment on lines
+26
to
+29
| env: | ||
| DEPLOY_DIR: ${{ vars.DEPLOY_DIR }} | ||
| PM2_ENV: ${{ inputs.environment == 'prod' && 'main' || inputs.environment }} | ||
| REMOTE_TMP: /home/${{ secrets.USERNAME }}/tmp/nestjs |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Duplicated REMOTE_TMP and DEPLOY_DIR logic on the remote side.
REMOTE_TMP is defined as a workflow env at line 29, then redefined inside the heredoc at line 79 with the same expression. Same pattern for the local DEPLOY_DIR/PM2_ENV re-assignments at lines 77–78 — they’re just re-emitting the workflow env values into the remote shell. You can pass them through the heredoc once (already injected via ${{ env.* }}) and drop the redundant local aliases, or keep only the remote-side definitions. Tightens the script and removes the chance of drift.
Also applies to: 77-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/_deploy.yml around lines 26 - 29, The workflow defines
REMOTE_TMP, DEPLOY_DIR and PM2_ENV in the job env and then re-defines identical
local aliases inside the remote heredoc; remove the duplicated re-assignments by
using the exported workflow env values directly inside the heredoc (referencing
${ env.DEPLOY_DIR }, ${ env.PM2_ENV } and ${ env.REMOTE_TMP } there) and
eliminate the local variable re-definitions (the redundant lines that set local
DEPLOY_DIR/PM2_ENV/REMOTE_TMP in the heredoc), or alternatively keep only the
remote-side definitions and delete the job-level env entries—pick one approach
and remove the other to avoid drift (symbols: REMOTE_TMP, DEPLOY_DIR, PM2_ENV,
the heredoc block).
Comment on lines
+43
to
+51
| - name: Install sshpass | ||
| run: sudo apt-get install -y sshpass | ||
|
|
||
| - name: Prepare SSH helper | ||
| # Write a one-liner wrapper so we don't repeat the sshpass boilerplate. | ||
| # SSHPASS env var is read by sshpass automatically — keeps the password | ||
| # out of the process argument list. | ||
| run: | | ||
| echo "SSHPASS=${{ secrets.PASSWORD }}" >> $GITHUB_ENV |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
Prefer SSH key authentication over sshpass + password.
sshpass with password auth is brittle (passwords end up in env vars and process state, harder to rotate, and many modern OpenSSH builds disable password auth). A repo-stored deploy SSH private key (secrets.DEPLOY_SSH_KEY) loaded into an ssh-agent or ~/.ssh/id_ed25519 is the standard pattern and removes the need for sshpass entirely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/_deploy.yml around lines 43 - 51, Replace the
sshpass-based steps ("Install sshpass" and "Prepare SSH helper") with an SSH
key-based deploy flow: read the private key from secrets.DEPLOY_SSH_KEY (instead
of secrets.PASSWORD/SSHPASS), write it to ~/.ssh/id_ed25519 with strict file
permissions, load it into ssh-agent (or use the official actions/ssh-agent) and
add known_hosts or use ssh-keyscan to trust the target host; remove any usage of
sshpass and SSHPASS env var. Ensure the workflow steps reference the same job
names ("Install sshpass", "Prepare SSH helper") if you keep names, update them
to reflect the new key setup, and make sure the key file permissions and
agent-add are performed before any ssh/scp/rsync steps.
Comment on lines
+55
to
+74
| sshpass -e ssh -o StrictHostKeyChecking=no \ | ||
| ${{ secrets.USERNAME }}@${{ secrets.HOST }} \ | ||
| "mkdir -p /home/${{ secrets.USERNAME }}/tmp/nestjs" | ||
|
|
||
| - name: Copy tarball to server | ||
| run: | | ||
| echo "══════════════════════════════════════════" | ||
| echo " Copying ${{ inputs.tarball }} → ${{ secrets.HOST }}:${{ env.REMOTE_TMP }}" | ||
| echo "══════════════════════════════════════════" | ||
| sshpass -e scp -o StrictHostKeyChecking=no \ | ||
| ${{ inputs.tarball }} \ | ||
| ${{ secrets.USERNAME }}@${{ secrets.HOST }}:${{ env.REMOTE_TMP }}/ | ||
|
|
||
| - name: Deploy on server | ||
| env: | ||
| TARBALL: ${{ inputs.tarball }} | ||
| ENVIRONMENT: ${{ inputs.environment }} | ||
| run: | | ||
| sshpass -e ssh -o StrictHostKeyChecking=no \ | ||
| ${{ secrets.USERNAME }}@${{ secrets.HOST }} << REMOTE |
Contributor
There was a problem hiding this comment.
StrictHostKeyChecking=no disables host-key verification on every connection — MITM exposure.
Three SSH/SCP invocations all use -o StrictHostKeyChecking=no, accepting any host key without verification. A network attacker who can MitM the runner→host path could intercept the deploy session, and sshpass will hand them the password. Pin the host key once via ssh-keyscan (stored as a secret or computed from a fingerprint) and write it to ~/.ssh/known_hosts before connecting.
🔒 Suggested approach
Add a one-time setup step:
- name: Trust deploy host
env:
HOST_KEY: ${{ secrets.HOST_SSH_KEY }} # output of `ssh-keyscan -H <host>`
run: |
mkdir -p ~/.ssh
chmod 700 ~/.ssh
printf '%s\n' "$HOST_KEY" >> ~/.ssh/known_hosts
chmod 600 ~/.ssh/known_hostsThen drop -o StrictHostKeyChecking=no from the sshpass/scp commands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/_deploy.yml around lines 55 - 74, Replace the insecure "-o
StrictHostKeyChecking=no" usage in the sshpass/scp/ssh commands by adding a
one-time "Trust deploy host" step that takes a HOST_SSH_KEY (e.g.
secrets.HOST_SSH_KEY or an ssh-keyscan output) and appends it to
~/.ssh/known_hosts with strict permissions, then remove the
StrictHostKeyChecking option from all sshpass/scp/ssh invocations; reference the
ssh/scp commands and the TARBALL/REMOTE_TMP deploy step to locate where to drop
the option and add the new setup step that writes HOST_KEY into known_hosts and
sets 700/600 permissions.
|
|
||
| return { | ||
| host, | ||
| port: Number(process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? 587), |
Contributor
There was a problem hiding this comment.
Validate SMTP port before returning config
port is parsed but never validated. Non-numeric or out-of-range values will surface as runtime mail failures instead of a clear startup error.
Proposed fix
export default registerAs('mailer', () => {
const host = process.env.RESEND_SMTP_HOST ?? process.env.SMTP_HOST;
const pass = process.env.RESEND_SMTP_API_KEY ?? process.env.SMTP_PASSWORD;
const smtpUser = process.env.RESEND_SMTP_USER ?? process.env.SMTP_USER ?? 'onboarding@resend.dev';
+ const rawPort = process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? '587';
+ const port = Number(rawPort);
if (!host) throw new Error('Mailer config: RESEND_SMTP_HOST or SMTP_HOST is required');
if (!pass) throw new Error('Mailer config: RESEND_SMTP_API_KEY or SMTP_PASSWORD is required');
+ if (!Number.isInteger(port) || port < 1 || port > 65535) {
+ throw new Error('Mailer config: RESEND_SMTP_PORT or SMTP_PORT must be a valid port (1-65535)');
+ }
@@
return {
host,
- port: Number(process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? 587),
+ port,
user: smtpUser,
pass,
from: process.env.MAIL_FROM ?? fallbackFrom,
};
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/mailer.config.ts` at line 17, The mailer config currently sets port
using Number(process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? 587)
without validation; change this to explicitly parse the env value (prefer
RESEND_SMTP_PORT then SMTP_PORT), ensure it is an integer and within 1–65535,
and throw a clear startup error if invalid so the app fails fast; update the
"port" field in the mailer config (the Number(...) expression) to perform
parsing and validation and raise an Error with the offending env value when out
of range or non-numeric.
Comment on lines
+1
to
+2
| import * as nodemailer from 'nodemailer'; | ||
| import * as dotenv from 'dotenv'; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, glob
pkgs = sorted(glob.glob('**/package.json', recursive=True))
if not pkgs:
print("No package.json found")
raise SystemExit(1)
for path in pkgs:
with open(path, 'r', encoding='utf-8') as f:
data = json.load(f)
deps = {}
deps.update(data.get('dependencies', {}))
deps.update(data.get('devDependencies', {}))
missing = [name for name in ('nodemailer', 'dotenv') if name not in deps]
status = "OK" if not missing else f"MISSING: {', '.join(missing)}"
print(f"{path}: {status}")
PYRepository: hngprojects/flowbrand-be
Length of output: 42330
🏁 Script executed:
# Check the root package.json to see the actual dependencies
head -50 package.jsonRepository: hngprojects/flowbrand-be
Length of output: 2171
🏁 Script executed:
# Check if there's a package.json in the scripts directory
find scripts -name "package.json" -type fRepository: hngprojects/flowbrand-be
Length of output: 50
🏁 Script executed:
# List the contents of the scripts directory
ls -la scripts/Repository: hngprojects/flowbrand-be
Length of output: 246
🏁 Script executed:
# Check the dependencies and devDependencies sections for nodemailer and dotenv
grep -A 200 '"dependencies"' package.json | head -100Repository: hngprojects/flowbrand-be
Length of output: 3055
🏁 Script executed:
# Also search for dotenv and nodemailer specifically in package.json
grep -E '(dotenv|nodemailer)' package.jsonRepository: hngprojects/flowbrand-be
Length of output: 164
Add missing dependencies to package.json
The scripts/test-resend.ts script imports nodemailer and dotenv, but neither is declared in the root package.json. Both must be added as dependencies or devDependencies for the script to function properly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-resend.ts` around lines 1 - 2, The project is missing
package.json entries for the modules imported in scripts/test-resend.ts
(nodemailer and dotenv); add them to package.json as dependencies or
devDependencies (e.g., "nodemailer" and "dotenv") and run npm install (or yarn)
so imports in test-resend.ts resolve; ensure versions are compatible with your
Node/TypeScript setup and update any lockfile.
TheCodeGhinux
suggested changes
May 10, 2026
ibraheembello
approved these changes
May 11, 2026
Nuel-09
approved these changes
May 11, 2026
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
8 participants
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.
Pull Request
Description
Implements the
POST /auth/registerendpoint with full session creation, Redis session caching, refresh token via HttpOnly cookie, and Terms & Conditions validation.Related Issue
Fixes #(issue) — User Account and Session Creation ticket
Type of Change
How Has This Been Tested?
Test Evidence
All 61 tests passing locally. Postman screenshot attached showing
201 Createdresponse with access token, redirect URL, and user data.Screenshots
Documentation Screenshots (if applicable)
Swagger UI and Postman response screenshots attached.
Checklist
Additional Notes
terms_acceptedboolean column touserstable via new migration1778335054510-migration.tsuser_sessions(Postgres) and Redis (sess:{uid}:{sid}with 15m TTL)user_id,session_id, andemailauth_metadatatableSummary by CodeRabbit
New Features
Bug Fixes