Mailtrain-org / mailtrain

Self hosted newsletter app
GNU General Public License v3.0
5.47k stars 691 forks source link

public url on context instead of subdomain breaks embedded links for statistics #1334

Closed konstantin-shatalov closed 4 months ago

konstantin-shatalov commented 1 year ago

Hello,

We have configured public url for mailtrain to be http://host.com/mailpub/. I realize recommended is to use subdomain but we have some restrictions that make it impossible, so we use a context.

Everything seems to work great with exception of tracking links being embedded into email. I see that a link that is being generate is: http://host.com/links/zFFkGjeOmH/juUswIDEhG/zjzUAABahB expected is: http://host.com/mailpub/links/zFFkGjeOmH/juUswIDEhG/zjzUAABahB Notice that mailpub is gone.

After digging in code I think I see the issue in the following file server\models\links.js const imgUrl = getPublicUrl('/links/${campaign.cid}/${list.cid}/${subscription.cid}'); If code is update to (relative path) I believe it will work as expected const imgUrl = getPublicUrl('links/${campaign.cid}/${list.cid}/${subscription.cid}');

Does this sound like a reasonable change? Embedded/attached files are working as expected (preserving mailpub context) so I assume this should work in similar fashion.

teloniusz commented 7 months ago

I've actually encountered this myself. And fixed.

The issue happens because Javascript's URL(path, baseUrl) constructor overrides path in baseUrl if the latter is present. My solution is to check the path in baseUrl, then add it to path in the argument.

Proposed patch:

diff --git a/server/lib/urls.js b/server/lib/urls.js
index 4e6f7077..f06ea4bc 100644
--- a/server/lib/urls.js
+++ b/server/lib/urls.js
@@ -18,12 +18,13 @@ function getPublicUrlBase() {
 }

 function _getUrl(urlBase, path, opts) {
-    const url = new URL(path || '', urlBase);
+    const baseUrl = new URL(urlBase);
+    const urlPath = baseUrl.pathname.replace(/\/$/, '') + '/' + (path || '').replace(/^\/+/, '');
+    const url = new URL(urlPath, urlBase);

     if (opts && opts.locale) {
         url.searchParams.append('locale', getLangCodeFromExpressLocale(opts.locale));
     }
-
     return url.toString();
 }
talheim-it commented 4 months ago

We are going to start with the development and testing of mailtrain v3 in the next weeks.

You are welcome to help us with the testing as soon as the first release candidate is available.