Bug fixes and added more tests #4

lnedry · 2025-02-07T17:20:34Z

Changes proposed in this pull request:

check host_list to confirm if local domain
added config file for allowed Message-IDs
added tests for bounce_spf()
added Sinon for creating mocks
moved bad_rcpt function from hook_data to hook_rcpt_ok
renamed config key check.reject_all to reject.all_bounces
moved config key check.bad_rcpt to reject.bad_rcpt
refactored most of the existing tests to be more thorough
Message-ID header regex to check for required angle brackets
moved reject_all from [check] to [reject] and renamed to all_bounces

Fixes #

fixed bug where config key reject.empty_return_path was ignored
fixed timeout bug in bounce_spf()
updated bounce_spf to use promises

Checklist:

docs updated
tests updated
Changes.md updated
package.json.version bumped

Update empty_return_path to match default in source code.

Moved reject_all=false from [check] to [reject] and renamed to all_bounces=false

Moved reject_all from [check] to [reject] and renamed to all_bounces

Added Sinon to create mocks for testing

- Updated bounce_spf() to use Promises. - Moved bad_rcpt function from hook_data to hook_rcpt_ok. - Renamed config key check.reject_all to reject.all_bounces. - Moved config key check.bad_rcpt to reject.bad_rcpt - Fixed bug where config key reject.empty_return_path was ignored. - Fixed timeout bug in bounce_spf() - Modified Message-ID header regex to check for required angle brackets.

Moved config key check.bad_rcpt to reject.bad_rcpt.

- Added tests for bounce_spf() - Refactored most of the existing tests to be more thorough.

Moved config key check.bad_rcpt to reject.bad_rcpt

msimerson · 2025-02-09T20:58:54Z

index.js

+  // legacy settings
+  const c = this.cfg
+  if (c.check.reject_all) c.reject.all_bounces = c.check.reject_all
+  if (c.check.reject) c.reject.reject = c.check.reject


this doesn't look right

msimerson · 2025-02-09T21:00:47Z

index.js


-  next(DENY, 'this bounce message does not have 1 recipient')
+  return next()


Suggested change

return next()

next()

Avoid a useless use of return here.

msimerson · 2025-02-09T21:02:39Z

index.js

  }

-  transaction.results.add(this, { fail: 'empty_return_path', emit: true })
-  return next(DENY, 'bounce with non-empty Return-Path (RFC 3834)')
+  return next()


Suggested change

return next()

next()

Again, avoid useless use of return.

msimerson · 2025-02-09T21:06:30Z

index.js


-  next()
+  return next()


Suggested change

return next()

next()

avoid useless use of return. (The calling function does nothing with the functions return. Not using return next... everywhere makes it subtly more obvious that the return next... patterns above are strictly for flow control.)

msimerson · 2025-02-09T21:11:54Z

index.js

 }

 exports.has_null_sender = function (connection, mail_from) {
-  const transaction = connection?.transaction
-  if (!transaction) return false
+  const { transaction } = connection


You're no longer guarding against the transaction having been destroyed. How can you be certain that when this function is called, the transaction still exists?

Under what circumstances could the transaction be destroyed before one of these hooks gets called?

It's most common when the remote disconnects in the middle of the conversation. Any hooks that were running before the connection dropped will still be running. The transaction may get cleaned up while a plugin hook is running, especially if the hook does anything asynchronous. Then subsequent attempts to access connection.transaction will be undefined and trigger an exception, which takes down the node process.

index.js

msimerson · 2025-02-09T21:16:42Z

index.js

-  //         {fail: 'Message-ID not local', emit: true });
-  // if (!plugin.cfg.reject.non_local_msgid) return next();
-  // return next(DENY, "bounce with non-local Message-ID (RFC 3834)");
+  return next()


Suggested change

return next()

next()

msimerson · 2025-02-09T21:18:35Z

index.js

+    return next(DENY, 'Invalid bounce (spoofed sender)')
+  }
+
+  return next()


Suggested change

return next()

next()

msimerson

I like this PR, especially the improvement of config naming.

Removed spurious line of code

- Removed description for reject_all - Added description for all_bounces

Removed useless use of return

Restored transaction guarding

Added test to check for missing transaction

lnedry · 2025-02-10T00:33:31Z

Removed spurious line of code.
Changed several return next() statements to just next()
Restored transaction guarding.
Added test for transaction guarding.
Replaced description in README for reject_all with description for all_bounces

