{"comment":{"created_at":"2011-10-09T16:02:10Z","body":"authentication tests added and the digest authentication fix cherry-picked","updated_at":"2011-10-09T16:02:10Z","url":"https://api.github.com/repos/haskell/HTTP/issues/comments/2337060","id":2337060,"user":{"avatar_url":"https://secure.gravatar.com/avatar/8821a5b7cc43976f9466bd36e9ddae2c?d=https://a248.e.akamai.net/assets.github.com%2Fimages%2Fgravatars%2Fgravatar-140.png","url":"https://api.github.com/users/hsenag","id":451141,"login":"hsenag"}},"action":"created","issue":{"number":5,"created_at":"2011-08-13T08:51:41Z","pull_request":{"diff_url":"https://github.com/haskell/HTTP/pull/5.diff","patch_url":"https://github.com/haskell/HTTP/pull/5.patch","html_url":"https://github.com/haskell/HTTP/pull/5"},"body":"These patches were submitted by Duncan Coutts by email. I've turned them into a pull request for tracking purposes.\r\n\r\nAbout the first two:\r\n\r\nThe first is to fix the support for HTTP digest authentication. The code\r\nseems to do everything right except that it misformats the md5 digests.\r\n(This is slightly odd because it cannot ever have worked like this.\r\nPresumably someone tested it previously and then the md5 formatting got\r\nbroken later.)\r\n\r\nCredit for this one goes to Matthew Gruen. We've tested this new code\r\nagainst happstack, but not against e.g. apache (though we do know the\r\nhappstack server-side digest code works with various standard web\r\nbrowsers).\r\n\r\nWe will be relying on this first fix in future so that cabal-install\r\n(and all other Haskell clients like the mirroring client) can\r\nauthenticate with the hackage-server.\r\n\r\n\r\nThe second fix is for the connection pool logic in the Browser\r\nmodule/monad.\r\n\r\nThe browser monad keeps its pool of open connections so that it can\r\nreuse connections rather than creating new ones. Obviously it is\r\nimportant that when it tries to reuse a connection, that it picks a\r\ncorrect one.\r\n\r\nCurrently the connections in the pool are only identified by the\r\nhostname in the URL, not the combinator of hostname and port number.\r\nThis means that if you have open connections for say, localhost and\r\nlocalhost:8080 then when connections are reused it'll pick one of these\r\narbitrarily which can obviously lead to talking to the wrong server and\r\nfailure.\r\n\r\nInternally the HandleStream records the host name and isTCPConnectedTo\r\nlooks at that.\r\n\r\n[ Generally the connection pool logic is a bit odd, instead of filtering\r\nby isTCPConnectedTo, I'd have thought it more sensible to keep a finite\r\nmap from (host, port) to connection. isTCPConnectedTo also checks (in an\r\nodd way) that the connection is still open. If it turns out it has been\r\nclosed, the connection is not removed from the pool, rather a new\r\nconnection is opened instead and the old connections eventually makes\r\nits way to the back of the connection list, which means in the mean time\r\nwe could well end up expiring and closing perfectly good connections. I\r\nrealise of course you didn't write this mess  ]\r\n\r\nI've fixed it by instead of relying on the hostname recorded inside the\r\nHandleStream via isTCPConnectedTo, keeping the hostname and port number\r\nin the connection pool itself in the Browser. So then when picking a\r\nconnection we filter by the hostname and port number and finally check\r\nif the connection is still connected.\r\n\r\n-  conn <- ioAction $ filterM (\\c -> c `isTCPConnectedTo` uriAuthToString hst) pool\r\n+  conn <- ioAction $ filterM isTCPConnected\r\n+                             [ c | (hostname,portnum, c) <- pool\r\n+                                 , hostname == uriRegName hst\r\n+                                 , portnum  == uriAuthPort Nothing hst ]\r\n\r\nTo do that I've introduced variants of isTCPConnectedTo and\r\nisConnectedTo that just test if they're connected and don't take any\r\nhostname as a parameter. As far as I can see both isTCPConnectedTo and\r\nisConnectedTo are pretty pointless functions so I've also taken the\r\nliberty of deprecating them (the module they're in is mostly internal\r\nanyway).\r\n\r\nI've tested this fix with my hackage-mirror client, mirroring between\r\nlocalhost and localhost:8080.\r\n\r\nAnd about the third:\r\n\r\nOh, and another one!\r\n\r\nThis is a fix to Network/Browser.hs to return 304 \"not modified\"\r\nresponses directly and not to treat them as a redirection (since it's\r\nnot a redirection).\r\n\r\nThe 304 code is returned by servers in response to conditional GET/PUT\r\nrequests (e.g. GET met this resource, but only if it changed more\r\nrecently than time t), to say that the resource has not changed. This\r\nconditional stuff is needed to validate client-side caches. I'm now\r\nusing this in the hackage-mirror and it would be nice to have it in\r\ncabal-install too, as it'd make cabal update a bit cheaper.\r\n\r\n\r\nYou may notice that this partially reverts a patch from aslatter from\r\nlast year. He changed:\r\n\r\n-      (3,0,x) | x `elem` [2,3,1,7]  ->  do\r\n+      (3,0,x) | x /= 5  ->  do\r\n\r\nand I've changed it back\r\n\r\n-      (3,0,x) | x /= 5  ->  do\r\n+      (3,0,x) | x `elem` [2,3,1,7]  ->  do\r\n\r\nThis isn't really the important part of the patch, the important bit is:\r\n\r\n-      (3,_,_) -> redirect uri rsp\r\n       _       -> return (Right (uri,rsp))\r\n\r\n\r\nhttp://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_Redirection\r\n\r\nWhat is going on is that there are four \"30x\" response codes that should\r\nbe treated as redirect: 301, 302, 303 and 307.\r\n\r\nThere are a few other \"30x\" codes which are not redirects, or not\r\nstandard redirects: 300, 304, 305 and 306. The current code handles 305\r\nexplicitly. The current code treats 300 as a redirect too, using the\r\ncatch-all case:\r\n\r\n      (3,_,_) -> redirect uri rsp\r\n\r\nWhile the HTTP spec says that clients MAY treat 300 as a redirect,\r\nthat's only if the server provides a location, which is optional for\r\n300. The current redirect code always expects a location, so it's been\r\nhandling 300 wrong anyway (300 is almost never used in practice). So I\r\ndecided that instead of preserving the current wrong behaviour for 300,\r\nit's better to just return it to the client.\r\n\r\nFinally, that leaves 304 and 306.\r\n      * 306 is explicitly not used (says so in the spec).\r\n      * 304 is not a redirect, it's the answer to conditional GET/PUT\r\n        when the resource has not changed. So again it must go to the\r\n        caller, and not be treated as a redirect (it has no location\r\n        header so the current code fails)\r\n\r\nThe current code treats all other unknown 3xx codes as redirects and\r\nlooks for a location header. I think this is not a safe thing to do, we\r\ncannot predict the behaviour of any newly allocated 3xx codes, so we\r\nshould just return them to the caller.\r\n\r\nThis simplifies the logic because it means we only match the ones we\r\nexplicitly handle, and return the others to the caller. Because we no\r\nlonger have the catch-all redirect case, then the 'redirect' function is\r\ndead code.\r\n\r\nClear as mud?\r\n","comments":8,"title":"patches from Duncan","updated_at":"2011-10-09T16:02:10Z","url":"https://api.github.com/repos/haskell/HTTP/issues/5","id":1399435,"assignee":null,"milestone":null,"closed_at":"2011-10-09T16:02:10Z","user":{"avatar_url":"https://secure.gravatar.com/avatar/8821a5b7cc43976f9466bd36e9ddae2c?d=https://a248.e.akamai.net/assets.github.com%2Fimages%2Fgravatars%2Fgravatar-140.png","url":"https://api.github.com/users/hsenag","id":451141,"login":"hsenag"},"html_url":"https://github.com/haskell/HTTP/issues/5","labels":[],"state":"closed"},"legacy":{"issue_id":1399435,"comment_id":2337060}}