Ruby: Minor change of SSRF concept #8524
Conversation
This is a port of the same change in Python from f8fc583 The description of that commit was: > I think `getUrl` is a bit too misleading, since from the name, I would > only ever expect ONE result for one request being made. > > `getAUrlPart` captures that there could be multiple results, and that > they might not constitute a whole URl. > > Which is the same naming I used when I tried to model this a long time ago > https://github.com/github/codeql/blob/a80860cdc6b06b363b0d0919600ab383a470b449/python/ql/lib/semmle/python/web/Http.qll#L102-L111
|
The only failing check is |
|
We have mostly been attempting to follow the deprecation policy, since we already have query-writers working in other repos. This change looks like a simple rename, so I feel like it would be a good idea to keep the old name with the |
Notice that we still don't fully keep our standard deprecation support, since the new `getAUrlPart` is still abstract, and therefore will cause compile errors if not implemented.
So an "old" query using the deprecated predicate, will still find the same results, even when the modeling has been updated.
|
I have fixed things up, so uses of the concept follows deprecation policy, but definition of models are required to change their override to the new name. If that's not good enough for "mostly" follow the deprecation policy, I can do further changes 😊 |
Co-authored-by: Nick Rolfe <[email protected]>
| @@ -484,6 +484,14 @@ module HTTP { | |||
| /** Gets a node which returns the body of the response */ | |||
| DataFlow::Node getResponseBody() { result = super.getResponseBody() } | |||
|
|
|||
| /** | |||
| * DEPRECATED: Use `getAUrlPart` instead. | |||
There was a problem hiding this comment.
This isn't entirely related to this PR, but I was wondering what you thought about adding (2022-03-24) or similar to this comment so it's clear when the predicate was deprecated. I know we can estimate this using git blame, but that can become inaccurate if code gets moved around.
Even if it's a good idea, it might be something better added to all the deprecated predicates as a separate PR. I'm mostly just interested in thoughts.
There was a problem hiding this comment.
I think it's an excellent idea 👍 (although git blame should have our back)
When I added the Python query in #7420, I did some minor changes. This is only part of those changes, I will do another PR for the changes I made to
disablesCertificateValidationonce I have had time to change our query. (this PR had been laying around for some time, so will do it now so I wont forget about it).I'm of the understanding that we can just make any breaking changes to the API as we want, since Ruby is still in beta, so that's why I haven't done any kind of deprecation 😉
Notice that the change-note would bump the major version of the library pack. If you don't want this, let me know, and I can change things 😊