Ruby NetHttpRequest improvements

[mschwager]

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:

codeql/ruby/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll

Lines 21 to 24 in 2dc88d8

	class NetHttpRequest extends Http::Client::Request::Range, DataFlow::CallNode {
	private DataFlow::CallNode request;
	private API::Node requestNode;
	private boolean returnsResponseBody;

codeql/ruby/ql/lib/codeql/ruby/frameworks/http_clients/Faraday.qll

Lines 25 to 28 in 2dc88d8

	class FaradayHttpRequest extends Http::Client::Request::Range, DataFlow::CallNode {
	API::Node requestNode;
	API::Node connectionNode;
	DataFlow::Node connectionUse;

codeql/ruby/ql/lib/codeql/ruby/frameworks/http_clients/RestClient.qll

Lines 19 to 21 in 2dc88d8

	class RestClientHttpRequest extends Http::Client::Request::Range, DataFlow::CallNode {
	API::Node requestNode;
	API::Node connectionNode;

codeql/ruby/ql/lib/codeql/ruby/frameworks/http_clients/Httparty.qll

Lines 26 to 27 in 2dc88d8

	class HttpartyRequest extends Http::Client::Request::Range, DataFlow::CallNode {
	API::Node requestNode;

codeql/ruby/ql/lib/codeql/ruby/frameworks/http_clients/Excon.qll

Lines 26 to 29 in 2dc88d8

	class ExconHttpRequest extends Http::Client::Request::Range, DataFlow::CallNode {
	API::Node requestNode;
	API::Node connectionNode;
	DataFlow::Node connectionUse;

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.

[aibaars]

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?

[mschwager]

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.