Re: [PR] merge DecodeShortURLs and Redirectors plugins [sp amassassin]

fkoyer commented on PR #36:
URL: https://github.com/apache/spamassassin/pull/36#issuecomment-5208760824

   The merge looks good but you're still keeping track of shorteners separately from redirectors via the `kind` flag. There already seems to be some confusion on the dev list over what the difference is, and my opinion is that they're really the same thing. I don't think it matters if the redirected URL is shorter than the original URL. They work identically. 
   
   I suggest dropping the `kind` flag and making url_shortener/url_shortener_get pure aliases of url_redirector/url_redirector_get, removing them completely in some future version.
   
   The same for the eval rules: short_url() can just be an alias of redir_url() etc. The former to be removed in some future version. Most of these flags are being set identically anyway:
   
         $pms->{redir_url_loop} = 1;
         $pms->{short_url_loop} = 1;
         
   Might as well set the flag once and have both rules read it. 
   
   Then we just need one cache and one set of budget caps (max_redir_urls, etc.) and no more confusion over "which list does X belong in". 
   
   Having one cache is a big win because currently we have two database handles, 8 statement handles, two autoclean blocks, and two SQL tables with nearly identical schemas.
   
   The only other snag is that DecodeShortURLs historically has been stripping the query string from URLs before fetching. Redirectors does not. The purpose of that is unclear to me. However, it's only safe to strip query strings if we're sure the query string doesn't carry any information about the target URL. That requires knowing the internal workings of every shortening service. The result is that we're fetching URLs that don't exist in the email. So we can't be sure we're getting the same response that the user would get. I suggest NOT stripping query parameters from any URLs. 
   
   It's safe (and appropriate) to strip fragments (i.e. the part after a #) because fragments are meant to be processed client-side per RFC 3986. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]