erlyaws / yaws

Yaws webserver
https://erlyaws.github.io
BSD 3-Clause "New" or "Revised" License
1.28k stars 267 forks source link

yaws_api:parse_query fails to find query parameters. #243

Open miby00 opened 8 years ago

miby00 commented 8 years ago

If I change:

%% parse the command line query data
parse_query(Arg) ->
    case get(query_parse) of
        undefined ->
            Res = case Arg#arg.querydata of
                      [] -> [];
                      D  -> parse_post_data_urlencoded(D)
                  end,
            put(query_parse, Res),
            Res;
        Res ->
            Res
    end.

to

%% parse the command line query data
parse_query(Arg) ->
    case get(query_parse) of
        _undefined -> %% <--- CHANGE
            Res = case Arg#arg.querydata of
                      [] -> [];
                      D  -> parse_post_data_urlencoded(D)
                  end,
            put(query_parse, Res),
            Res;
        Res ->
            Res
    end.

Yaws works just fine.

The above code optimization will only work if the same process never handles two different queries.

Scenario for repeating error:

1) Do a "arg_rewrite" of an incoming query, add some query parameters, and send it to a .yaws file. 2) Read query parameters in the .yaws file using the function yaws_api:parse_query/1, the query parameters will not be found.

I stumbled across this error when testing my code for automatic redirect the login page when accessing information requiring login.

The error was introduced in commit: a3edbee77324aa0a6e15b5c4084f45bf7f86d122

capflam commented 8 years ago

Hi,

Could you share your arg_rewrite module ? Because I cannot reproduce the problem. Here is the naive arg_rewrite module I used to do my tests:

-module(add_query_param).
-export([arg_rewrite/1]).
-include_lib("yaws/include/yaws_api.hrl").

arg_rewrite(#arg{}=Arg) ->
    Req  = Arg#arg.req,
    {abs_path, Path} = Req#http_request.path,
    Path1 = case string:chr(Path, $?) of
               0 -> Path ++ "?param=value";
               _ -> Path ++ "&param=value"
           end,
    Req1 = Req#http_request{path={abs_path, Path1}},
    Arg#arg{req=Req1}.
miby00 commented 8 years ago

Sorry my description wasn't enough to reproduce the error.

Here I have some stupid boiled down code to make it easy to reproduce.

arg_rewrite(Arg = #arg{req = Req}) ->
    {abs_path, Path} = Req#http_request.path,
    arg_rewrite(Arg, Path).

arg_rewrite(Arg = #arg{req = Req}, "/reproduce/error") ->
    yaws_api:parse_query(Arg), %% This will save wrong params in process dictionary
    Url = "/reproduceError.yaws?test1=first&test2=second",
    Arg#arg{req = Req#http_request{path = {abs_path, Url}}};
arg_rewrite(Arg, "/reproduceError.yaws" ++ _) ->
    Arg;

If you then create a file reproduceError.yaws that looks like:

<erl>
out(Arg)->
    case yaws_api:parse_query(Arg) of
        [] ->
            [{status, 500},
             {header, {content_type, "application/json"}},
             {header, {cache_control, "no-cache"}},
             {html, json2:encode({struct, [{"error", "Params lost in space"}]})}];
        Params ->
            [{status, 200},
             {header, {content_type, "application/json"}},
             {header, {cache_control, "no-cache"}},
             {html, json2:encode({struct, Params})}]
    end.
</erl>

This call will work:

http://localhost/reproduceError.yaws?test1=first&test2=second

and produce the following output:

{"test1":"first","test2":"second"}

This call will fail:

http://localhost/reproduce/error

with the following output:

{"error":"Params lost in space"}

If you remove the line

yaws_api:parse_query(Arg), %% This will save wrong params in process dictionary

everything works as intended.

In my more complex code I do not have a explicit yaws_api:parse_query, but something I do makes it happen anyway. But the above code will at least show you the problem.

If the above doesn't works for you we run different setups. But lets start with you trying the above code :)

Thx for a great open source effort!

capflam commented 8 years ago

Ok, I understand. I think you call yaws_api:getvar/2 or yaws_api:queryvar/2. These functions depend on yaws_api:parse_query/1. Query parsing is designed to be done in yaws scripts or appmod. But this is not mandatory. The API should be improved.

In the meantime, a workaround would be to erase previously parsed query by calling erase(query_parse) by hand in your arg_rewrite module. Of course, all parameters from the original request not found in the new one will be lost.

miby00 commented 8 years ago

Thanks for the quick response, I will try to work around the problem. I still think it would be nice if the problem was fixed :)

capflam commented 8 years ago

Of course, I will work on it. It's just a matter of time now :) I'm pretty busy at the moment.