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.

Sign up for GitHub

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

Fix bug in handling of invalid if-modified-since/last-modified values #23

Closed
wants to merge 6 commits into from
+75 −5
Closed

Fix bug in handling of invalid if-modified-since/last-modified values #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bfdd169
Fix typo in in invalid date header tests
benurb Jul 19, 2017
bbdc335
Fix handling of invalid if-modified-since/last-modified dates
benurb Jul 19, 2017
3a685f8
Change date check to be more robust against monkey patched Date.parse
benurb Jul 21, 2017
dbed31d
Add tests for edge cases in date parsing
benurb Jul 21, 2017
a40c886
Update test names for unix timestamp 0 tests
benurb Jul 24, 2017
18a72a8
Change monkey patching test to better fit datejs behavior
benurb Jul 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,21 @@ function fresh (reqHeaders, resHeaders) {

// if-modified-since
if (modifiedSince) {
var lastModified = resHeaders['last-modified']
var modifiedStale = !lastModified || Date.parse(lastModified) > Date.parse(modifiedSince)
var lastModifiedTimestamp = Date.parse(resHeaders['last-modified'])
var modifiedSinceTimestamp = Date.parse(modifiedSince)

// If-Modified-Since request header has an invalid date value. Every falsy value except 0 is treated as invalid,
// because the default behavior of Date.parse to return NaN on invalid dates might have been monkey patched
if ((!modifiedSinceTimestamp && modifiedSinceTimestamp !== 0) || !isFinite(modifiedSinceTimestamp)) {
return false
}

// Last-Modified response header has an invalid date value
if ((!lastModifiedTimestamp && lastModifiedTimestamp !== 0) || !isFinite(lastModifiedTimestamp)) {
return false
}

var modifiedStale = lastModifiedTimestamp > modifiedSinceTimestamp

if (modifiedStale) {
return false
Expand Down
63 changes: 60 additions & 3 deletions test/fresh.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,18 +107,75 @@ describe('fresh(reqHeaders, resHeaders)', function () {
describe('with invalid If-Modified-Since date', function () {
it('should be stale', function () {
var reqHeaders = { 'if-modified-since': 'foo' }
var resHeaders = { 'modified-since': 'Sat, 01 Jan 2000 00:00:00 GMT' }
var resHeaders = { 'last-modified': 'Sat, 01 Jan 2000 00:00:00 GMT' }
assert.ok(!fresh(reqHeaders, resHeaders))
})
})

describe('with invalid Modified-Since date', function () {
describe('with invalid Last-Modified date', function () {
it('should be stale', function () {
var reqHeaders = { 'if-modified-since': 'Sat, 01 Jan 2000 00:00:00 GMT' }
var resHeaders = { 'modified-since': 'foo' }
var resHeaders = { 'last-modified': 'foo' }
assert.ok(!fresh(reqHeaders, resHeaders))
})
})

describe('with unix epoch (timestamp 0) in If-Modified-Since header', function () {
it('should be stale', function () {
var reqHeaders = { 'if-modified-since': 'Thu, 01 Jan 1970 00:00:00 GMT' }
var resHeaders = { 'last-modified': 'Sat, 01 Jan 2000 00:00:00 GMT' }
assert.ok(!fresh(reqHeaders, resHeaders))
})
})

describe('with unix epoch (timestamp 0) in Last-Modified header', function () {
it('should be fresh', function () {
var reqHeaders = { 'if-modified-since': 'Sat, 01 Jan 2000 00:00:00 GMT' }
var resHeaders = { 'last-modified': 'Thu, 01 Jan 1970 00:00:00 GMT' }
assert.ok(fresh(reqHeaders, resHeaders))
})
})

describe('with unix epoch (timestamp 0) in Last-Modified and If-Modified-Since header', function () {
it('should be fresh', function () {
var reqHeaders = { 'if-modified-since': 'Thu, 01 Jan 1970 00:00:00 GMT' }
var resHeaders = { 'last-modified': 'Thu, 01 Jan 1970 00:00:00 GMT' }
assert.ok(fresh(reqHeaders, resHeaders))
})
})

describe('when Date.parse was monkey patched', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is going to actually have a test, just test with what we really care about specifically: the datejs patching.

Copy link
Contributor Author

@benurb benurb Jul 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Mhh ok. I haven't had a look at datejs yet, but do you suggest adding it as a devDependency or rather make the mock more similar to the datejs patching? Or are these tests a bad idea in general?

Copy link
Contributor

Choose a reason for hiding this comment

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

A devDep is just fine, as they are available during the testing. If you want to just not have any tests, that's OK too, but if you wanted to make some patching tests, I'd say to either (a) directly use datejs in them or (b) make the mocking patches actually function like the datejs does (which afaik these are different from what datejs returns).

Copy link
Contributor Author

@benurb benurb Jul 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I was not able to use datejs without polluting global objects, but I updated the mock to return the same result as datejs for the strings in the tests. If you don't like this I would rather remove the tests again.

var dateParse = Date.parse

before(function () {
Date.parse = function (str) {
var date = dateParse(str)
// https://www.npmjs.com/package/datejs unconditionally patches Date.parse and returns null instead of NaN
// for invalid dates
return isNaN(date) ? null : date
}
})

after(function () {
Date.parse = dateParse
})

describe('with invalid If-Modified-Since date', function () {
it('should be stale', function () {
var reqHeaders = { 'if-modified-since': 'foo' }
var resHeaders = { 'last-modified': 'Sat, 01 Jan 2000 00:00:00 GMT' }
assert.ok(!fresh(reqHeaders, resHeaders))
})
})

describe('with invalid Last-Modified date', function () {
it('should be stale', function () {
var reqHeaders = { 'if-modified-since': 'Sat, 01 Jan 2000 00:00:00 GMT' }
var resHeaders = { 'last-modified': 'foo' }
assert.ok(!fresh(reqHeaders, resHeaders))
})
})
})
})

describe('when requested with If-Modified-Since and If-None-Match', function () {
Expand Down