github
Ruby NetHttpRequest improvements
Description of the issue
Hi all,
I'm building on the Ruby language's Http::Client::Request class, particularly NetHttpRequest. This is going well, except NetHttpRequest appears to be somewhat of an outlier compared to other client requests. There are two things: 1) its class fields are private instead of public, and 2) it only has a requestNode field and is lacking connectionNode.
For example:
https://github.com/github/codeql/blob/2dc88d87ae0c9e04f271992d0a6c8ad383d054dd/ruby/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll#L21-L24
https://github.com/github/codeql/blob/2dc88d87ae0c9e04f271992d0a6c8ad383d054dd/ruby/ql/lib/codeql/ruby/frameworks/http_clients/Faraday.qll#L25-L28
https://github.com/github/codeql/blob/2dc88d87ae0c9e04f271992d0a6c8ad383d054dd/ruby/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll#L19-L21
https://github.com/github/codeql/blob/2dc88d87ae0c9e04f271992d0a6c8ad383d054dd/ruby/ql/lib/codeql/ruby/frameworks/http_clients/Httparty.qll#L26-L27
https://github.com/github/codeql/blob/2dc88d87ae0c9e04f271992d0a6c8ad383d054dd/ruby/ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll#L26-L29
So my question is, are NetHttpRequest class fields private for a reason, and if not would it be reasonable to make them public? And if so, would it also be reasonable to add a connectionNode field similar to FaradayHttpRequest, RestClientHttpRequest, and ExconHttpRequest?
I'm happy to open a PR with the changes myself - I just wanted to open an issue to track it first and check that there's not a reason for this discrepancy.
Those are very good questions. The inconsistency between private/public is unfortunate. As you point out some of the classes lack the connection fields. Apart from that some fields with the same names have have different types for the various classes. It feels like those fields are more of an implementation detail, and perhaps they should all have been private. On the other hand all those classes represent http requests, so the code can also be refactored by pulling those fields up into a common super class.
What are you trying to achieve? Why do you need access to the fields?
On the other hand all those classes represent http requests, so the code can also be refactored by pulling those fields up into a common super class.
This sounds like a good idea to me 👍. I think at least requestNode could be pulled up into the super class because, after all, all these classes do represent an HTTP request. So it'd be natural to include the node of the request in the public API of the class.
The connectionNode nodes are a commonality among some, but not all, HTTP request classes. Oftentimes these HTTP request frameworks have a way to create a connection pool and use that to make requests rather than create a new connection for every request. I'm not sure the best way to represent this in QL, but some kind of optional (i.e. present or null/nil//etc) class field makes sense to me.
What are you trying to achieve? Why do you need access to the fields?
I'm building on these HTTP request classes to analyze the HTTP headers used in these various frameworks. For example, say we have the following code:
require 'faraday'
# requestNode
resp1 = Faraday.get('http://example.com', nil, {'somekey' => 'somevalue'})
# connectionNode
conn = Faraday.new(
url: 'http://example.com',
headers: {'somekey' => 'somevalue'}
)
resp2 = conn.get('/')
The easiest way to find these request and connection calls is to use something like FaradayHttpRequest. And then I can write QL code to use requestNode and connectionNode to analyze the call's HTTP headers. I came up with this by reading through the relevant Ruby queries and libraries, so let me know if there's a better way to accomplish this objective. But, overall, creating a custom class that extends FaradayHttpRequest has been a great, succinct way to accomplish this.
Would making these fields in NetHttpRequest public be an acceptable solution here?
