Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[response/index.js] protect against undefined self.headers in sendFile #466

Closed
wants to merge 1 commit into from

3 participants

@barberdt

Noticed on certain page requests that the self.headers object in response.sendFile was undefined. I figured a quick fix was to protect against this inline. However, if there is a higher level fix, that's cool too.

@ben-ng
Owner

I've run into this issue before as well, in my before filters. I wonder if instead of leaving headers undefined we should set them to an empty object.

@mde
Owner
mde commented

I actually removed a similar fix as part of resolving a merge-conflict. Part of the problem is the fact that we're still wrapping the response object. We no longer do this with request, so it might be a good idea at this point to just enhance the native request with the few things we need (as much as I hate to do that).

@mde
Owner
mde commented

Ah, and you also uncovered a pretty serious bug in the same section of code that I created in the process of resolving that merge-conflict. The fix is pushed to Geddy v0.10.3.

Our tests didn't catch it, because we actually had no tests for static file serving. Up to now, that feature has been primarily just for dev use. But lately more people have not been bothering with Nginx, and are just using Node to serve everything.

You also apparently had questions (in IRC) about master vs. the release branch. That might be an interesting question for other people as well. Maybe you could post it to the mailing list, so other folks can see the answer, or chime in?

@barberdt

@mde Sounds great. I'll go ahead and post in the discussion group.

As far as this request goes, if you've already got a fix in 0.10.3, should we go ahead and close this out?

@mde
Owner
mde commented

Yes, I'll close it out. Thanks!

@mde mde closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 5, 2013
This page is out of date. Refresh to see the latest.
Showing with 1 addition and 1 deletion.
  1. +1 −1  lib/response/index.js
View
2  lib/response/index.js
@@ -121,7 +121,7 @@ utils.mixin(Response.prototype, new (function () {
}
else {
// include additional headers
- var headers = utils.mixin(self.headers, opts.headers || {});
+ var headers = utils.mixin(self.headers || {}, opts.headers || {});
headers['Content-Type'] = contentType;
headers['Content-Length'] = stat.size || (stat.blksize * stat.blocks);
headers['Last-Modified'] = stat.mtime.toUTCString();
Something went wrong with that request. Please try again.