+notrack not removed if DNS server doesn't return CNAME record

[wankdanker]

I've had various issues with +notrack not being removed from URLs over the past year or so rendering the received messages to be un-viewable or un-clickable.

The original problem I believe was related to connections to the database server being reset due to a misconfiguration that would sometimes cause Postal.tracking_available? && @domain.track_clicks? to be false in lib/postal/message_parser.rb. When that happened, insert_links would not be called and therefore part.gsub!(/(https?)\+notrack\:\/\//) do... would not be executed to replace +notrack.

Recently I inadvertently started routing my tracking domains through Cloudflare's proxy services and when I did that, they would no longer return a CNAME record for my tracking domains (they would only return an A record). So, at some point, postal detected the lack of CNAME as a DNS error and updated the tracking domain to have dns_status=Missing. In that case the message parser would ultimately not call the insert_links function.

Clearly it's very important to have everything set up and running correctly. I'm just wondering if it would be possible to do the +notrack replacement no matter what before a message is sent. I'd rather have messages be sent that will work for the recipient rather than have tracking.

[willpower232]

I believe the code should not be this fragile and this can be fixed by moving the removal of +notrack to its own function to be called regardless of whether tracking is enabled in the parse function.

Would you like to submit a PR? You can also run your modified code to resolve this problem for yourself at least.

[wankdanker]

If I create a working solution I will definitely submit a PR.

[willpower232]

I'm not a ruby expert but I think you can move this whole bit up to the parse function so it is always used

https://github.com/postalhq/postal/blob/5ecc3a5a80476e420fdd226787cb303fa15641c5/lib/postal/message_parser.rb#L119-L122

We use click tracking but haven't made use of notrack (yet) otherwise we would have probably come across this sooner.

[wankdanker]

I'm not a ruby expert either, but I did trace the code best I could and it seems like parse is ultimately not called at all if dns_status is not "OK". I'm assuming that if @domain is false when there is no record returned.

https://github.com/postalhq/postal/blob/5ecc3a5a80476e420fdd226787cb303fa15641c5/lib/postal/message_parser.rb#L6-L16

My other concern is, is it possible that this function is called multiple times? Like if it's held or something? I wouldn't want to remove the +notrack stuff too soon if it might be parsed again later. I'm not sure of the overall flow here.

[willpower232]

The process is a bit back and forth but I don't think it is run repeatedly because of this check here but it would be worth testing a message that cannot be sent (i.e. to example.com or something else that can't receive email so the message is retried).

https://github.com/postalhq/postal/blob/5ecc3a5a80476e420fdd226787cb303fa15641c5/app/jobs/unqueue_message_job.rb#L323-L326

Good spot on that :dns_status, I would say for the purpose of this situation that part of the query should be removed.

[uhlhosting]

Any updates on this, tracking seems to have stopped working after upgrade to version 2.0

[wankdanker]

This is the patch that I have in place on some older version (2595481) before 2.0. I didn't even realize 2.0 was out! I haven't had any tracking issues since having this in place.

diff --git a/lib/postal/message_parser.rb b/lib/postal/message_parser.rb
index 3453914..7230cc3 100644
--- a/lib/postal/message_parser.rb
+++ b/lib/postal/message_parser.rb
@@ -8,7 +8,7 @@ module Postal
       @actioned = false
       @tracked_links = 0
       @tracked_images = 0
-      @domain = @message.server.track_domains.where(:domain => @message.domain, :dns_status => "OK").first
+      @domain = @message.server.track_domains.where(:domain => @message.domain).first
 
       if @domain
         @parsed_output = generate
@@ -88,6 +88,11 @@ module Postal
         part = insert_tracking_image(part)
       end
 
+      part.gsub!(/(https?)\+notrack\:\/\//) do
+        @actioned = true
+        "#{$1}://"
+      end
+
       part
     end
 
@@ -116,11 +121,6 @@ module Postal
         end
       end
 
-      part.gsub!(/(https?)\+notrack\:\/\//) do
-        @actioned = true
-        "#{$1}://"
-      end
-
       part
     end