feat: implement MTA hooks for quota enforcement#82
Open
jzunigax2 wants to merge 1 commit into
Open
Conversation
- Added MTA hooks module with authentication guard, controller, and service. - Implemented `handleRcpt` method in `MtaHooksService` to manage recipient quota checks. - Introduced `getUserUsage` method in `BridgeClient` for fetching user storage usage. - Updated `.env.template` to include MTA hooks credentials. - Added unit tests for MTA hooks functionality and authentication guard to ensure reliability.
|
📝 WalkthroughWalkthroughAdds an MTA Hooks feature: a NestJS module, controller, and service handling Stalwart's RCPT hook to enforce mailbox quota using Bridge usage data, protected by a Basic-auth guard. Adds protocol types, config/env variables, a Bridge client ChangesMTA Hooks feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Stalwart
participant MtaHooksController
participant MtaHooksAuthGuard
participant MtaHooksService
participant AccountService
participant BridgeClient
Stalwart->>MtaHooksController: POST /mta-hooks/rcpt (MtaHookRequest)
MtaHooksController->>MtaHooksAuthGuard: canActivate(request)
MtaHooksAuthGuard-->>MtaHooksController: allowed or UnauthorizedException
MtaHooksController->>MtaHooksService: handleRcpt(request)
MtaHooksService->>AccountService: findUserIdByAddress(recipient)
AccountService-->>MtaHooksService: userUuid
MtaHooksService->>BridgeClient: getUserUsage(userUuid)
BridgeClient-->>MtaHooksService: maxSpaceBytes, totalUsedSpaceBytes
MtaHooksService-->>MtaHooksController: accept or reject response
MtaHooksController-->>Stalwart: MtaHookResponse
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.env.template (1)
33-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuedotenv-linter flags key ordering.
MTA_HOOKS_SECRETshould appear beforeMTA_HOOKS_USERNAMEper the linter's alphabetical ordering rule.🔧 Fix key ordering
# MTA hooks (RCPT) -MTA_HOOKS_USERNAME=stalwart MTA_HOOKS_SECRET= +MTA_HOOKS_USERNAME=stalwart🤖 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 @.env.template around lines 33 - 36, The dotenv linter is flagging the MTA hooks entries for incorrect alphabetical ordering. Reorder the variables in the .env.template section so MTA_HOOKS_SECRET appears before MTA_HOOKS_USERNAME, keeping the existing values unchanged and preserving the surrounding comment and section structure.Source: Linters/SAST tools
src/modules/mta-hooks/mta-hooks-auth.guard.spec.ts (1)
65-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for empty-secret credentials.
This would guard against the empty-secret bypass (see comment on the guard). When the configured secret is non-empty, sending
stalwart:(empty password) should reject.♻️ Suggested additional test
+ it('when the secret is empty, then throws Unauthorized', () => { + const context = contextWithAuth(basic(username, '')); + + expect(() => guard.canActivate(context)).toThrow(UnauthorizedException); + });🤖 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/mta-hooks/mta-hooks-auth.guard.spec.ts` around lines 65 - 71, Add a spec in mta-hooks-auth.guard.spec for the empty-secret bypass case by extending the existing Basic auth parsing tests around guard.canActivate and contextWithAuth. Verify that when the configured secret is non-empty, a Basic credential decoding to a username with an empty password such as stalwart: is rejected with UnauthorizedException, alongside the existing no-colon test. Use the guard’s credential validation path in AuthGuard to locate the behavior being exercised.
🤖 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/config/configuration.ts`:
- Around line 57-60: The MTA hooks Basic auth check in configuration handling
currently allows an empty secret, which can let `stalwart:` pass as valid
credentials. Update the logic around `mtaHooks` in `configuration.ts` and the
related startup/request guard so `MTA_HOOKS_SECRET` must be non-empty: fail
startup or reject access when the secret resolves to an empty string, and ensure
the check uses the `MTA_HOOKS_SECRET`/`mtaHooks` values consistently.
In `@src/modules/mta-hooks/mta-hooks-auth.guard.ts`:
- Around line 16-20: The auth guard in MTAHooksAuthGuard is accepting an empty
secret because ConfigService.getOrThrow only rejects undefined, not ''. Update
the constructor to validate that both mtaHooks.username and mtaHooks.secret are
non-empty strings before storing them, and fail fast if the secret is missing or
blank. Use the existing MTAHooksAuthGuard constructor and
expectedSecret/expectedUsername fields to locate the change, and keep the
validation aligned with the mtaHooks configuration contract.
---
Nitpick comments:
In @.env.template:
- Around line 33-36: The dotenv linter is flagging the MTA hooks entries for
incorrect alphabetical ordering. Reorder the variables in the .env.template
section so MTA_HOOKS_SECRET appears before MTA_HOOKS_USERNAME, keeping the
existing values unchanged and preserving the surrounding comment and section
structure.
In `@src/modules/mta-hooks/mta-hooks-auth.guard.spec.ts`:
- Around line 65-71: Add a spec in mta-hooks-auth.guard.spec for the
empty-secret bypass case by extending the existing Basic auth parsing tests
around guard.canActivate and contextWithAuth. Verify that when the configured
secret is non-empty, a Basic credential decoding to a username with an empty
password such as stalwart: is rejected with UnauthorizedException, alongside the
existing no-colon test. Use the guard’s credential validation path in AuthGuard
to locate the behavior being exercised.
🪄 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: 26106eeb-7d02-47e8-93d7-20a9ea34f049
📒 Files selected for processing (12)
.env.templatesrc/app.module.tssrc/config/configuration.tssrc/modules/infrastructure/bridge/bridge.service.spec.tssrc/modules/infrastructure/bridge/bridge.service.tssrc/modules/mta-hooks/mta-hooks-auth.guard.spec.tssrc/modules/mta-hooks/mta-hooks-auth.guard.tssrc/modules/mta-hooks/mta-hooks.controller.tssrc/modules/mta-hooks/mta-hooks.module.tssrc/modules/mta-hooks/mta-hooks.service.spec.tssrc/modules/mta-hooks/mta-hooks.service.tssrc/modules/mta-hooks/mta-hooks.types.ts
Comment on lines
+57
to
+60
| mtaHooks: { | ||
| username: process.env.MTA_HOOKS_USERNAME ?? 'stalwart', | ||
| secret: process.env.MTA_HOOKS_SECRET ?? '', | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the auth guard rejects empty secrets
rg -n 'secret' src/modules/mta-hooks/mta-hooks-auth.guard.ts -C3Repository: internxt/mail-server
Length of output: 875
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== configuration.ts ==\n'
cat -n src/config/configuration.ts | sed -n '45,75p'
printf '\n== mta-hooks auth guard ==\n'
cat -n src/modules/mta-hooks/mta-hooks-auth.guard.ts | sed -n '1,120p'
printf '\n== search for mtaHooks.secret validation/usages ==\n'
rg -n "mtaHooks\.secret|MTA_HOOKS_SECRET|mtaHooks" src -C 2Repository: internxt/mail-server
Length of output: 5316
Reject empty MTA_HOOKS_SECRET values
If MTA_HOOKS_SECRET is unset, the guard compares against '', so a Basic auth value of stalwart: will satisfy the check and expose the MTA hooks endpoint. Fail startup or reject requests when the secret is empty.
🤖 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/config/configuration.ts` around lines 57 - 60, The MTA hooks Basic auth
check in configuration handling currently allows an empty secret, which can let
`stalwart:` pass as valid credentials. Update the logic around `mtaHooks` in
`configuration.ts` and the related startup/request guard so `MTA_HOOKS_SECRET`
must be non-empty: fail startup or reject access when the secret resolves to an
empty string, and ensure the check uses the `MTA_HOOKS_SECRET`/`mtaHooks` values
consistently.
Comment on lines
+16
to
+20
| constructor(configService: ConfigService) { | ||
| this.expectedUsername = | ||
| configService.getOrThrow<string>('mtaHooks.username'); | ||
| this.expectedSecret = configService.getOrThrow<string>('mtaHooks.secret'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Empty secret default enables authentication bypass when MTA_HOOKS_SECRET is unset.
The config contract (src/config/configuration.ts:59) defaults mtaHooks.secret to '':
secret: process.env.MTA_HOOKS_SECRET ?? '',getOrThrow only throws on undefined, not empty strings, so the guard silently accepts '' as the expected secret. An attacker can then authenticate with Basic c3RhbHdhcnQ6 (decodes to stalwart:), since safeEqual('', '') passes the length check and timingSafeEqual returns true for two zero-length buffers.
Validate non-empty at construction time:
🔒️ Proposed fix
constructor(configService: ConfigService) {
this.expectedUsername =
configService.getOrThrow<string>('mtaHooks.username');
this.expectedSecret = configService.getOrThrow<string>('mtaHooks.secret');
+ if (!this.expectedSecret) {
+ throw new Error(
+ 'mtaHooks.secret must be configured with a non-empty value',
+ );
+ }
}🤖 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/mta-hooks/mta-hooks-auth.guard.ts` around lines 16 - 20, The auth
guard in MTAHooksAuthGuard is accepting an empty secret because
ConfigService.getOrThrow only rejects undefined, not ''. Update the constructor
to validate that both mtaHooks.username and mtaHooks.secret are non-empty
strings before storing them, and fail fast if the secret is missing or blank.
Use the existing MTAHooksAuthGuard constructor and
expectedSecret/expectedUsername fields to locate the change, and keep the
validation aligned with the mtaHooks configuration contract.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
handleRcptmethod inMtaHooksServiceto manage recipient quota checks.getUserUsagemethod inBridgeClientfor fetching user storage usage..env.templateto include MTA hooks credentials.Summary by CodeRabbit
New Features
Bug Fixes