[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 👍 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user atsci commented on the issue: https://github.com/apache/trafficserver/pull/866 Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/470/ for details. --- If your project is set up for it, you can reply to this email and have your

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user atsci commented on the issue: https://github.com/apache/trafficserver/pull/866 FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/574/ for details. --- If your project is set up for it, you can reply to this email and have your

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/866 @maskit I pushed up a new version that separates escapify_url into pure_escapify_url (which does not attempt to detect the double encoding case) and regular escapify_url (which does). I

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 > are you ok with the change if we make a new version of LogUtils::escapify_url that does not include this change that is used by TSStringPercentEncode? So the change only affects core logic?

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/866 Ok just to be clear. The current PR will change the behavior of TSStringPercentEncode. Strings that included "%20" would have been double encoded and with this change they will not.

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 I'd say it is not sufficient. As a workaround for most cases, it works, and personally I'm OK with it as is. However, I think the behavior of `TSStringPercentEncode` shouldn't be

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user zwoop commented on the issue: https://github.com/apache/trafficserver/pull/866 @maskit Are you saying the patch as is is not good / sufficient? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 ``HttpRequestData::get_string()`` unescapes an URL and it is called from ``UrlMatcher::Match(RequestData *rdata, Result *result)``. I think this is the code Sudheer mentioned.

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user atsci commented on the issue: https://github.com/apache/trafficserver/pull/866 Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/431/ for details. --- If your project is set up for it, you can reply to this email and have your

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user atsci commented on the issue: https://github.com/apache/trafficserver/pull/866 FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/534/ for details. --- If your project is set up for it, you can reply to this email and have your

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/866 Reviewing the bug comments Sudheer says "Yes, the external URLs should be already encoded - however, internally, I see code that decodes the URL strings (e.g. UrlMatcher::Match). So, by the

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user shinrich commented on the issue: https://github.com/apache/trafficserver/pull/866 @maskit, you list some lovely tricky cases. Abort encode on detect seems like a reasonable approach too. Ideally, we just could declare that nothing comes in already encoded, but that

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 I wrote a unit test for CURRENT `escapify_url`. http://pastebin.com/XZ4x8bKg With your change, you would need to change the last expected value in the test cases. --- If your

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user maskit commented on the issue: https://github.com/apache/trafficserver/pull/866 I don't think this is the right approach. With this change, "%%20" will be encoded to "%25%20", right? What if "%%20" was not encoded string? It should be encoded to "%25%2520". Shouldn't

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user atsci commented on the issue: https://github.com/apache/trafficserver/pull/866 FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/533/ for details. --- If your project is set up for it, you can reply to this email and have your

[GitHub] trafficserver issue #866: TS-2237: Fix double encoding of URLs in squid logs...

Github user atsci commented on the issue: https://github.com/apache/trafficserver/pull/866 Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/430/ for details. --- If your project is set up for it, you can reply to this email and have your