Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Insecure Mail Transport #55

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

fritz-fritz
Copy link
Contributor

Changed the transport builder to be more flexible.

Defaults to secure = true if mail.secure is not specified
Does not require auth to be specified if not needed

This fixes the issue revealed in #54

Changed the transport builder to be more flexible.

> Defaults to secure = true if mail.secure is not specified
> Does not require auth to be specified if not needed
@ItzNotABug
Copy link
Owner

Hey @fritz-fritz 👋,
Thanks for creating this PR!

I'm away for a while so won't be able to test this behaviour. But the changes are minimal so looks fine to me atm.

However the auth fields are marked required if I recall correctly in the view files. We'd need to change those as well.

@fritz-fritz
Copy link
Contributor Author

Yes it is hardcoded there as well. There's actually several ways I'd suggest reworking the settings view to allow for a smoother user experience and more flexibility. But my goal with this commit was simply to allow for advanced configuration directly from the configuration file.

If you'd like to hold this and rework the frontend too I'd be open to that.

Use case is mainly for when the mail server is on localhost and
no auth or security is required. This updates the form so that
it can be configured successfully from the browser instead of
only from the configuration file.
@fritz-fritz
Copy link
Contributor Author

I went ahead and modified the view to allow setting these values via the browser.

I also fixed what seems to have been a bug where Reply-To was marked as optional but still was a required form field.

@ItzNotABug
Copy link
Owner

ItzNotABug commented May 31, 2024

Hey @fritz-fritz,
I'm a bit sceptical for a change like this.

Probable Issues -
Current logic allows adding multiple email configurations, this can cause unnecessary and unwanted problems if there are multiple sources that support TLS.

Assume this scenario -

  1. A few hundred subscribers exists
  2. There's a bunch of email credentials with auth/pass and some without those
  3. Newsletters may be sent but others might end up in the spam due to sendmail, this isn't something I'd want
  4. There's a good possibility later that the domain/ip is marked as a spammy sender, worse - marked as a blacklist
  5. Also - IMO Non secure connection isn't something that should usually be used on production

Solution -
Ghosler is pretty much stable in terms of usage and doesn't require that many changes, bug fixes overall, so you can host a fork with the above changes if you'd like. You wouldn't need to use Ghosler CLI for this. A simple -

pm2 start app.js --no-autorestart --name ghosler

Alternatively, if you need the features from the cli, download it, change the base url here, install it globally -

npm install -g .

and you should be good to go!

@fritz-fritz
Copy link
Contributor Author

Hey @ItzNotABug,

Totally understand the initial skepticism.

Regarding the multiple email credentials, you should check out the code in how I implemented this. It is per mail config. So that should be a non-issue.

Regarding the use of non-secure and/or non-authenticating connections. I absolutely agree that security is important. However, it is totally normal to be talking to a mail server over localhost that doesn't require either and would introduce latency and overhead to do so. As long as the mail server is local there is no issue. Ultimately my view is that this is a valid configuration to implement and concerns regarding spam and security fail on the mail server not Ghosler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants