-
Notifications
You must be signed in to change notification settings - Fork 29
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
cache: Add in ExpirationTime to local caches. #46
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -270,6 +270,26 @@ describe('vulnRegexDetector', () => { | |
const response3 = vulnRegexDetector.testSync(pattern, invalidConfig); | ||
assert.ok(response3 === vulnRegexDetector.responses.invalid, `Query succeeded? response ${response3}`); | ||
}); | ||
it('should not return an expired cache value', () => { | ||
const pattern = 'abcde'; // TODO perhaps make some temporary cache for all of the tests and clear it each time. | ||
// Make sync query to prime local persistent cache, but use negative cache value to ensure expiration. | ||
let validConfig = { cache: { type: vulnRegexDetector.cacheTypes.persistent, expirationTime: -1 } }; | ||
const response1 = vulnRegexDetector.testSync(pattern, validConfig); | ||
assert.ok(response1 === vulnRegexDetector.responses.safe, `Error, unexpected response for sync query: ${response1}`); | ||
|
||
let invalidConfig = { | ||
server: { | ||
hostname: 'no such host', | ||
port: 1 | ||
}, | ||
cache: { | ||
type: vulnRegexDetector.cacheTypes.persistent, | ||
expirationTime: -1 | ||
} | ||
}; | ||
const response2 = vulnRegexDetector.testSync(pattern, invalidConfig); | ||
assert.ok(response2 === vulnRegexDetector.responses.invalid, `Query succeeded? response ${response2}. Unless 'no such host' is a valid hostname we must have a cache hit on an expired entry`); | ||
}); | ||
}); | ||
|
||
describe('memory', () => { | ||
|
@@ -292,6 +312,26 @@ describe('vulnRegexDetector', () => { | |
const response2 = vulnRegexDetector.testSync(pattern, invalidConfig); | ||
assert.ok(response2 === vulnRegexDetector.responses.safe, `Query failed: response ${response2}, probably due to my invalid config.server (so cache failed)`); | ||
}); | ||
it('should not return an expired cache value', () => { | ||
const pattern = 'abcde'; // TODO perhaps make some temporary cache for all of the tests and clear it each time. | ||
// Make sync query to prime local in-memory cache, but use negative cache value to ensure expiration. | ||
let validConfig = { cache: { type: vulnRegexDetector.cacheTypes.memory, expirationTime: -1 } }; | ||
const response1 = vulnRegexDetector.testSync(pattern, validConfig); | ||
assert.ok(response1 === vulnRegexDetector.responses.safe, `Error, unexpected response for sync query: ${response1}`); | ||
|
||
let invalidConfig = { | ||
server: { | ||
hostname: 'no such host', | ||
port: 1 | ||
}, | ||
cache: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactor into a |
||
type: vulnRegexDetector.cacheTypes.memory, | ||
expirationTime: -1 | ||
} | ||
}; | ||
const response2 = vulnRegexDetector.testSync(pattern, invalidConfig); | ||
assert.ok(response2 === vulnRegexDetector.responses.invalid, `Query succeeded? response ${response2}. Unless 'no such host' is a valid hostname we must have a cache hit on an expired entry`); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,7 +47,8 @@ const defaultServerConfig = { | |
|
||
const defaultCacheConfig = { | ||
type: CACHE_TYPES.persistent, | ||
persistentDir: path.join(os.tmpdir(), 'vuln-regex-detector-client-persistentCache') | ||
persistentDir: path.join(os.tmpdir(), 'vuln-regex-detector-client-persistentCache'), | ||
expirationTime: 60 * 60 * 24 * 7 // 7 days | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Change comment to |
||
}; | ||
|
||
/********** | ||
|
@@ -276,6 +277,11 @@ function handleCacheConfig (cacheConfig) { | |
cacheConfig.persistentDir = defaultCacheConfig.persistentDir; | ||
} | ||
|
||
// expirationTime must be an integer value | ||
if (!cacheConfig.hasOwnProperty('expirationTime') || !Number.isInteger(cacheConfig.expirationTime)) { | ||
cacheConfig.expirationTime = defaultCacheConfig.expirationTime; | ||
} | ||
|
||
return cacheConfig; | ||
} | ||
|
||
|
@@ -341,8 +347,19 @@ function updateCache (config, pattern, response) { | |
if (!useCache(config)) { | ||
return; | ||
} | ||
/* Only cache VULNERABLE|SAFE responses. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a newline before and after this block. |
||
if (response !== RESPONSE_VULNERABLE && response !== RESPONSE_SAFE) { | ||
return; | ||
} | ||
/* This entry will expire config.expirationTime seconds from now. */ | ||
let expirationTimeInMilliseconds = 1000 * config.cache.expirationTime; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Declare these variables |
||
let expiryDate = new Date(Date.now() + expirationTimeInMilliseconds); | ||
let wrappedResponse = { | ||
response: response, | ||
validUntil: expiryDate.toISOString() | ||
}; | ||
|
||
return kvPut(config, pattern, response); | ||
return kvPut(config, pattern, wrappedResponse); | ||
} | ||
|
||
/* Returns RESPONSE_{VULNERABLE|SAFE} on hit, else RESPONSE_UNKNOWN on miss or disabled. */ | ||
|
@@ -351,15 +368,20 @@ function checkCache (config, pattern) { | |
return RESPONSE_UNKNOWN; | ||
} | ||
|
||
return kvGet(config, pattern); | ||
let valueRetrieved = kvGet(config, pattern); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Declare these variables |
||
if (valueRetrieved === RESPONSE_UNKNOWN) { | ||
return RESPONSE_UNKNOWN; | ||
} | ||
/* Check if the cache entry has expired. */ | ||
let lastValidDate = new Date(valueRetrieved.validUntil); | ||
if (Date.now() > lastValidDate) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to write comparisons as |
||
/* The entry in the cache has expired. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For consistency, |
||
return RESPONSE_UNKNOWN; | ||
} | ||
return valueRetrieved.response; | ||
} | ||
|
||
function kvPut (config, key, value) { | ||
/* Only cache VULNERABLE|SAFE responses. */ | ||
if (value !== RESPONSE_VULNERABLE && value !== RESPONSE_SAFE) { | ||
return; | ||
} | ||
|
||
/* Put in the appropriate cache. */ | ||
switch (config.cache.type) { | ||
case CACHE_TYPES.persistent: | ||
|
@@ -418,7 +440,7 @@ function kvPersistentFname (config, key) { | |
* Using a hash might give us false reports on collisions, but this is | ||
* exceedingly unlikely in typical use cases (a few hundred regexes tops). */ | ||
const hash = crypto.createHash('md5').update(key).digest('hex'); | ||
const fname = path.join(config.cache.persistentDir, `${hash}.json`); | ||
const fname = path.join(config.cache.persistentDir, `${hash}-v2.json`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think we should the number "2" a global |
||
return fname; | ||
} | ||
|
||
|
@@ -463,9 +485,7 @@ function kvGetPersistent (config, key) { | |
let pattern2response = {}; | ||
|
||
function kvPutMemory (key, value) { | ||
if (!pattern2response.hasOwnProperty(key)) { | ||
pattern2response[key] = value; | ||
} | ||
pattern2response[key] = value; | ||
} | ||
|
||
function kvGetMemory (key) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest you declare a
cacheConfig
variable at the top of the function and use it here and above.