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

Add request (input) header for output handler #40

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jtfrey
Copy link

@jtfrey jtfrey commented Sep 26, 2016

If mod_xsendfile is enabled on a request the X-SENDFILE-IS-ENABLED header is added to the request (input) headers so that the output handler knows X-Sendfile is supported on the request output.

Updated the docs accordingly.

@nmaier
Copy link
Owner

nmaier commented Sep 26, 2016

lgtm in general.

I am pondering right now if it would be better to have the header value state it is mod_xsendfile and what version (kind of a user-agent string) instead of just 1. That way the backend could detect it's mod_xsendfile (and a particular version to work around quirks) that is enabled.
Checking just for the header to be present would still be the same.
And as far as information leaks... If an attacker really controls the backend, and mod_xsendfile had some kind of vulnerability they could exploit... Then they still would try even if mod_xsendfile did not announce it's version so "leaking" the version to the backend like this has still would be no gain for an attacker.

Anything I am missing?

Copy link
Owner

@nmaier nmaier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no objections regarding announcing mod_xsendfile and the version in the header value, I'd like to see you add that too.

@@ -292,8 +295,19 @@ static apr_status_t ap_xsendfile_get_filepath(request_rec *r,
return rv;
}

static int ap_xsendfile_add_request_header(request_rec *r) {
xsendfile_conf_active_t enabled = ((xsendfile_conf_t *)ap_get_module_config(r->per_dir_config, &xsendfile_module))->enabled;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated code, add some function like ap_xsendfile_enabled_for(request_rec*) and use it in all places.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented ap_xsendfile_enabled_for_request()

<li><code>X-SENDFILE-IS-ENABLED</code> - the filter is enabled for this request</li>
</ul>
</li>
<li>Returned by output handler:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit vague...
Something like "mod_xsendfile will process these response headers, if present"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote accordingly

@jtfrey
Copy link
Author

jtfrey commented Sep 26, 2016

I am pondering right now if it would be better to have the header value state it is mod_xsendfile and what version (kind of a user-agent string) instead of just 1. That way the backend could detect it's mod_xsendfile (and a particular version to work around quirks) that is enabled.
Checking just for the header to be present would still be the same.

That seems like a good idea. Since the version doesn't appear in any other strings, I'm leaning toward:

#define AP_XSENDFILE_IS_ENABLED_HEADER "X-SENDFILE-IS-ENABLED"
#ifndef AP_XSENDFILE_IS_ENABLED_HEADER_VALUE
#define AP_XSENDFILE_IS_ENABLED_HEADER_VALUE "mod_xsendfile/1.0"
#endif

And as far as information leaks... If an attacker really controls the backend, and mod_xsendfile had some kind of vulnerability they could exploit... Then they still would try even if mod_xsendfile did not announce it's version so "leaking" the version to the backend like this has still would be no gain for an attacker.

Correct.

Working on your requested alterations right now, should have a commit pushed shortly.

@nmaier
Copy link
Owner

nmaier commented Sep 26, 2016

Sounds good re:version string... Or maybe make it
AP_XSENDFILE_VERSION "1.0"
and then "mod_xsendfile/" AP_XSENDFILE_VERSION

…ke string, "mod_xsendfile/[version]"

- Added function ap_xsendfile_enabled_for_request() to check for enablement per-request
- Documentation altered accordingly
@jtfrey
Copy link
Author

jtfrey commented Sep 26, 2016

Requested changes pushed to my fork

@zee0
Copy link

zee0 commented May 31, 2018

hey folks. it would be great to get this pull merged. folks relying on mod_xsendfile are starting to get push back because it's "no longer actively maintained".

https://serverfault.com/questions/879130/is-mod-xsendfile-deprecated/

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.

3 participants