fix: add workaround for “greeting never received” (#5079)

* fix: add workaround for “greeting never received”

* add changeset

* include suggestions from review

* Changes from lint:fix

* improve tests

---------

Co-authored-by: cloud-sdk-js <cloud-sdk-js@github.com>

.changeset/yellow-poets-ring.md

@@ -0,0 +1,5 @@
+---
+'@sap-cloud-sdk/mail-client': patch
+---
+
+[Fixed Issue] Fix sending e-mails through socks proxies in Node 20 and higher.

packages/mail-client/package.json

@@ -40,6 +40,7 @@
  "dependencies": {
  "@sap-cloud-sdk/connectivity": "^3.22.1",
  "@sap-cloud-sdk/util": "^3.22.1",
+ "async-retry": "^1.3.3",
  "nodemailer": "6.9.15",
  "socks": "2.8.3"
  },

packages/mail-client/src/mail-client-types.ts

@@ -339,21 +339,3 @@ export interface SmtpTransportOptions {
  */
  proxy?: string;
 }
-
-/**
- * @internal
- */
-interface ReadableState {
- readableListening: boolean;
-}
-
-/**
- * Represents a socket object used for On-Premise proxy.
- * @internal
- */
-export interface SocksSocket extends net.Socket {
- /**
- * @internal
- */
- _readableState: ReadableState;
-}

packages/mail-client/src/mail-client.spec.ts

@@ -1,3 +1,4 @@
+import { EventEmitter } from 'node:stream';
 import nodemailer from 'nodemailer';
 import { SocksClient } from 'socks';
 import { registerDestination } from '@sap-cloud-sdk/connectivity';
@@ -25,15 +26,6 @@ describe('mail client', () => {
  jest.resetAllMocks();
  });
 
- const mockSocket = {
- socket: {
- _readableState: {
- readableListening: false
- },
- end: jest.fn(),
- destroy: jest.fn()
- }
- };
  const mockTransport = {
  sendMail: jest.fn(),
  close: jest.fn(),
@@ -86,12 +78,11 @@ describe('mail client', () => {
  });
 
  it('should work with destination from service - proxy-type OnPremise', async () => {
- jest
- .spyOn(SocksClient, 'createConnection')
- .mockReturnValue(mockSocket as any);
+ mockSocketConnection();
  jest
  .spyOn(nodemailer, 'createTransport')
  .mockReturnValue(mockTransport as any);
+
  const mailOptions1: MailConfig = {
  from: 'from2@example.com',
  to: 'to2@example.com'
@@ -215,27 +206,17 @@ describe('mail client', () => {
  await expect(
  sendMail(destination, [mailOptions1, mailOptions2], mailClientOptions)
  ).resolves.not.toThrow();
- expect(spyCreateTransport).toBeCalledTimes(1);
- expect(spyCreateTransport).toBeCalledWith(
+ expect(spyCreateTransport).toHaveBeenCalledTimes(1);
+ expect(spyCreateTransport).toHaveBeenCalledWith(
  expect.objectContaining(mailClientOptions)
  );
- expect(spySendMail).toBeCalledTimes(2);
- expect(spySendMail).toBeCalledWith(mailOptions1);
- expect(spySendMail).toBeCalledWith(mailOptions2);
- expect(spyCloseTransport).toBeCalledTimes(1);
+ expect(spySendMail).toHaveBeenCalledTimes(2);
+ expect(spySendMail).toHaveBeenCalledWith(mailOptions1);
+ expect(spySendMail).toHaveBeenCalledWith(mailOptions2);
+ expect(spyCloseTransport).toHaveBeenCalledTimes(1);
  });
 
- it('[OnPrem] should create transport/socket, send mails and close the transport/socket', async () => {
- const spyCreateSocket = jest
- .spyOn(SocksClient, 'createConnection')
- .mockReturnValue(mockSocket as any);
- const spyCreateTransport = jest
- .spyOn(nodemailer, 'createTransport')
- .mockReturnValue(mockTransport as any);
- const spySendMail = jest.spyOn(mockTransport, 'sendMail');
- const spyCloseTransport = jest.spyOn(mockTransport, 'close');
- const spyEndSocket = jest.spyOn(mockSocket.socket, 'end');
- const spyDestroySocket = jest.spyOn(mockSocket.socket, 'destroy');
+ describe('on premise', () => {
  const destination: any = {
  originalProperties: {
  'mail.password': 'password',
@@ -258,25 +239,96 @@ describe('mail client', () => {
  from: 'from1@example.com',
  to: 'to1@example.com'
  };
- await expect(
- sendMail(destination, mailOptions, { sdkOptions: { parallel: false } })
- ).resolves.not.toThrow();
- expect(spyCreateSocket).toBeCalledTimes(1);
- expect(spyCreateTransport).toBeCalledTimes(1);
- expect(spySendMail).toBeCalledTimes(1);
- expect(spySendMail).toBeCalledWith(mailOptions);
- expect(spyCloseTransport).toBeCalledTimes(1);
- expect(spyEndSocket).toBeCalledTimes(1);
- expect(spyDestroySocket).toBeCalledTimes(1);
+
+ it('should create transport/socket, send mails and close the transport/socket', async () => {
+ const { connection, createConnectionSpy } = mockSocketConnection();
+ const spyCreateTransport = jest
+ .spyOn(nodemailer, 'createTransport')
+ .mockReturnValue(mockTransport as any);
+ const spySendMail = jest.spyOn(mockTransport, 'sendMail');
+
+ const spyCloseTransport = jest.spyOn(mockTransport, 'close');
+ const spyEndSocket = jest.spyOn(connection.socket, 'end');
+ const spyDestroySocket = jest.spyOn(connection.socket, 'destroy');
+
+ await expect(
+ sendMail(destination, mailOptions, { sdkOptions: { parallel: false } })
+ ).resolves.not.toThrow();
+ expect(createConnectionSpy).toHaveBeenCalledTimes(1);
+ expect(spyCreateTransport).toHaveBeenCalledTimes(1);
+ expect(spySendMail).toHaveBeenCalledTimes(1);
+ expect(spySendMail).toHaveBeenCalledWith(mailOptions);
+ expect(spyCloseTransport).toHaveBeenCalledTimes(1);
+ expect(spyEndSocket).toHaveBeenCalledTimes(1);
+ expect(spyDestroySocket).toHaveBeenCalledTimes(1);
+ });
+
+ it('should resend greeting', async () => {
+ const { connection } = mockSocketConnection();
+ jest
+ .spyOn(nodemailer, 'createTransport')
+ .mockReturnValue(mockTransport as any);
+
+ const req = sendMail(destination, mailOptions, {
+ sdkOptions: { parallel: false }
+ });
+
+ // The socket emits data for the first time before nodemailer listens to it.
+ // We re-emit the data until a listener listened for it.
+ // In this test we listen for the data event to check that we in fact re-emit the message.
+ const emitsTwice = new Promise(resolve => {
+ let dataEmitCount = 0;
+ const collectedData: string[] = [];
+ connection.socket.on('data', data => {
+ dataEmitCount++;
+ collectedData.push(data.toString());
+ if (dataEmitCount === 2) {
+ resolve(collectedData);
+ }
+ });
+ });
+
+ await expect(emitsTwice).resolves.toEqual([
+ '220 smtp.gmail.com ESMTP',
+ '220 smtp.gmail.com ESMTP'
+ ]);
+ await expect(req).resolves.not.toThrow();
+ });
+
+ it('should fail if nodemailer never listens to greeting', async () => {
+ mockSocketConnection();
+
+ jest
+ .spyOn(nodemailer, 'createTransport')
+ .mockReturnValue(mockTransport as any);
+
+ const req = sendMail(destination, mailOptions, {
+ sdkOptions: { parallel: false }
+ });
+ // jest.useFakeTimers();
+ // jest.advanceTimersByTime(1000);
+
+ await expect(req).rejects.toThrow();
+ }, 10000);
+
+ it('should throw if greeting (really) was not received', async () => {
+ mockSocketConnection(true);
+
+ await expect(() =>
+ sendMail(destination, mailOptions, {
+ sdkOptions: { parallel: false }
+ })
+ ).rejects.toThrowErrorMatchingInlineSnapshot('"Something went wrong"');
+ });
  });
 });
 
 describe('isMailSentInSequential', () => {
- it('should return false when the mail client options is undefined', () => {
+ it('should return false when the mail client options are undefined', () => {
  expect(isMailSentInSequential()).toBe(false);
  });
 
- it('should return false when the sdk options is undefined', () => {
+ it('should return false when the sdk options are undefined', () => {
  const mailClientOptions: MailClientOptions = {};
  expect(isMailSentInSequential(mailClientOptions)).toBe(false);
  });
@@ -327,3 +379,28 @@ function isValidSocksProxy(proxy) {
  (proxy.type === 4 || proxy.type === 5)
  );
 }
+
+function mockSocketConnection(fail = false) {
+ class MockSocket extends EventEmitter {
+ end = jest.fn();
+ destroy = jest.fn();
+ }
+
+ const connection = {
+ socket: new MockSocket()
+ };
+ const createConnectionSpy = jest
+ .spyOn(SocksClient, 'createConnection')
+ .mockImplementation(() => {
+ setImmediate(() => {
+ if (fail) {
+ connection.socket.emit('error', 'Something went wrong');
+ } else {
+ connection.socket.emit('data', '220 smtp.gmail.com ESMTP');
+ }
+ });
+ return connection as any;
+ });
+
+ return { connection, createConnectionSpy };
+}

packages/mail-client/src/mail-client.ts

@@ -3,10 +3,12 @@ import { resolveDestination } from '@sap-cloud-sdk/connectivity/internal';
 import { createLogger } from '@sap-cloud-sdk/util';
 import nodemailer from 'nodemailer';
 import { SocksClient } from 'socks';
+import retry from 'async-retry';
 import {
  customAuthRequestHandler,
  customAuthResponseHandler
 } from './socket-proxy';
+import type { Socket } from 'node:net';
 import type { SentMessageInfo, Transporter } from 'nodemailer';
 import type { SocksClientOptions, SocksProxy } from 'socks';
 // eslint-disable-next-line import/no-internal-modules
@@ -15,8 +17,7 @@ import type {
  MailClientOptions,
  MailConfig,
  MailDestination,
- MailResponse,
- SocksSocket
+ MailResponse
 } from './mail-client-types';
 import type {
  Destination,
@@ -129,9 +130,7 @@ export function buildSocksProxy(mailDestination: MailDestination): SocksProxy {
  };
 }
 
-async function createSocket(
- mailDestination: MailDestination
-): Promise<SocksSocket> {
+async function createSocket(mailDestination: MailDestination): Promise<Socket> {
  const connectionOptions: SocksClientOptions = {
  proxy: buildSocksProxy(mailDestination),
  command: 'connect',
@@ -140,20 +139,57 @@ async function createSocket(
  port: mailDestination.port!
  }
  };
- const socketConnection =
- await SocksClient.createConnection(connectionOptions);
-
- const socksSocket = socketConnection.socket as SocksSocket;
- // Setting `_readableListening` to true in the next line makes the socket readable.
- // Otherwise nodemailer is not able to receive the SMTP greeting message and send emails.
- socksSocket._readableState.readableListening = true;
- return socksSocket;
+ const { socket } = await SocksClient.createConnection(connectionOptions);
+
+ // Workaround for incorrect order of events in nodemailer https://github.com/nodemailer/nodemailer/issues/1684
+ await resendGreetingUntilReceived(socket);
+
+ return socket;
+}
+
+function retrieveGreeting(socket: Socket): Promise<Buffer> {
+ logger.debug('Waiting for SMTP greeting message...');
+ return new Promise((resolve, reject) => {
+ const onData = data => {
+ logger.debug(`Data received from mail socket: ${data?.toString()}`);
+ if (data?.toString().startsWith('220')) {
+ logger.debug('Removing mail socket listeners...');
+ socket.removeListener('data', onData);
+ socket.removeListener('error', onError);
+ resolve(data);
+ }
+ };
+
+ const onError = err => {
+ reject(new Error(err));
+ };
+
+ socket.on('data', onData);
+ socket.on('error', onError);
+ });
+}
+
+async function resendGreetingUntilReceived(socket: Socket): Promise<void> {
+ const greeting = await retrieveGreeting(socket);
+
+ await retry(
+ () => {
+ // resend the greeting message until a listener is attached
+ // note: this is dangerous because there could be another listener that is not the mailer
+ if (!socket.emit('data', greeting)) {
+ throw new Error(
+ 'Failed to re-emit greeting message. No data listener found.'
+ );
+ }
+ },
+ { maxRetryTime: 5000 }
+ );
 }
 
 function createTransport(
  mailDestination: MailDestination,
  mailClientOptions?: MailClientOptions,
- socket?: SocksSocket
+ socket?: Socket
 ): Transporter<SentMessageInfo> {
  const baseOptions: Options = {
  pool: true,
@@ -239,7 +275,7 @@ async function sendMailWithNodemailer<T extends MailConfig>(
  mailConfigs: T[],
  mailClientOptions?: MailClientOptions
 ): Promise<MailResponse[]> {
- let socket: SocksSocket | undefined;
+ let socket: Socket | undefined;
  if (mailDestination.proxyType === 'OnPremise') {
  socket = await createSocket(mailDestination);
  }
@@ -271,10 +307,7 @@ async function sendMailWithNodemailer<T extends MailConfig>(
  return response;
 }
 
-function teardown(
- transport: Transporter<SentMessageInfo>,
- socket?: SocksSocket
-) {
+function teardown(transport: Transporter<SentMessageInfo>, socket?: Socket) {
  transport.close();
  logger.debug('SMTP transport connection closed.');
  if (socket) {