Double url-encoding of query string with ALB

[jurasofish]

I've found that if I send a get request to FastAPI like GET eg.com/something?name=John+Smith then the database query generated uses literally John+Smith. Same situation using %20.

It seems that Mangum is doing a second round of url-encoding on the query strings, so when FastAPI (or whatever might sit in the middle) decodes the query string it gets the once-encoded version back.

I'm working with an Application Load Balancer.

I'm not 100% sure what the correct behaviour should be, or whether the URL encoding behaviour should be different between API gateway and ALB.

I'll create a pull request with a failing test for this in a moment.

Some references:

https://docs.aws.amazon.com/elasticloadbalancing/latest/application/lambda-functions.html
"If the query parameters are URL-encoded, the load balancer does not decode them. You must decode them in your Lambda function."

with ALB source, serverless-wsgi decodes and then encodes the query strings before passing off to WSGI:
https://github.com/logandk/serverless-wsgi/blob/f8d5a92f63901ed25d1a09ae19fcaa791e22436f/serverless_wsgi.py#L82-L93

Related issues from serverless-express
CodeGenieApp/serverless-express#241
CodeGenieApp/serverless-express#219

[nathanglover]

Also seeing this issue with ALB, but with the / character being encoded as %252F.

I am getting it when passing a "next" query parameter: /login/?next=/some/redirect/path/.

[jurasofish]

Very nice, any chance of a pull request?
One nitpick --+ is actually encoded, it represents whitespace.

[nathanglover]

Hey @jurasofish.

Are you referring to this part of the tests that I updated?

"queryStringParameters": {
            "q1": "1234ABCD",
            "q2": "b+c",  # not encoded  (this should be a space?)
            "q3": "b%20c",  # encoded
            "q4": "/some/path/",  # not encoded
            "q5": "%2Fsome%2Fpath%2F",  # encoded
        }

And yes I plan to open a PR soon. Was hoping to test the solution on my application first, haven't had the opportunity to yet. Feel free to use my fork to test within you own project as well.

[jurasofish]

Are you referring to this part of the tests that I updated?

Yeah that's the bit.

It also looks like ALB lost support for multi value headers/query strings in #170.
Compare the removed code with the refactored code.
I'll have a go at adding that back in for ALB as part of this issue.

[nathanglover]

@jurasofish I noticed the headers issue with ALB as well while testing this. Was actually the thing that prevented me from seeing if my fix was working. I think we can open a separate issue for it. I might open a PR for it I can figure out how to fix it.

[jurasofish]

ah yeah looks like we're working on the same thing - I'm still working in #179
I think fundamentally here the tests for alb are very poor

[jordaneremieff]

Thanks for all your efforts here! The ALB support was a fairly recent addition even prior to the refactoring, so I appreciate both of you looking into these issues.

@jurasofish in regards to the tests, were the tests for ALB prior to #170 catching these things, or did these issues exists prior? I haven't been using Mangum with ALB myself, so I've been relying on user reports to understand what problems may exist in the implementation.

I don't have a lot of time to look deeply into this myself at the moment, but please mention myself or @koxudaxi in the related PRs when they are ready for review.

[jurasofish]

Yeah #155 looks pretty good. From reading it, looks like it:

• performed double encoding of query params (bug)
• handled returning multi value headers and multi value query strings correctly (including detecting whether multi-value headers are enabled from the ALB input)
• tested the multi value query string/headers stuff well.

[nathanglover]

@jordaneremieff I think this can be closed now? Since you merged #184

[jurasofish]

closed by #184