Netflix / zuul

Zuul is a gateway service that provides dynamic routing, monitoring, resiliency, security, and more.
Apache License 2.0
13.44k stars 2.37k forks source link

zuul2 http query param parse disorder #525

Open lsgggg123 opened 5 years ago

lsgggg123 commented 5 years ago

https://github.com/Netflix/zuul/blob/2.1/zuul-core/src/main/java/com/netflix/zuul/message/http/HttpQueryParams.java#L41

HttpQueryParams using ListMultimap<String, String> as data structure to save query params, this map is support multi value of query param values, and the values is ordered, but the query param name itself become disordered, I'm afraid sometimes this may cause an accident.

lsgggg123 commented 5 years ago

com.google.common.collect.LinkedListMultimap maybe better ?

lsgggg123 commented 5 years ago

here is unit test:

image

image

artgon commented 5 years ago

It's not safe to rely on ordering of query param key-values because maps don't guarantee any sort of ordering.

That said, can you provide a use case where this is necessary? Are you trying to make a decision based on enumerating keys/values in order?

lsgggg123 commented 5 years ago

Yes, I have a use case that need query param key-values keep order as APP send. The APP add a sign of the url and query params, in zuul2, I use a filter to check the sign is correct from the APP and query params not changed by someone, if the query param order changed, then the sign will not passed.

As a proxy, shouldn't zuul2 keep the http the same as before?

agx5109 commented 3 years ago

In a lot of cases the request param order is often used to verify the signature headers in which case re-ordering of the request params would lead to issues.

Switching over to LinkedListMultimap would make a lot of sense.

Some examples:

Would it be possible @artgon, @carl-mastrangelo ? I have raised a PR for the intended change as well.