From 9cd15342e0e4aac92fc8965f6b1131bb9db6347a Mon Sep 17 00:00:00 2001 From: Juergen Kunz Date: Tue, 28 Oct 2025 11:13:47 +0000 Subject: [PATCH] feat(tests): Implement ERR-02 Invalid Sequence Handling and update test migration documentation --- test/helpers/server.loader.ts | 1 + test/readme.md | 50 +++++++++++++++---- test/readme.testmigration.md | 12 ++--- .../delivery/smtpserver/command-handler.ts | 16 +++--- 4 files changed, 54 insertions(+), 25 deletions(-) diff --git a/test/helpers/server.loader.ts b/test/helpers/server.loader.ts index 47399ef..e2cd97f 100644 --- a/test/helpers/server.loader.ts +++ b/test/helpers/server.loader.ts @@ -133,6 +133,7 @@ export async function startTestServer(config: ITestServerConfig): Promise {}, recordSyntaxError: async (_ip: string) => {}, recordCommandError: async (_ip: string) => {}, + recordError: (_ip: string) => false, // Returns false = don't block IP in tests isBlocked: async (_ip: string) => false, cleanup: async () => {}, }; diff --git a/test/readme.md b/test/readme.md index 3edf5e1..4e5dea4 100644 --- a/test/readme.md +++ b/test/readme.md @@ -99,7 +99,7 @@ Tests for proper error handling and recovery. | ID | Test | Priority | Status | |----|------|----------|--------| | **ERR-01** | **Syntax Error Handling** | **High** | **✅ PORTED** | -| ERR-02 | Invalid Sequence Handling | High | Planned | +| **ERR-02** | **Invalid Sequence Handling** | **High** | **✅ PORTED** | | ERR-05 | Resource Exhaustion | High | Planned | | ERR-07 | Exception Handling | High | Planned | @@ -275,16 +275,36 @@ Tests for proper error handling and recovery. - Server lifecycle management **Key validations**: -- ✓ Invalid commands rejected with appropriate error codes +- ✓ Invalid commands rejected with 500/502 error codes - ✓ MAIL FROM requires angle brackets (501 error if missing) - ✓ RCPT TO requires angle brackets (501 error if missing) - ✓ EHLO requires hostname parameter (501 error if missing) -- ✓ Extra parameters on QUIT handled (accepted or rejected with 501) -- ✓ Malformed email addresses rejected (501 or 553 error) +- ✓ Extra parameters on QUIT handled (501 syntax error) +- ✓ Malformed email addresses rejected (501 error) - ✓ Commands in wrong sequence rejected (503 error) - ✓ Excessively long commands handled gracefully -**Note**: Server currently has a bug where `rateLimiter.recordError` is not implemented, causing invalid commands to return 451 (temporary error) instead of 500/502 (syntax error). Tests accept 451 as valid until this is fixed. +### ✅ ERR-02: Invalid Sequence Handling (`test.err-02.invalid-sequence.test.ts`) + +**Tests**: 10 total (10 passing) +- Rejects MAIL FROM before EHLO +- Rejects RCPT TO before MAIL FROM +- Rejects DATA before RCPT TO (RFC 5321 compliance) +- Allows multiple EHLO commands +- Handles second MAIL FROM without RSET +- Rejects DATA without MAIL FROM +- Handles commands after QUIT +- Recovers from syntax errors in sequence +- Server lifecycle management + +**Key validations**: +- ✓ MAIL FROM requires EHLO first (503 error if missing) +- ✓ RCPT TO requires MAIL FROM first (503 error if missing) +- ✓ DATA requires RCPT TO with at least one recipient (503 error if missing) +- ✓ Multiple EHLO commands allowed (resets session state) +- ✓ Commands after QUIT handled correctly (connection closed) +- ✓ Session recovers from syntax errors without terminating +- ✓ RFC 5321 compliance: strict command sequence enforcement ## Running Tests @@ -383,7 +403,7 @@ import { connectToSmtp, sendSmtpCommand } from '../../helpers/utils.ts'; - 🔄 SEC-08: Rate Limiting - 🔄 SEC-10: Header Injection Prevention - ✅ ERR-01: Syntax Error Handling -- 🔄 ERR-02: Invalid Sequence Handling +- ✅ ERR-02: Invalid Sequence Handling ### Phase 3: Advanced Features (Medium Priority) - 🔄 SEC-03: DKIM Processing @@ -408,7 +428,7 @@ import { connectToSmtp, sendSmtpCommand } from '../../helpers/utils.ts'; - SMTP protocol utilities with readSmtpResponse helper - Test certificates (self-signed RSA) -**Tests Ported**: 10/100+ test files (72 total tests passing) +**Tests Ported**: 11/100+ test files (82 total tests passing) - ✅ CMD-01: EHLO Command (5 tests passing) - ✅ CMD-02: MAIL FROM Command (6 tests passing) - ✅ CMD-03: RCPT TO Command (7 tests passing) @@ -419,6 +439,7 @@ import { connectToSmtp, sendSmtpCommand } from '../../helpers/utils.ts'; - ✅ EP-01: Basic Email Sending (7 tests passing) - ✅ SEC-06: IP Reputation Checking (7 tests passing) - ✅ ERR-01: Syntax Error Handling (10 tests passing) +- ✅ ERR-02: Invalid Sequence Handling (10 tests passing) **Coverage**: Complete essential SMTP transaction flow - EHLO → MAIL FROM → RCPT TO → DATA → QUIT ✅ @@ -427,10 +448,19 @@ import { connectToSmtp, sendSmtpCommand } from '../../helpers/utils.ts'; **Phase 1 Status**: ✅ **COMPLETE** (7/7 tests, 100%) +**Phase 2 Status**: 🔄 **IN PROGRESS** (3/6 tests, 50%) +- ✅ SEC-06: IP Reputation +- ✅ ERR-01: Syntax Errors +- ✅ ERR-02: Invalid Sequence +- 🔄 SEC-01: Authentication +- 🔄 SEC-08: Rate Limiting +- 🔄 SEC-10: Header Injection + **Next Steps**: -1. Port remaining security tests (SEC-01 Authentication, SEC-08 Rate Limiting, SEC-10 Header Injection) -2. Port ERR-02: Invalid Sequence Handling test -3. Continue with remaining high-priority tests +1. Port SEC-01: Authentication test +2. Port SEC-08: Rate Limiting test +3. Port SEC-10: Header Injection Prevention test +4. Continue with Phase 3 (Advanced Features) ## Production Readiness Criteria diff --git a/test/readme.testmigration.md b/test/readme.testmigration.md index 45baf58..d242089 100644 --- a/test/readme.testmigration.md +++ b/test/readme.testmigration.md @@ -92,7 +92,7 @@ Tests for proper error handling and recovery. | Test ID | Source File | Destination File | Status | Tests | Notes | |---------|-------------|------------------|--------|-------|-------| | **ERR-01** | (dcrouter ERR-01 tests) | `test/suite/smtpserver_error-handling/test.err-01.syntax-errors.test.ts` | **✅ Ported** | 10/10 | Invalid commands, missing brackets, wrong sequences, long commands, malformed addresses | -| ERR-02 | TBD | `test/suite/smtpserver_error-handling/test.err-02.sequence-errors.test.ts` | 📋 Planned | - | Out-of-order commands | +| **ERR-02** | (dcrouter ERR-02 tests) | `test/suite/smtpserver_error-handling/test.err-02.invalid-sequence.test.ts` | **✅ Ported** | 10/10 | MAIL before EHLO, RCPT before MAIL, DATA before RCPT, multiple EHLO, commands after QUIT, sequence recovery | | ERR-05 | TBD | `test/suite/smtpserver_error-handling/test.err-05.resource-exhaustion.test.ts` | 📋 Planned | - | Memory/connection limits | | ERR-07 | TBD | `test/suite/smtpserver_error-handling/test.err-07.exception-handling.test.ts` | 📋 Planned | - | Unexpected errors, crashes | @@ -146,9 +146,9 @@ Tests for RFC 5321/5322 compliance. ### Overall Statistics - **Total test files identified**: ~100+ -- **Files ported**: 10/100+ (10%) -- **Total tests ported**: 72/~500+ (14%) -- **Tests passing**: 72/72 (100%) +- **Files ported**: 11/100+ (11%) +- **Total tests ported**: 82/~500+ (16%) +- **Tests passing**: 82/82 (100%) ### By Priority @@ -169,9 +169,9 @@ Tests for RFC 5321/5322 compliance. - 📋 SEC-08: Rate Limiting - 📋 SEC-10: Header Injection - ✅ ERR-01: Syntax Errors (10 tests) -- 📋 ERR-02: Sequence Errors +- ✅ ERR-02: Invalid Sequence (10 tests) -**Phase 2 Progress**: 2/6 complete (33%) +**Phase 2 Progress**: 3/6 complete (50%) #### Medium Priority (Phase 3: Advanced Features) - 📋 SEC-03: DKIM diff --git a/ts/mail/delivery/smtpserver/command-handler.ts b/ts/mail/delivery/smtpserver/command-handler.ts index ae6d678..d7f1741 100644 --- a/ts/mail/delivery/smtpserver/command-handler.ts +++ b/ts/mail/delivery/smtpserver/command-handler.ts @@ -719,22 +719,20 @@ export class CommandHandler implements ICommandHandler { return; } - // For tests, be slightly more permissive - also accept DATA after MAIL FROM - // But ensure we at least have a sender defined - if (session.state !== SmtpState.RCPT_TO && session.state !== SmtpState.MAIL_FROM) { + // RFC 5321: DATA must only be accepted after RCPT TO + if (session.state !== SmtpState.RCPT_TO) { this.sendResponse(socket, `${SmtpResponseCode.BAD_SEQUENCE} Bad sequence of commands`); return; } - - // Check if we have a sender + + // RFC 5321: Must have a sender if (!session.mailFrom) { this.sendResponse(socket, `${SmtpResponseCode.BAD_SEQUENCE} No sender specified`); return; } - - // Ideally we should have recipients, but for test compatibility, we'll only - // insist on recipients if we're in RCPT_TO state - if (session.state === SmtpState.RCPT_TO && !session.rcptTo.length) { + + // RFC 5321: Must have at least one recipient + if (!session.rcptTo.length) { this.sendResponse(socket, `${SmtpResponseCode.BAD_SEQUENCE} No recipients specified`); return; }