Handle argument errors when http request raises argument error for headers having CR/LF characters

[BeckaL]

Faraday / ruby cannot parse headers which contain CR/LF characters. As this issue appears to exist deep down in the faraday / ruby stack, we should handle the error rather than letting the application alert. This change attempts to handle only those argument errors that roughly fuzzy match the exception string 'header field value cannot include CR/LF' (with a bit of fuzziness in case the exact message changes) but still raise for other exceptions.

Trello

[kevindew]

Nice work putting this together, it looks like it'll do the trick. I've made some suggestions to try make this a bit more of a generic implementation. Happy to chat if they don't make sense.

I'll put on my todo list to raise an issue with Ruby net-http to see if it can be fixed at the library.

[BeckaL]

@kevindew thanks for the helpful comments! Should all be addressed now. And thanks for putting following up on this on your to-do list 😄

[kevindew]

Thanks @BeckaL that looks good, thanks for addressing my comments.

I managed to fail to send a comment somehow too, sorry - it was probably my longest comment as well 🤦

I think we should have a more generic exception than CRLFArgumentError as I think that is too specific for the users of this link checker as it's very technical. Since we would have expected that this error would be caught with the generic Faraday error I reckon we can use the same messaging as that.

I also wondered if rather than needing to create a new error exception we could repurpose the FaradayError one. I think we've created that as basically the generic failure error. I wonder if we could just call it something like CommunicationError and use it rather than creating a new user facing error for this scenario.

Thanks again, and sorry about the late comment.

[BeckaL]

Yup good point @kevindew I was struggling to work out good messaging and naming, will have a look 🙂

[BeckaL]

Hey @kevindew - I've pushed my suggestion for this, a lot of it from what you suggested. I have:

• Removed CRLFArgumentError
• Renamed FaradayError to HttpCommunicationError (as this one is just in code rather than manifested externally, think it makes it a bit more specific for developers than just CommunicationError);
• Changed the error summary in the case of hitting the CR/LF case to be the generic 'page unavailable' error
• Still kept an error message that's specific to this case.

My reasoning for this last one is that it's a bit of a balancing act - the message is definitely a bit too technical for the layperson. However, I'm thinking about guarding against a more generic 'page couldn't load' error coming to someone in the report, where they'd clearly be able to see that the page does load in their browser, so worried about people losing faith in the link checker with this more generic message. I'm also thinking that it'll be easier for a developer to quickly diagnose if the error message is specific, so thinking about the diagnosis effort for future zendesk tickets. I think having the balance of a non-techy summary and a slightly more techy message might be the right one to strike in this particular case. Have attempted to word it in the least technical way I can, but I guess as soon as you have to talk about headers, it's tricky. Also I'm now too ingrained in software to know what normal people would say instead of 'parse' 😂

What do you reckon?

[kevindew]

Thanks @BeckaL looks great.

Your argument makes sense, I can see where you're coming from. My perspective is that, were Ruby's net-http returning a different exception that Faraday was catching, we would not be putting any additional messaging for this and they'd get the generic pairing of "Page unavailable", "This page is failing to load." and our reasons for not using that error here probably also apply to a generic faraday error.

I think mentioning anything about headers is going too technical compared to others, if we were to do it I'd probably go down an ambiguous "technical misconfigured". I'm not worried about the debugging situation as it only distinguishes it from one of multiple sources of the above generic error.

We could change the generic error to be something like "This page has a technical problem"

[BeckaL]

OK yep I follow your reasoning, that makes sense. Updated the generic error as you suggested to be 'This page has a technical problem'.

[kevindew]

Great stuff

[kevindew]

I've opened: ruby/net-http#72 to try resolve this upstream

Annoyingly our problem link seems to have fixed it's header now and I've found it very difficult to find another one that produces the problem:

irb(main):002:0> LinkChecker.new("https://redkiteseo.co.uk").call
=> #<LinkChecker::UriChecker::Report:0x00000001474c95f8 @problems=[]>