Security issue

[ehsanrasta]

This package has a serious security issue In mail tracker controller methods getN and getL use query string to get url that user should redirect to, value of url should be get from the database to prevent manipulation by attacker

[mirkos93]

@ehsanrasta From my observation, it appears that not all the URLs included in each email are being recorded in the database; only those that are clicked by users are currently captured. I believe a potential solution could involve invoking the URL extrapolation function from the HTML of the email (which is already utilized in the injectLinkTracker function). When a user clicks on a link (handled by the linkClicked function in MailTrackerController), the function should, at a minimum, verify that the domain of the clicked link is among the domains listed in the body of the email.

If the company, where I am testing this module, approves its implementation and if @jdavidbakr concurs, I intend to submit a pull request incorporating these modifications.

[jdavidbakr]

I'm happy with @mirkos93's suggestion, a PR would be appreciated.

[zajinx]

proposed fix: in MailTrackerController, update protected function linkClicked($url, $hash)

after line 65 (the if (!$url) check) add the following block of code, it checks if the url sent matches what the app has set, if it doesn't it throws an error and doesn't move forward with the redirect, no need to store the url in the database

$app_url = config('app.url');
$normalizedUrl = rtrim($url, '/');
$normalizedBaseUrl = rtrim($app_url, '/');
$check_url=false;
if (strpos($normalizedUrl, $normalizedBaseUrl) === 0) {
    // Ensure that after the base URL, the URL either ends or continues with a '/'
    $remaining = substr($normalizedUrl, strlen($normalizedBaseUrl));
    if (empty($remaining) || $remaining[0] === '/') {
        $check_url=true;
    }
}
if(!$check_url){
    throw new BadUrlLink('Mail hash: '.$hash.', URL: '.$url);
}

[jdavidbakr]

PR would be appreciated, this is something I'd like to fix but personally just haven't had time to implement it

[mirkos93]

@jdavidbakr I created a pull request with the fix I implemented for the project at the company I work for. It's been a long time since I've done pull requests, so please forgive any mistakes.

@zajinx I don't agree much with your proposal: if an email contains an external URL (for example, a URL to a Social Network), this would automatically return an exception, even if the URL is valid.

[jdavidbakr]

I think the best solution would actually be to have a table of links that are parsed from the outgoing emails, with an indexed hash (Str::ulid() probably) that is used in the email text replacement. Then it just looks up the reference in the table. The downside to this is 1) this table could get very large for busy sites and 2) this would break any links generated in previous versions.

[zajinx]

@mirkos93 i understand what you are saying but for my use case that was enough, i am using this package to track if the users have access links within the same app, not to track social media and external clicks.

If you want to do more you could look at MailTracker.php line 209 , inject_link_callback or line 196 injectLinkTracker, that's where the package is checking the body of the mail and inserts the tracking link, i think here would be a place to hook into what are the allowed urls that you need to save the the database.

[bilogic]

Reading from the DB requires additional code and work (need to save when sending, and load when opened/clicked) and has a performance hit at a certain scale.

I would go for something simpler e.g. signing the URLs will prevent unwanted URL manipulation