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

[mod_ssi] Add SSI vars "SCRIPT_URI" and "REQUEST_SCHEME" #24

Closed
wants to merge 1 commit into from
Closed

[mod_ssi] Add SSI vars "SCRIPT_URI" and "REQUEST_SCHEME" #24

wants to merge 1 commit into from

Conversation

fbrosson
Copy link
Contributor

This is a proposal to add to lighttpd the famous SSI variables SCRIPT_URI and SCRIPT_URL (known to Apache users), as well as a bonus ENV variable called REQUEST_SCHEME.

SCRIPT_URI and SCRIPT_URL will be available as SSI variables from within documents handled by mod_ssi.
They can be used like any other SSI var with the #echo var SSI command:

<!--#echo var="SCRIPT_URI"-->
<!--#echo var="SCRIPT_URL"-->

Webmasters willing to display links to the W3C Validator will be able to use:

<a href="http://validator.w3.org/check?uri=<!--#echo var="SCRIPT_URI"-->">…</a>

instead of the generic http://validator.w3.org/check?uri=referer link which does not work on some (most?) browsers which do not send referers when the link itself resides in a document sent through https.

REQUEST_SCHEME will be available as an environment variable. It is defined as "http" or "https", depending on the scheme of the connection. It is safe to use this name as it does not conflict with any existing variable in Apache or Nginx. This is slightly different from the HTTPS var which is often added by webadmins on their server's configuration. EDIT: Some Apache modules also define REQUEST_SCHEME with the same possible values as this proposal.

@fbrosson
Copy link
Contributor Author

@stbuehler I noticed that some users also need or ask for SCRIPT_URL, so I just updated the patch to include that one too. I also need to check whether or not we need some sanitization to prevent exploits, check whether special chars would need to be escaped, and find out whether or not it would be appropriate to add such sanitization and/or escape sequences in mod_ssi. But I think all required sanitization is already performed by core at the time mod_ssi is called and I also believe that special chars are already handled by the #echo var SSI command. Any help/feedback is welcome from anyone. Thanks!

@gstrauss
Copy link
Member

@fbrosson please replace (con->uri.scheme && con->uri.scheme->used) and other direct use of b->used with (!buffer_string_is_empty(con->uri.scheme))

[Corrected]

@fbrosson
Copy link
Contributor Author

@gstrauss : Done, thanks!
BTW, it's not buffer_is_string_empty() but buffer_string_is_empty().
Good catch anyway ;-)
@stbuehler & @gstrauss : I've noticed that with the current proposal #echo var will not yeld anything for SCRIPT_URI if con->uri.authority is not set. It is always set for HTTP/1.1 (and newer), as well as for HTTP/1.0 if the client is polite (i.e. it sends the Host: request header even when instructed to use HTTP/1.0). Anyway, situations where a client does not send the Host: request header are really uncommon. So I guess the proposal is still OK.
Regarding the potential security issues, I've been thinking about it and my current position is that since SCRIPT_URI is only available as a SSI var (and not as an environment var), the only thing exploits could do would be to have mod_ssi produce funny html code, but that code would only be sent to the client. There is therefore no risk of having such unexpected code being sent to another handler since SCRIPT_URI is only know to mod_ssi which, in turn, does not call any other interpreter. And if mod_ssi calls some CGI code through the #exec command, then, again, that code won't see SCRIPT_URI because it is not being set as an ENV var but as a SSI var. So I think there are no security problems.

@gstrauss
Copy link
Member

gstrauss commented Mar 1, 2016

@fbrosson, thanks for the correction. I updated my comment.

Regarding the bonus "SCHEME" variable, I think I prefer the name "REQUEST_SCHEME" which matches the name used by some Apache modules.

BTW, if con->uri.authority is not set, you might use con->server_name, if that is set, and assume that admin has set the server_name to a working DNS name. That won't always work. Alternatively, since the client got routed to the vhost without providing Host header, it should be correct to provide an IP address in place of con->uri.authority. Take a look in http-header-glue.c:http_response_redirect_to_directory() to see how it handles empty con->uri.authority.

Or, just leave it blank if the client does not provide Host. I agree with you that this is the simplest solution, and not a likely scenario since all modern, popular browsers send Host header, even for HTTP/1.0 requests, else those clients would not work with a large portion of the internet.

@gstrauss
Copy link
Member

gstrauss commented Mar 1, 2016

Aside: I see that SERVER_NAME in mod_ssi is the server IP, which looks suboptimal to me (it should be the vhost name where available) and the IP might be a wildcard address instead of the IP of the current socket connection (getsockname()). Ick.

FYI: in historical versions of Apache, mod_rewrite set SCRIPT_URI and SCRIPT_URL. If mod_rewrite was not enabled, then those variables were not present. That seems to still be the case if you look at https://httpd.apache.org/docs/2.4/mod/mod_rewrite.html (search for SCRIPT_URx). I am having trouble finding documentation on SCRIPT_URI and SCRIPT_URL in the current Apache 2.4 documentation. I did find it in http://httpd.apache.org/docs/2.2/en/mod/mod_rewrite.html

