jfhbrook / node-ecstatic

A static file server middleware that works with core http, express or on the CLI!
https://github.com/jfhbrook/node-ecstatic
MIT License
975 stars 194 forks source link

Remove extra double quotes in ETAG inserted by JSON.stringify #224

Closed s25g5d4 closed 6 years ago

s25g5d4 commented 6 years ago

Hi there. According to RFC7232 section 2.3., a valid etag value should be double-qouted and string between should contains no double quote character itself. While it doesn't seem to break any browser, it is kind of feeling weird each time I inspect the headers when I'm debuging some other things.

There're not much difference after this PR applied. the string is almost the same, only with the double qoutes being removed when converting Date object to plain string. Internally JSON.stringify() calls Date.prototype.toJSON() on Date objects to get a valid JSON representation of its value, and toJSON() is implemented to return the returned value of Date.prototype.toISOString(). So here I removed JSON.stringify() and made a direct call to toISOString().

Headers before patch applied:

$ curl -so /dev/null -D - http://127.0.0.1:8080
HTTP/1.1 200 OK
server: ecstatic-3.2.0
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: Authorization, Content-Type, If-Match, If-Modified-Since, If-None-Match, If-Unmodified-Since
cache-control: max-age=3600
last-modified: Fri, 06 Jul 2018 04:41:35 GMT
etag: W/"2266206-15946-"2018-07-06T04:41:35.507Z""
content-length: 15946
content-type: text/html; charset=UTF-8
Date: Fri, 06 Jul 2018 07:48:18 GMT
Connection: keep-alive

Headers after patch applied:

$ curl -so /dev/null -D - http://127.0.0.1:8080
HTTP/1.1 200 OK
server: ecstatic-3.2.0
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: Authorization, Content-Type, If-Match, If-Modified-Since, If-None-Match, If-Unmodified-Since
cache-control: max-age=3600
last-modified: Fri, 06 Jul 2018 04:41:35 GMT
etag: W/"2266206-15946-2018-07-06T04:41:35.507Z"
content-length: 15946
content-type: text/html; charset=UTF-8
Date: Fri, 06 Jul 2018 07:46:44 GMT
Connection: keep-alive

It would be appreciated if you could take a look. Thanks.

codecov-io commented 6 years ago

Codecov Report

Merging #224 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #224   +/-   ##
=======================================
  Coverage   75.86%   75.86%           
=======================================
  Files           9        9           
  Lines         518      518           
  Branches      117      117           
=======================================
  Hits          393      393           
  Misses         45       45           
  Partials       80       80
Impacted Files Coverage Δ
lib/ecstatic/etag.js 80% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 609f1c9...1f9984b. Read the comment docs.

jfhbrook commented 6 years ago

Good catch!! I'll try to deploy this this afternoon (new dev environment not 100% set up).

jfhbrook commented 6 years ago

ok, published w/ some other things as 3.2.1.

Cheers!