Domains in Context

[bretto36]

Hi There

I have used this package for many years, since before the log content strategy was introduced. We store our content data in S3 using our own mechanism. It would take quite a while

We've begun upgrading to Laravel 12 and have found that the Domains in Context functionality doesn't play nice with our storage mechanism as well as people who have the config('log-content') === false.

I'm happy to do the PR, just want to know which way you'd prefer me to handle this.

I could introduce a new config option to disable domains in context or something else if you have a better idea?

There is already a PR #258 but i don't think that will actually work, because domains in context will be an empty array and will trigger the if check to redirect back to the default domain and not go to where it needs to go.

[jdavidbakr]

I think the only safe answer would be to disable link tracking - because if we don't have a way to validate that the url we're tracking is indeed what we sent, we have created a way for malicious links to be injected into our website.

[bretto36]

@jdavidbakr So the config('mail-tracker.log-content') being false currently always redirects to the default route is that a bug, or is that config option now defunct?

I might override my SentEmail model to generate a list of valid domains based on known possibles...

[jdavidbakr]

That's a bug, I think.

What if we used Cipher::encryptString() for the tracking code instead of the base 64 hash? That way it couldn't be tampered with. If someone wants to make a PR with that, let's change the query parameter so that we stay backwards compatible, but then we don't have to validate the new parameter's links.

[bretto36]

Are you suggesting encrypting the $url value in the mailTracker_n route, or the hash?

I don't think encrypting the hash would necessarily stop anything, as all someone would have to do is find a valid value by receiving an email from the system then change the link value.

Could we use a ValidateSignature middleware instead on the mailTracker_n route? We'd need to handle a bit of overlap time somehow. As if we turned this on all of a sudden it would mean any old links are now useless

URL::signedRoute - this doesn't expire and validates against the parameters in the string as well.

protected function inject_link_callback($matches)
    {
        if (empty($matches[2])) {
            $url = app()->make('url')->to('/');
        } else {
            $url = str_replace('&', '&', $matches[2]);
        }

        return $matches[1].URL::signedRoute(
            'mailTracker_n',
            [
                'l' => $url,
                'h' => $this->hash,
            ]
        );
    }

[bretto36]

@jdavidbakr any thoughts on this?

[bretto36]

#265 i think could solve this once and for all if we can agree on a "migration" strategy

[jdavidbakr]

I think we change the query parameter for the signed route, and continue to check the database for the existing route. Then we can deprecate the existing route but let it remain until a future version.

[bretto36]

@jdavidbakr Do you think it's worth only checking the database if the route is NOT signed? There is no point doing both. It just means it's hitting the storage everytime someone clicks a link on an email.

Would it be better to introduce a new event for valid links as a fallback. We could move the Domains in Context to an event listener that marks the link as valid/acceptable.

Then people could add their own event listener to do their own logic, compare to a list of their own domains for example?

[jdavidbakr]

Yes, the event should only happen on the legacy route; no need with a signed route. The reason we're checking the database is to prevent malicious links from being injected ... but with the signed route there's no attack vector.

I do like the idea introducing an event.

[bretto36]

Awesome. I'll add to my PR