lunarmodules
expose parseRequest() methods for ftp and http requests
Alternative for #131
The tests seem to be local to @diegonehab system, so I could not run them. So please check carfully.
Additional modifications; I updated the create function to be a method call. For the http module this means that the create function can check its context and be aware of redirecting over other protocols; http -> https or vice versa. (ftp and smtp updated for consistency). Based on discussion here; https://github.com/brunoos/luasec/issues/34
see also @brunoos remark here; https://github.com/brunoos/luasec/issues/34#issuecomment-78609541 on importing more code. Exposing the parse commands would allow LuaSec to reduce the amount of imported code.
Needs testing, so for discussion only at the moment.
Additionally; I thought about adding the fields redirectedFrom and redirectedTo to the request tables. Effectively creating a linked-list of the redirected requests, so the caller (but also the create method) can check the history of redirects. Please comment...
This PR is the basis for a LuaSec fix (https://github.com/brunoos/luasec/issues/38) to allow redirects from http to https and vice versa. That PR uses the create function as a method call to adjust the scheme and port number in case of a https call.
The code is good to go as far as I'm concerned, except for the ftp code, which I could not test. I've tested the http and smtp code with some of my own code using that functionality. But had none for ftp.
Unfortunately by now I mixed two functionalities in this PR;
- use
createas a method call - export the parse functionality
I can disentangle them and provide two separate requests, but that would only make sense if they stand a chance of being pulled.
So @diegonehab can you give me a hint of whether you're inclined to pull either of those functionalities? Then I will prepare the required changes.
Here's yet another request caused by the failure to detect redirects; https://github.com/brunoos/luasec/issues/57#issuecomment-152340735
@Babbleshack for now you could use copas, it has utility functions for http and https, including cross scheme redirects.
see http://keplerproject.github.io/copas/manual.html#highlevel
It's my bad. I'll take a look and see if I can tweak the interface a bit.
I'm going to split this into two separate issues. Will look first into the exporting of parse request. I recently fixed an issue in which HTTP was redirecting to HTTPS without checking the scheme. So we should probably revisit that part of the pull request.
@tieske, can you check the latest push? I exposed the URL parsing functions as "_M.genericform" in http.lua and ftp.lua since that is the name I use for the type of function that accepts tables instead of urls. Maybe generalform is a better name. Either way...
If you are happy with the code, we can discuss the cross-scheme redirects. Sounds good?
Looks good to me :+1:
On the naming; genericform is not very descriptive. Something with parse in it would be better imo.
Agree. I didn't like parseurl because there is a module url that actually does the parsing. It's more like building a generic form of the request from an url.
With regard to the cross scheme redirection, I'm thinking about including a "schemes" field in the request table that includes all schemes that should be supported. The default would be { ["http"] = true }. This would let the code abort redirects from http to https but would also allow the user to override this behavior.
What would "supported" mean in that case?
So if I do a request with { http = true, https = true }, what would LuaSocket do if the scheme turns out to be https? LuaSocket should not have to know anything about LuaSec for example...
Another thing; considering the same http & https example; redirects of http to https (increasing security) should be allowed, whilst https to http (reducing security) should be disallowed by default. I don't see how to handle that with such a scheme table.
If we already support the create function, then the approach I took in PR (passing the request table as an argument to create) allows the user to handle all those cases. Or am I missing something?
Right now, the code does the right thing. I.e., if there is a redirect to any scheme other than http, LuaSocket won't follow it. Previously, it would follow any scheme blindly.
Naturally, this will break the redirect from http to https that LuaSec relies on. The question is how to best allow for this behavior so that LuaSec can keep using all the code in LuaSocket without LuaSocket ever knowing.
It will involve the create trick, of course. But that trick is no longer enough.
Not sure we're on the same page here. The changes I made pass the request table to the create function. Once called, the function can check its context and adjust the request table as needed. Here's an example, the LuaSec http implementation based on my PR; https://github.com/Tieske/luasec/blob/redirects/src/https.lua#L80-L114
But that trick is no longer enough.
I fail to see why that is no longer enough? What am I missing?
The default create function should verify that it is 'http' and return a tcp socket. It should fail on anything else. This leaves anyone free to override the default create and implement whatever scheme or port they need, as long as it uses the http protocol that the core http code in LuaSocket can handle.
I understand that it is possible for the create method to look at the request table and make modifications so it works with https. (We'd need to fix adjust request to not mess with the port, as it currently does. Or at least the messing around should be dependent on the scheme.) This is a good solution for the case in which the original scheme was already https.
I am not sure I agree with the redirects, though. What you seem to be suggesting is that we use a default create method in LuaSocket that checks for the scheme and aborts on anything other than http. I think there should exist a separate mechanism for that. Perhaps a schemes field with the create methods for each scheme.
There was an issue request some time ago that reported LuaSocket would blindly follow a redirect to https ignoring the fact it was https (it assumed it was http). So added a check for the scheme. It is this mechanism that I am not happy with at the moment.
I understand that it is possible for the create method to look at the request table and make modifications so it works with https. (We'd need to fix adjust request to not mess with the port, as it currently does. Or at least the messing around should be dependent on the scheme.) This is a good solution for the case in which the original scheme was already https.
imho LuaSocket should not mess with the port. Use what is specified and if it is absent, assume 80, that's it, no more, no less.
I am not sure I agree with the redirects, though. What you seem to be suggesting is that we use a default create method in LuaSocket that checks for the scheme and aborts on anything other than http. I think there should exist a separate mechanism for that. Perhaps a schemes field with the create methods for each scheme.
Using the create method is a workaround for adjusting request parameters after parsing and before executing. We could leave create alone and implement some other callback to handle it, before_execute or whatever is a better name.
The core responsibility of the http module is to execute a request. The module has 2 uses; 1) provide a simple http request method, with proper sanity checks, 2) provide a way to implement the raw protocol, with no guarantees; garbage in == garbage out. If I want to perform a POST request to ftp://whatever.com that should be possible.
So I would expect the module to;
- prepare the request
- validate/update the request parameters using a provided callback (eg. the
createimplementation I wrote). It should fill in sensible defaults for unknowns (eg.httpscheme, and port80). If this callback is not provided, the default LuaSocket callback should error on anything else thanhttp(as currently implemented). In the ftp example above, I could override the default to make it accept my ftp scheme. - execute the raw request, http protocol on whatever parameters were provided (even scheme
ftp)
In case of redirects rerun the cycle with the new info, passing some old info along, to track number of redirects, etc. preferably a list with the entire redirect history.
A table with schemes could be implemented to do this.Holding the callbacks by 'scheme'. It would also imply one 'catch_all' scheme in the table. That will be used if there is no match (and by default throws an error that the scheme is unsupported).
What I don't like about that is the 'global' nature of such a table within the module. In that case I'd prefer to see an extra table as a field in the request table (same as create is now, but only a function then.)
My idea for the scheme table was as a field in the request. Obviously, there will be a default value in http.lua which we will use when it has not been overridden. The only thing missing is how to specify the "default" scheme in the table. The word "default" is not very safe because someone might create a scheme called "default". :) Maybe we can use the array part. Any good ideas come to mind?
Just throwing my two-cents in here. But you could use [0] on the table for the default scheme. That way, you do not interfere with typical string keys someone might pick, and you do not need to worry about the possibility of people using numeric keys because very few lua programmers seem to deviate from the 1-indexed style.
I know it's not perfect, but perhaps it is GoodEnough™?
