controller icon indicating copy to clipboard operation
controller copied to clipboard

Return only validated params

Open tak1n opened this issue 3 years ago • 2 comments

When trying out the hanami 2 application template and using the params class method in an action I faced the issue that non-whitelisted params were accessible through req.params.to_h. This is tracked by https://trello.com/c/xvCYJUMi/174-check-out-the-deal-with-actionparams.

In a nutshell, the issue appears with the latest hanami/router version. In the latest version router.params in the Rack env is containing more params than previously, which hanami/controller merges with the dry-validation contract result output in https://github.com/hanami/controller/blob/main/lib/hanami/action/params.rb#L241.

hanami v1.3.5 - ruby 2.7.6:

module Web
  module Controllers
    module Items
      class Show
        include Web::Action

        params do
          required(:id).value(type?: Integer)
        end

        def call(params)
          require "debug"
          binding.break
        end
      end
    end
  end
end

(ruby@/home/benjamin/.gem/ruby/2.7.6/bin/hanami#375267) params.env["REQUEST_URI"]
"http://localhost:3000/items/1?testparam=abc"
(ruby@/home/benjamin/.gem/ruby/2.7.6/bin/hanami#370483) params.env["router.params"]
{:id=>"1"}
(ruby@/home/benjamin/.gem/ruby/2.7.6/bin/hanami#370483) params.to_h
{:id=>1}

hanami v2 (github main branch) - ruby 3.1.2:

module Main
  module Actions
    module Items
      class Show < Action::Base
        params do
          required(:id).value(:integer)
        end

        def handle(req, res)
          require "debug"
          binding.break
        end
      end
    end
  end
end

(ruby) req.params.env["REQUEST_URI"]
"/items/1?testparam=abc"
(ruby) req.params.env["router.params"]
{:testparam=>"abc", :id=>"1"}
(ruby) req.params.to_h
{:testparam=>"abc", :id=>1}

Although at first sight, it seems to be more of a problem of hanami/router which changed the behavior here, I still think it makes more sense to fix this in hanami/controller. As in hanami/controller the params class method lives and dictates whether we want to validate incoming params.

tak1n avatar May 12 '22 21:05 tak1n

It seems the changes made in here are missing a few pieces, one can still access the non whitelisted params through get and []:

module Main
  module Actions
    module Items
      class Show < Action::Base
        include Deps[
          repo: "repositories.items",
          logger: "application.logger"
        ]

        params do
          required(:id).value(:integer)
        end

        def handle(req, res)
          logger.info(req.params.env["REQUEST_URI"])
          logger.info("Params: #{req.params.to_h}")
          logger.info("Raw params: #{req.params.raw}")
          logger.info(req.params[:testparam])
          logger.info(req.params.get(:testparam))

          item = repo.find(req.params[:id])
          res.body = Serializers::Item.new(item).serialize
        end
      end
    end
  end
end
[timetracker] [INFO] [2022-05-13 13:45:37 +0200] /items/1?testparam=test
[timetracker] [INFO] [2022-05-13 13:45:37 +0200] Params: {:id=>1}
[timetracker] [INFO] [2022-05-13 13:45:37 +0200] Raw params: {"testparam"=>"test", :testparam=>"test", :id=>"1"}
[timetracker] [INFO] [2022-05-13 13:45:37 +0200] test
[timetracker] [INFO] [2022-05-13 13:45:37 +0200] test
[timetracker] [INFO] [2022-05-13 13:45:37 +0200] GET 200 22ms 127.0.0.1 /items/1 - {:testparam=>"test", :id=>"1"}

To be sure I'm following the right approach here I would like to know whether this should really be fixed in hanami/controller or we have to revert to the old behavior of hanami/router.

tak1n avatar May 13 '22 11:05 tak1n