@fbrosson
Copy link
Contributor Author

fbrosson commented Mar 1, 2016

Thanks @gstrauss for your messages. I had not noticed that some Apache modules alread defined REQUEST_SCHEME. I don't know if these are third-party modules or if they are original Apache modules, but I agree with you that it would be better to follow your suggestion to use REQUEST_SCHEME instead of SCHEME. I have therefore updated the proposal.
@stbuehler I guess you also agree with this name change, but please let me know if this is not the case.

@stbuehler @gstrauss :
I also got rid of some duplicate code I had added: SCHEME was being available both as an SSI var and as an ENV var. But since the #echo var command has recently been changed (for 1.4.40) to look for ENV vars if there are no matches in SSI vars, it was not necessary to create a SSI_ECHO_ for it.
Regarding the way http-header-glue.c:http_response_redirect_to_directory() handles empty con->uri.authority, this is exactly what I was looking for! I had thought about using the same logic, but there might be a risk: an attacker willing to discover the local IP address of a server behing a proxy would be able to get that information by making a plain HTTP/1.0 request (without any Host: request header.) But I guess this "risk" has already been deemed minor, otherwise that logic would not have been added to http_response_redirect_to_directory().
So if you wish I can add that logic to make SCRIPT_URI available in all cases. What do you think? That would need around 73 extra lines. Would it be worth adding all these lines of code just to handle really uncommon clients? On the other side, making sure SCRIPT_URI will always be defined looks great. Hmm, maybe we could move that logic to a separate function and call it from both places to avoid code duplication. But this would take a little bit longer.

@gstrauss
Copy link
Member

gstrauss commented Mar 1, 2016

(I think you meant con->uri.authority (for use in SCRIPT_URI) instead of REQUEST_SCHEME in the last paragraph above.)

Instead of greatly increasing the size of this patch for what should be a rare case, I would suggest leaving the patch as-is for now.

If it turns out to be strongly desired to handle that case, then, as you also suggested, I think it would be better to create a new routine in http-header-glue.c which sets con->uri.authority if it has not been set, and to use that routine in both http_response_redirect_to_directory() and in mod_ssi.c

@fbrosson
Copy link
Contributor Author

fbrosson commented Mar 1, 2016

Oops, you are perfectly right about my confusion between SCRIPT_URI and REQUEST_SCHEME in the last paragraph of my previous message. I'll fix it right now to avoid confusing new readers. Thanks!
Regarding the corner case I agree that the patch is better the way it is now, and that, as you say, we can always change it someday if there is a need to.

@fbrosson fbrosson changed the title [mod_ssi] Add SSI vars "SCRIPT_URI" and "SCHEME" [mod_ssi] Add SSI vars "SCRIPT_URI" and "REQUEST_SCHEME" Mar 1, 2016
@gstrauss
Copy link
Member

x-ref: I filed https://redmine.lighttpd.net/issues/2721 to track this issue on lighttpd redmine issue tracker.

@fbrosson
Copy link
Contributor Author

@gstrauss Good idea, thanks!
@stbuehler I've just rebased the patch against master.

Ref: Feature #2721: https://redmine.lighttpd.net/issues/2721

This is a proposal to add to lighttpd the famous SSI variables
SCRIPT_URI and SCRIPT_URL (known to Apache users), as well as a bonus
ENV variable called REQUEST_SCHEME.

SCRIPT_URI and SCRIPT_URL will be available as SSI variables from
within documents handled by mod_ssi.
They can be used like any other SSI var with the "#echo var" command:
<!--#echo var="SCRIPT_URI"-->
<!--#echo var="SCRIPT_URL"-->
Webmasters willing to display links to the W3C Validator will be able
to use:
<a href="http://validator.w3.org/check?uri=<!--#echo var="SCRIPT_URI"-->">…</a>
instead of the generic http://validator.w3.org/check?uri=referer link
which does not work on some (most?) browsers which do not send
referers when the link itself resides in a document sent through
https.

REQUEST_SCHEME will be available both as an environment variable. It
is defined as "http" or "https", depending on the scheme of the
connection. It is safe to use this name as it does not conflict with
any existing variable on Apache or Nginx. This is slightly different
from the HTTPS var which is often added by webadmins on their server's
configuration. EDIT: Some Apache modules also define REQUEST_SCHEME
with the same possible values as this proposal.
@stbuehler
Copy link
Member

Thanks a lot! Applied in a579e7f with some whitespace changes.

@stbuehler stbuehler closed this Mar 26, 2016
@fbrosson
Copy link
Contributor Author

You're welcome! That's great!
Thank you for having accepted the patch ;-)

@fbrosson fbrosson deleted the script_uri branch March 26, 2016 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants