Plumber param collision in dynamic path and query string

mmuurr · September 17, 2023, 9:30pm

Perhaps I'm being dense, but I'm struggling to find any description of how to handle this breaking behavior with a simple Plumber endpoint defined on a dynamic path. Here's the annotation-based definition:

#* @get /foo/<x>
#* @serializer text
function(x) {
  str(x)  ## log to the console for debugging/inspection purposes
  x
}

Simple enough ... and works like a charm, a GET to http://localhost/foo/bar returns bar, as expected.

But what happens if someone makes a request to that endpoint and decides to add a query parameter like so: http://localhost/foo/bar?x=baz? Well, in that case:

baz is returned -- which is incorrect from the perspective of the API designer, and
an abstraction has just been leaked to the caller (which could be considered a security flaw, as the API designer cannot necessarily control how a caller will probe that API).

Adding the endpoint annotation:

#* @preempt queryString

... solves the issue, but Plumber's default behavior is (i) to have that filter enabled and (ii) it's not immediately obvious (especially to any new programmer). To know to explicitly disable that filter requires inspecting the default router (or reading Plumber's source).

I cannot find any mention of this issue in the documentation; am I missing something re: guidance on how best to handle such scenarios?

meztez · September 17, 2023, 10:11pm

Open an issue on github?

I believe just reversing the order the args are added to the endpoint args list would solve the issue.

Matching is done here :

github.com

rstudio/plumber/blob/e829af6a94380cb897441c1c56129504afb9564f/R/plumber-step.R#L84


      
              req$args
            )
          }
          
          
preexecStep <- function(...) {
            private$runHooks("preexec", c(list(data = hookEnv), args_for_formal_matching()))
          }
          execStep <- function(...) {
            private$runHooksAround("aroundexec", args_for_formal_matching(), .next = function(...) {
              relevant_args <- getRelevantArgs(list(...), func = private$func)
              do.call(private$func, relevant_args, envir = private$envir)
            })
          }
          postexecStep <- function(value, ...) {
            private$runHooks("postexec", c(list(data = hookEnv, value = value), args_for_formal_matching()))
          }
          runSteps(
            NULL,
            function(error) {
              # rethrow error
              stop(error)

During implementation, there is an ote to keep only the first matched same name argument to pass to the endpoint.

github.com

rstudio/plumber/blob/e829af6a94380cb897441c1c56129504afb9564f/R/plumber-step.R#L161


      
            # If there is `...`, then the unnamed args will not be in `fargs` and will be passed through
          
          
  if (!("..." %in% fargs)) {
              # Use the named arguments that match, drop the rest.
              args <- args[names(args) %in% fargs]
            }
          
          
  # dedupe matched formals
            arg_names <- names(args)
            is_farg <- arg_names %in% fargs
            # keep only the first matched formal argument (and all other non-`farg` params)
            args <- args[(is_farg & !duplicated(arg_names)) | (!is_farg)]
          
          
  args
          }
          
          
#' Plumber Endpoint
          #'
          #' Defines a terminal handler in a Plumber router.
          #'
          #' @export

And here is the order the parameters are added to the args list

github.com

rstudio/plumber/blob/e829af6a94380cb897441c1c56129504afb9564f/R/plumber.R#L694


      
            if (!is.null(h$parsers)) {
              h$parsers
            } else {
              private$default_parsers
            }
          req$argsPath <- h$getPathParams(path)
          # `req_body_parser()` will also set `req$body` with the untouched body value
          req$body <- req_body_parser(req, parsers)
          req$argsBody <- req_body_args(req)
          
          
req$args <- c(
            # (does not contain req or res)
            # will contain all args added in filters
            # `req$argsQuery` is available, but already absorbed into `req$args`
            req$args,
            # path is more important than body
            req$argsPath,
            # body is added last
            req$argsBody
          )

All args are available, but only one is matched to the endpoint.

#* @get /foo/<x>
#* @serializer text
function(x, req) {
  str(req$args)  ## log to the console for debugging/inspection purposes
  x
}

meztez · September 17, 2023, 10:15pm

And the relevant issue since this has come up in the past :

github.com/rstudio/plumber

Throw error if multiple matched endpoint formals are found

rstudio:master ← rstudio:multiple_matched_args

opened 03:22PM - 07 Aug 20 UTC

schloerke

+267 -77

Pivoting on approach of #578 to throw an error if multiple args are found vs try…ing to merge args. * Store the parsed post body into `req$postBodyParsed` to avoid possible conflicts with `req$args` * Provides a message about how to address the multiple matched formals * If only `req` and `res` are the available formals, only provide the Plumber `req` and `res`. This allow for routes with many arguments matching to `req` and `res` not fail and allow users to retrieve the proper information from the Plumber `req` object Questions: - [x] Should this be enforced for all POST requests? (Only allow named arguments: `req`, `res`) * No. Allow for `req`, `req` to be given only. - [x] Should path parameters be put into a special location? * Yes. `req$argsPath`. And query parameters at `req$argsQuery` PR task list: - [x] Update NEWS - [x] Add tests - [x] Update documentation with `devtools::document()`

meztez · September 17, 2023, 10:20pm

And here is how to handle it from the comment in NEWS.md

#* @get /foo/<x>
#* @serializer text
function(req) {
  x <- req$argsPath$x
  str(x)  ## log to the console for debugging/inspection purposes
  x
}

Hopefully you have enough information to make an informed decision. Have a good day.

mmuurr · September 17, 2023, 10:25pm

@meztez It looks like some of the behavior is defined only for POST requests? If so, was that intentional? While it's not necessarily normal for other HTTP methods to include bodies, there's nothing stopping the API caller from adding a payload to the request.

I think I'll open a GitHub issue anyhow, as minimally there's probably some additional documentation that can be added to help prevent subtle errors here. In particular, it seems somewhat cumbersome at present to safely protect against this, as it requires disabling the queryStringFilter from the defaultPlumberFilters, which itself then causes some more headaches when one does want to parse the query string ... worth a discussion at least, IMO (though perhaps y'all might feel differently )

Thanks for the response!

meztez · September 17, 2023, 10:40pm

You can protect against this by using the req$argsPath directly instead of relying on the internal matching. I fail to see where this would be defined only for POST request. Beside routes matching, I do not think the method has an impact on args.