Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid modifying r->args.data, which breaks the $arg_name in nginx #538

Merged
merged 1 commit into from
Dec 20, 2018

Conversation

coercible
Copy link
Contributor

This pull request fixes a bug in nginx-zauth-module. We pass the &r->args.data to ngx_unescape_uri, the pointer will be modified in ngx_unescape_uri, see https://github.com/nginx/nginx/blob/master/src/core/ngx_string.c#L1735. Then the argument variables in query string would be lost in nginx script.

You can confirm the trouble by this way:

  1. in nginx.conf, create such a location:
        location /bug {
            return 200 "bug is $arg_bug";
        }
  1. visit this endpoint via curl: curl -i -XGET http://localhost/bug?bug=zauth-bug. You should get bug is zauth-bug however currently the value or this variable is null.

@CLAassistant
Copy link

CLAassistant commented Dec 4, 2018

CLA assistant check
All committers have signed the CLA.

@fisx
Copy link
Contributor

fisx commented Dec 5, 2018

This looks very plausible. I don't know this code very well so I want to find somebody to double-check. I will let you know soon.

Thanks!

@raphaelrobert
Copy link

This doesn't seem to pose a problem on our end and doesn't break anything. Could you kindly link to where this breaks in your code?

@coercible
Copy link
Contributor Author

We have integrated nginz with lua-nginx-module. We need to do some argument-dependent dispatch in nginx script, just something like:

location /a {
     content_by_lua {
         local args, err = ngx.req.get_uri_args()
         if args['key1'] and args['key2'] then
             local res = ngx.location.capture('/b', ...)
             ngx.say(res.body)
         else
             local res = ngx.location.capture('/c', ...)
             ngx.say(res.body)
         end
     }
}

location /b {
    proxy_pass http://upstream1;
}

location /c {
    proxy_pass http://upstream2;
}

The bug fixed by this pull request leads to inaccessibility to the query arguments.

@raphaelrobert
Copy link

This is a friendly reminder that Wire's server code is published under AGPL. This means, among other things, that derived work from our code also needs to be published under the same license and the source code needs to be made available.

@fisx fisx merged commit d9c8395 into wireapp:develop Dec 20, 2018
@coercible
Copy link
Contributor Author

Thanks for merging this PR.

@raphaelrobert Thanks for your kiindful reminder, we will open source our code after certain development and testing.

@jschaul jschaul mentioned this pull request Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants