Ruby: Rails route resolution #7061

hmac · 2021-11-04T16:56:09Z

Add Route classes which model Rails routing information, typically
defined in a routes.rb file. We extract only the most basic
information: HTTP method, path, controller and action. This is enough to
determine whether a given controller method is a route handler, and what
HTTP method it handles, which is useful for, among other things, the URL
redirect query.

This is quite a big PR with some probably-questionable design decisions, but does work correctly at least. I'd appreciate feedback on a) whether this is worth it and b) if there's a
more concise/more performant way to do it.

nickrolfe

This looks like it was a ton of work. I've only got part-way through reviewing this, but it looks great!

nickrolfe · 2021-12-14T17:44:33Z

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

+
+    override string getAction() { result = action }
+
+    override string getHTTPMethod() { result = httpMethod }


I noticed this returning patch for a call like this:

resources :foo, path: "/foo/:foo_id/bar", only: %w(new create edit update destroy), ...

Oh wait, am I mixing up actions and HTTP methods?

PATCH is a valid, if rarely used, HTTP method which acts similarly to PUT, but is intended for partial updates to a resource (whereas PUT is for completely replacing the resource). Rails prefers PATCH for updates because it is a better fit semantically. (https://guides.rubyonrails.org/v6.0/4_0_release_notes.html)

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

ruby/ql/lib/codeql/ruby/security/UrlRedirectCustomizations.qll

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

nickrolfe · 2021-12-21T12:12:00Z

The tests are failing because of a warning about the decapitalize predicate being unused. You also have a number of alerts from QL for QL. It'd be good to fix those.

Otherwise, LGTM.

alexrford

This is really nice - very clearly structured and well commented. Only a handful of minor comments.

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

alexrford · 2022-01-25T18:35:22Z

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

+      result = method.getKeywordArgument("to").(StringlikeLiteral).getValueText()
+      or
+      method.getKeywordArgument("to").(MethodCall).getMethodName() = "redirect" and
+      result = "<redirect>#<redirect>"


Is this a placeholder value?

Took me a while to remember what this is for! It covers cases like

get "/stories" => redirect("/posts")

In this case there's no controller/action combo that we can map GET /stories to, except by trying to resolve /posts to a route and going from there. That seemed like a lot of effort so instead we just return <redirect>#<redirect> for the action string.

Thinking about it again, perhaps we should just return no result in this case?

It's probably okay as it is - by doing this it looks like getAction() will return "<redirect>", so that's a bit more informative than having no result.

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

alexrford · 2022-01-25T18:42:33Z

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

+
+    override string getHTTPMethod() {
+      result = method.getKeywordArgument("via").(StringlikeLiteral).getValueText() or
+      result = method.getKeywordArgument("via").(ArrayLiteral).getElement(_).getValueText()


Wonder if this should have a special case for :all where the result = ["get", "post", "put", "patch", "delete"] (https://guides.rubyonrails.org/routing.html#http-verb-constraints)?

alexrford

@hmac just one last comment, let me know when it's done and I'll approve this PR.

alexrford · 2022-01-26T13:25:41Z

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

+      |
+        via = "all" and result = anyHttpMethod()
+        or
+        via != "all" and result = "via"


Suggested change

via != "all" and result = "via"

via != "all" and result = via

🤦 good catch!

hvitved

A few drive-by comments.

hvitved · 2022-01-26T13:30:04Z

ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll

+  ActionDispatch::Route getARoute() {
+    result.getController() + "_controller" =
+      ActionDispatch::underscore(namespaceDeclaration(controllerClass)) and
+    this.getName() = result.getAction()


This looks like a candidate for a bad join, but I don't know if joining on getName() and getAction() alone can yield a large set in practice. It should be possible to join simultaneously on name and controller, see e.g. #7738 for an example.

I'm still getting my head around this problem and its mitigation. I've replaced this line with a call to isActionControllerMethod(this, result.getAction(), controllerClass) (749dc09), which will hopefully force a join simultaneously on getName(), getAction() and controllerClass. Is that enough to protect against this issue or do we need to do something else?

If you add a new predicate

pragma[nomagic] private predicate isRoute( ActionDispatch::Route r, string name, ActionControllerControllerClass controllerClass ) { r.getController() + "_controller" = ActionDispatch::underscore(namespaceDeclaration(controllerClass)) and name = r.getAction() }

and change the body to

exists(string name | isRoute(result, name, controllerClass) and isActionControllerMethod(this, name, controllerClass) )

then simultaneous joining on name and controllerClass should be ensured.

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll

ruby/ql/lib/codeql/ruby/security/UrlRedirectCustomizations.qll

Add `Route` classes which model Rails routing information, typically defined in a `routes.rb` file. We extract only the most basic information: HTTP method, path, controller and action. This is enough to determine whether a given controller method is a route handler, and what HTTP method it handles, which is useful for, among other things, the URL redirect query.

Handlers for non-GET requests aren't vulnerable to URL redirect attacks, because browsers won't initiate non-GET requests when you click a link. We can use Rails routing information, if present, to filter out any handlers for non-GET requests.

This predicate isn't used.

This shows how the predicate behaves, as well as a case where it goes wrong.

Any classes/predicates not used externally or in tests are now private. Also fix some typos.

This version is much shorter and hopefully performs a bit better.

ActionDispatch modelling now understands that match "/foo", to: "foo#bar", via: :all is equivalent to match "/foo", to: "foo#bar", via: [:get, :post, :put, :patch, :delete]

`x.isStringOrSymbol(result)` is slightly terser than `result = x.getStringOrSymbol()`.

By joining simultaneously on controller class and name.

Make ActionDispatch::Route into a private class ActionDispatch::RouteImpl, defining a new class Route which exposes the necessary public API from RouteImpl. Also rename getHTTPMethod to getHttpMethod.

hvitved · 2022-02-08T08:05:32Z

ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll

  }
 }

+private predicate isRoute(


I suggest adding pragma[nomagic] to ensure that the compiler does not mess up our optimization (e.g. by inlining the predicate)

Fixed the issue with via

hmac added the Ruby label Nov 4, 2021

hmac force-pushed the hmac/actiondispatch branch 3 times, most recently from 4e858e9 to 01b51c4 Compare November 10, 2021 08:55

hmac force-pushed the hmac/actiondispatch branch from 01b51c4 to 2fe011f Compare November 19, 2021 15:25

hmac force-pushed the hmac/actiondispatch branch from 9820a8e to 0fdbe22 Compare December 13, 2021 02:53

hmac marked this pull request as ready for review December 13, 2021 04:02

hmac requested a review from a team as a code owner December 13, 2021 04:02

nickrolfe reviewed Dec 14, 2021

View reviewed changes

nickrolfe reviewed Dec 15, 2021

View reviewed changes

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll Show resolved Hide resolved

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll Outdated Show resolved Hide resolved

ruby/ql/lib/codeql/ruby/security/UrlRedirectCustomizations.qll Outdated Show resolved Hide resolved

hmac force-pushed the hmac/actiondispatch branch 2 times, most recently from 2989cab to 5e9b250 Compare December 15, 2021 22:00

nickrolfe reviewed Dec 16, 2021

View reviewed changes

ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll Outdated Show resolved Hide resolved

hmac force-pushed the hmac/actiondispatch branch from 183a1bb to aa96717 Compare December 21, 2021 19:18

hmac force-pushed the hmac/actiondispatch branch from aa96717 to 0fd3e46 Compare January 5, 2022 23:57

alexrford suggested changes Jan 25, 2022

View reviewed changes

hmac force-pushed the hmac/actiondispatch branch from 0fd3e46 to 7369846 Compare January 26, 2022 02:59

alexrford previously requested changes Jan 26, 2022

View reviewed changes

hvitved requested changes Jan 26, 2022

View reviewed changes

hmac and others added 10 commits February 2, 2022 16:26

Ruby: Document getACapture

751d8a7

Ruby: Document ActionDispatch modelling

1766916

Add missing documentation to ActionDispatch::Route

ad71fdb

Remove ActionDispatch::capitalize

9c67869

This predicate isn't used.

Add a test for ActionDispatch::underscore

fa28e55

This shows how the predicate behaves, as well as a case where it goes wrong.

Ruby: Make most of ActionDispatch private

dead7a8

Any classes/predicates not used externally or in tests are now private. Also fix some typos.

Ruby: remove unused predicate

eff2136

Ruby: Rewrite ActionDispatch::underscore

3786fbf

This version is much shorter and hopefully performs a bit better.

hmac added 11 commits February 2, 2022 16:26

Ruby: remove unused predicate

e975f92

Ruby: QL style fixes

4172871

getValueText -> getConstantValue

8bdc05d

Handle via: :all in Rails routes

47823b5

ActionDispatch modelling now understands that match "/foo", to: "foo#bar", via: :all is equivalent to match "/foo", to: "foo#bar", via: [:get, :post, :put, :patch, :delete]

Minor cleanup to ActionDispatch modelling

856c3d3

`x.isStringOrSymbol(result)` is slightly terser than `result = x.getStringOrSymbol()`.

Ruby Fix handling of via: in ActionDispatch

a38bc9f

Ruby: Attempt to mitigate potential bad join

749dc09

By joining simultaneously on controller class and name.

Ruby: Cache ActionDispatch IPA types

8f53801

Ruby: Minor refactor

5adcdf1

via - update tests

a8a7c15

Ruby: Don't expose abstract class

80835a5

Make ActionDispatch::Route into a private class ActionDispatch::RouteImpl, defining a new class Route which exposes the necessary public API from RouteImpl. Also rename getHTTPMethod to getHttpMethod.

hmac force-pushed the hmac/actiondispatch branch from c94f4c1 to 80835a5 Compare February 2, 2022 21:43

hmac added 2 commits February 3, 2022 10:55

Ruby: Ensure TRoute and TRouteBlock are private

61cd05c

Ruby: prevent bad join in ActionController.qll

3031b39

hvitved reviewed Feb 8, 2022

View reviewed changes

Ruby: Add nomagic pragma to helper

f276904

hvitved approved these changes Feb 9, 2022

View reviewed changes

hmac merged commit f302222 into main Feb 9, 2022

hmac deleted the hmac/actiondispatch branch February 9, 2022 20:46

Dec	JAN	Feb
	07
2025	2026	2027


		override string getAction() { result = action }

		override string getHTTPMethod() { result = httpMethod }

	via != "all" and result = "via"
	via != "all" and result = via

Ruby: Rails route resolution #7061

Ruby: Rails route resolution #7061

Uh oh!

Conversation

hmac commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickrolfe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickrolfe commented Dec 21, 2021

Uh oh!

alexrford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexrford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

hvitved Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hmac commented Nov 4, 2021 •

edited

Loading

hvitved Jan 26, 2022 •

edited

Loading