msimerson · 2025-02-10T15:52:13Z

index.js

@@ -103,16 +120,19 @@ exports.single_recipient = function (next, connection) {

  transaction.results.add(this, { fail: 'single_recipient', emit: true })

-  if (!this.cfg.reject.single_recipient) return next()
+  if (this.cfg.reject.single_recipient) {
+    next(DENY, 'this bounce message has too many recipients')


Suggested change

next(DENY, 'this bounce message has too many recipients')

return next(DENY, 'this bounce message has too many recipients')

You do need the return here, for flow control. Otherwise you'll be calling next() twice.

msimerson · 2025-02-10T16:01:06Z

index.js

-    const rcpt = element.address()
-    if (!this.cfg.invalid_addrs[rcpt]) continue
-    transaction.results.add(this, { fail: 'bad_rcpt', emit: true })
+  if (this.cfg.invalid_addrs.includes(rcpt.address())) {


Is the result of rcpt.address() always lower case?

msimerson · 2025-02-10T16:02:34Z

index.js

-  const transaction = connection?.transaction
-  if (!transaction) return false
+  const { transaction } = connection
+  if (!transaction) return next()


Suggested change

if (!transaction) return next()

if (!transaction) return false

There's no next() in context here.

msimerson · 2025-02-10T16:09:58Z

Perhaps a good test to add for each bounce test is passing in a connection without a transaction, and assuring that the function returns (doesn't trigger an exception).

minor cose cleanup

msimerson

These are the last few issues I see.

msimerson · 2025-02-10T22:36:32Z

index.js

  }
 }

 exports.non_local_msgid = function (next, connection) {
  if (!this.cfg.check.non_local_msgid) return next()
  if (!this.has_null_sender(connection)) return next()

-  const transaction = connection?.transaction
-  if (!transaction) return next()
+  const { transaction } = connection


Suggested change

const { transaction } = connection

const { transaction } = connection

if (!transaction) return

msimerson · 2025-02-10T22:39:33Z

index.js

@@ -293,24 +305,22 @@ function find_received_headers(ips, body, connection, self) {
 }

 exports.bounce_spf_enable = function (next, connection) {
-  if (!connection.transaction) return next()


restore transaction guarding here.

msimerson · 2025-02-10T22:40:01Z

index.js

  if (!this.cfg.check.bounce_spf) return next()
  if (!this.has_null_sender(connection)) return next()

-  const txn = connection?.transaction
-  if (!txn) return next()
+  const { transaction } = connection


Suggested change

const { transaction } = connection

const { transaction } = connection

if (!transaction) return next()

- added tests for transaction guarding - moved transaction guarding from has_null_sender() to each hook - refactored has_null_sender()

Added tests for guarding transactions

msimerson

Looking good!

A final issue is documentation. You've added the use of three new config files to the plugin:

host_list
bounce_bad_rcpt
bounce_allowed_msgids

None of those files are mentioned in the README, nor is the format for what you'd put in them mentioned. There are alternatives to accomplish this:

1. Move the data into bounce.ini and add inline docs there

[check]
single_recipient=true
empty_return_path=true

[reject]
single_recipient=true
empty_return_path=true

; reject all bounce messages (generally a bad idea)
all_bounces=false

; reject bounces to "bad" recipients
bad_rcpt=true

; bad_rcpts[] = [email protected]
; bad_rcpts[] = [email protected]
; bad_rcpts[] = ....


; to validate bounce message IDs, add an entry to the domains array
; for each email domain you accept email for.
[msgid]
; domains[] = example.com
; domains[] = example.net
; domains[] = example.org

2. Add a section to the README for each config file

Explain why you'd use the file and what the contents of the file should be.

Finally, the local msgid checks the domain against the contents of the host_list file. In my case, there's 4 domains listed in hosts_list, but that file hasn't been updated since 2021 and the rcpt_to.in_hosts_list plugin isn't even loaded. I think you want to say something like this:

The Message-ID test needs to know what domains are 'local' to your mail server. If you use the rcpt_to.host_list plugin and have populated the config/host_list file, then you're done.
If you don't use that plugin, make sure the host_list file is empty and populate the [msgid].domains[] array in bounce.ini (or config/bounce_allowed_msgids).

Also, if you keep the file name bounce_allowed_msgids, a name that better indicates what goes in it might be bounce_msgid_allowed_domains?

updated settings to match defaults in source

- updated documentation for all_bounces and bad_rcpt - added documentation for non_local_msgid

minor code clean up

cleaned up test code

lnedry added 8 commits February 7, 2025 11:44

Update README.md

e1cc1fd

Update empty_return_path to match default in source code.

Update README.md

6444e75

Moved reject_all=false from [check] to [reject] and renamed to all_bounces=false

Update bounce.ini

7e8e7cc

Moved reject_all from [check] to [reject] and renamed to all_bounces

Update package.json

8fd7fe0

Added Sinon to create mocks for testing

Update bounce.ini

f568dce

Moved config key check.bad_rcpt to reject.bad_rcpt.

Update index.js

997f24b

- Added tests for bounce_spf() - Refactored most of the existing tests to be more thorough.

Update README.md

f4684f3

Moved config key check.bad_rcpt to reject.bad_rcpt

msimerson reviewed Feb 9, 2025

View reviewed changes

index.js Show resolved Hide resolved

msimerson reviewed Feb 9, 2025

View reviewed changes

index.js Outdated

return next(DENY, 'Invalid bounce (spoofed sender)')

}

return next()

Copy link

Member

msimerson Feb 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

return next()

next()

msimerson reviewed Feb 9, 2025

View reviewed changes

lnedry added 6 commits February 9, 2025 18:36

Update index.js

6326bbf

Removed spurious line of code

Update README.md

1affd94

- Removed description for reject_all - Added description for all_bounces

Update index.js

35a07b3

Removed useless use of return

Update index.js

8cc9bc9

Removed useless use of return

Update index.js

4c967ee

Restored transaction guarding

Update index.js

18f8a69

Added test to check for missing transaction

msimerson reviewed Feb 10, 2025

View reviewed changes

Update index.js

a5ce318

minor cose cleanup

msimerson requested changes Feb 10, 2025

View reviewed changes

lnedry added 2 commits February 11, 2025 16:33

Update index.js

fc6d428

- added tests for transaction guarding - moved transaction guarding from has_null_sender() to each hook - refactored has_null_sender()

Update index.js

7da824e

Added tests for guarding transactions

msimerson requested changes Feb 11, 2025

View reviewed changes

lnedry added 4 commits February 15, 2025 08:48

Update bounce.ini

d0aa69e

updated settings to match defaults in source

Update README.md

526f08f

- updated documentation for all_bounces and bad_rcpt - added documentation for non_local_msgid

Update index.js

2064336

minor code clean up

Update index.js

76e56f0

cleaned up test code

msimerson merged commit 6f5d5a6 into haraka:master Feb 16, 2025
6 checks passed

This was referenced Feb 16, 2025

Bounce: non_local_msgid is broken #1

Closed

Release v1.0.5 #5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixes and added more tests #4

Bug fixes and added more tests #4

lnedry commented Feb 7, 2025

msimerson Feb 9, 2025

msimerson Feb 9, 2025

msimerson Feb 9, 2025

msimerson Feb 9, 2025

msimerson Feb 9, 2025

lnedry Feb 10, 2025

msimerson Feb 10, 2025 •

edited

Loading

msimerson Feb 9, 2025

msimerson Feb 9, 2025

msimerson left a comment

lnedry commented Feb 10, 2025

msimerson Feb 10, 2025

msimerson Feb 10, 2025

msimerson Feb 10, 2025 •

edited

Loading

msimerson commented Feb 10, 2025

msimerson left a comment

msimerson Feb 10, 2025

msimerson Feb 10, 2025

msimerson Feb 10, 2025

msimerson left a comment •

edited

Loading


		next(DENY, 'this bounce message does not have 1 recipient')
		return next()

	next(DENY, 'this bounce message has too many recipients')
	return next(DENY, 'this bounce message has too many recipients')

	if (!transaction) return next()
	if (!transaction) return false

	const { transaction } = connection
	const { transaction } = connection
	if (!transaction) return

Bug fixes and added more tests #4

Bug fixes and added more tests #4

Conversation

lnedry commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimerson Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimerson left a comment

Choose a reason for hiding this comment

lnedry commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimerson Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

msimerson commented Feb 10, 2025

msimerson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimerson left a comment • edited Loading

Choose a reason for hiding this comment

1. Move the data into bounce.ini and add inline docs there

2. Add a section to the README for each config file

msimerson Feb 10, 2025 •

edited

Loading

msimerson Feb 10, 2025 •

edited

Loading

msimerson left a comment •

edited

Loading