fix: enforce 401 in RequireAuthentication when session is invalid

[aalemayhu]

Previously the middleware always called next() regardless of whether authentication succeeded, allowing requests with expired or missing cookies to reach handlers with undefined owner, causing Knex binding errors. Added a test to cover both authenticated and unauthenticated cases.

Summary by CodeRabbit

• Bug Fixes

  • Enhanced authentication middleware to properly validate user authorization. Unauthorized requests now receive a 401 Unauthorized response instead of proceeding.

• Tests

  • Added comprehensive test coverage for authentication middleware, validating both authorized and unauthorized access scenarios.

[coderabbitai]

Warning

Rate limit exceeded

@aalemayhu has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 50 minutes and 45 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 50 minutes and 45 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a677822f-c8d6-4a80-8905-9ffe304ff740

📥 Commits

Reviewing files that changed from the base of the PR and between 921f914 and ac4b021.

📒 Files selected for processing (2)

• src/routes/middleware/RequireAuthentication.test.ts
• src/routes/middleware/RequireAuthentication.ts

📝 Walkthrough

Walkthrough

A new test suite was added for the RequireAuthentication middleware, and the middleware itself was modified to perform an authorization check. The middleware now returns HTTP 401 with { error: 'Unauthorized' } if res.locals.owner is absent, instead of always proceeding to the next middleware.

Changes

Cohort / File(s)	Summary
Middleware Authorization Logic src/routes/middleware/RequireAuthentication.ts	Added early-exit authorization check: returns 401 response if res.locals.owner is falsy after configureUserLocal, introducing gated control flow instead of unconditional next() call.
Test Suite src/routes/middleware/RequireAuthentication.test.ts	New Jest test suite with mocked AuthenticationService and data layer. Tests two scenarios: user found (middleware calls next) and user not found (middleware returns 401 with error object).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A guard at the gate, with a check sharp and swift,
Now blocks those without the right shift.
With tests all aligned to the newly drawn line,
The owner must present, or doors won't unwind. 🚪✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main change: enforcing a 401 response in RequireAuthentication when the session is invalid, which matches the core fix implemented in the middleware.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/require-authentication-enforce-401

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/routes/middleware/RequireAuthentication.test.ts (1)

37-46: Strengthen the happy-path assertion to prevent false positives.

At Line 45, asserting only next() can miss cases where a response is also sent. Capture res and assert status/send were not called.

Suggested test hardening

   it('calls next when the user is authenticated', async () => {
     mockedAuthService.prototype.getUserFrom = jest
       .fn()
       .mockResolvedValue({ id: 1, owner: 1, email: 'test@test.com' });

+    const res = mockResponse();
     const next = jest.fn();
-    await RequireAuthentication(mockRequest(), mockResponse(), next);
+    await RequireAuthentication(mockRequest(), res, next);

     expect(next).toHaveBeenCalled();
+    expect(res.status).not.toHaveBeenCalled();
+    expect(res.send).not.toHaveBeenCalled();
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/routes/middleware/RequireAuthentication.test.ts` around lines 37 - 46,
The test for RequireAuthentication currently only asserts next() was called
which can miss cases where a response was also sent; change the test to capture
the mocked response object (use mockResponse()), pass that res into
RequireAuthentication alongside mockRequest() and next, and then assert that
res.status and res.send (or res.json if used by middleware) were not called in
addition to expect(next).toHaveBeenCalled(); this ensures the happy path does
not inadvertently send a response.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/routes/middleware/RequireAuthentication.ts`:
- Around line 30-32: The current auth gate uses a falsy check on
res.locals.owner and will block valid values like 0; change the check in
RequireAuthentication (where res.locals.owner is evaluated) to a nullish check
that only rejects null or undefined (e.g., test for owner == null or owner ===
undefined) so legitimate falsy IDs are allowed, and keep the same 401 response
path when owner is null/undefined.

---

Nitpick comments:
In `@src/routes/middleware/RequireAuthentication.test.ts`:
- Around line 37-46: The test for RequireAuthentication currently only asserts
next() was called which can miss cases where a response was also sent; change
the test to capture the mocked response object (use mockResponse()), pass that
res into RequireAuthentication alongside mockRequest() and next, and then assert
that res.status and res.send (or res.json if used by middleware) were not called
in addition to expect(next).toHaveBeenCalled(); this ensures the happy path does
not inadvertently send a response.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 19feec3e-43d3-4824-ab16-02fc01ba0bdc

📥 Commits

Reviewing files that changed from the base of the PR and between da3774f and 921f914.

📒 Files selected for processing (2)

• src/routes/middleware/RequireAuthentication.test.ts
• src/routes/middleware/RequireAuthentication.ts

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud