Invalid path parameter parsing in concurrent requests

[malesch]

It looks as if parsed path parameters can end up in other requests/threads.

To reproduce this issue I took the simplied source of the ring-swagger example, removed swagger and the existing routes, and added a new POST route which takes a path parameter and receives a JSON document with same path parameter value. Both values should then be the same what is checked in the handler:

(ns example.server
  (:require [reitit.ring :as ring]
            [reitit.coercion.spec]
            [reitit.ring.coercion :as coercion]
            [reitit.dev.pretty :as pretty]
            [reitit.ring.middleware.muuntaja :as muuntaja]
            [reitit.ring.middleware.exception :as exception]
            [reitit.ring.middleware.multipart :as multipart]
            [reitit.ring.middleware.parameters :as parameters]
            [ring.adapter.jetty :as jetty]
            [muuntaja.core :as m]))

(def app
  (ring/ring-handler
    (ring/router

      [["/check/:id"
        {:post {:handler (fn [{:keys [uri body-params path-params]}]
                           (let [path-params-id (:id path-params)
                                 body-params-id (:id body-params)]
                             (if (= path-params-id body-params-id)
                               {:status 200}
                               {:status 500 :body (format "uri=%s, body-param-id=%s, path-param-id=%s" uri body-params-id path-params-id)})))}}]]

      {;;:reitit.middleware/transform dev/print-request-diffs ;; pretty diffs
       ;;:validate spec/validate ;; enable spec validation for route data
       ;;:reitit.spec/wrap spell/closed ;; strict top-level validation
       :exception pretty/exception
       :data      {:coercion   reitit.coercion.spec/coercion
                   :muuntaja   m/instance
                   :middleware [;; query-params & form-params
                                parameters/parameters-middleware
                                ;; content-negotiation
                                muuntaja/format-negotiate-middleware
                                ;; encoding response body
                                muuntaja/format-response-middleware
                                ;; exception handling
                                exception/exception-middleware
                                ;; decoding request body
                                muuntaja/format-request-middleware
                                ;; coercing response bodys
                                coercion/coerce-response-middleware
                                ;; coercing request parameters
                                coercion/coerce-request-middleware
                                ;; multipart
                                multipart/multipart-middleware
                                ]}})
    (ring/routes
      (ring/create-default-handler))))

(defn start []
  (jetty/run-jetty #'app {:port 3000,
                          ;:max-threads 4
                          ;:min-threads 1
                          :join? false})
  (println "server running in port 3000"))

To provoke the issue a simple Bash script with curl can be used which calls this route with the corresponding JSON document:

#!/bin/bash

for i in {1..10000}
do
    out=$(curl -s -X POST -H "Content-Type: application/json" --data-raw "{\"id\": \"$i\"}" http://localhost:3000/check/$i)
    if [ ! -z "$out" ]
    then
        echo $out
    fi
done

If the script is executed in two terminal it should output the cases where the two parameters are not identical, something like:

uri=/check/948, body-param-id=948, path-param-id=5
uri=/check/953, body-param-id=953, path-param-id=10
uri=/check/954, body-param-id=954, path-param-id=11
uri=/check/956, body-param-id=956, path-param-id=13
[...]

[ikitommi]

That is really bad. Despite there is Java involved, path-parameters should all be immutable, will dig into this asap.

[miikka]

Looks like this bug affects every version in the 0.3.x series. SegmentTrie, which was introduced in 0.2.11 and was replaced by Trie in 0.3.0, handles Match objects differently enough that at least it does not have exactly the same bug.

[ikitommi]

This is fixed in [metosin/reitit "0.3.5"].

[ikitommi]

Root cause: despite all parameters were immutable, there was one mutable reference to those on the Java side. Now it's all final. Will go through all the other Java interop places to make sure there are no more errors like this.