Skip to content

Commit 473c149

Browse files
ironashramironashram
committed
feat: prevent hash collisions, add file locking and cache unit test
Signed-off-by: Michele Palazzi <sysdadmin@m1k.cloud>
1 parent 0f7a62f commit 473c149

3 files changed

Lines changed: 249 additions & 21 deletions

File tree

‎src/cache.php‎

Lines changed: 56 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,24 @@
1515
/**
1616
* Generate a cache key for a user's request
1717
*
18+
* Uses structured JSON format to prevent hash collisions between different
19+
* user/options combinations that could produce the same concatenated string.
20+
*
1821
* @param string $user GitHub username
1922
* @param array $options Additional options that affect the stats (mode, exclude_days, starting_year)
2023
* @return string Cache key (filename-safe)
2124
*/
2225
function getCacheKey(string $user, array $options = []): string
2326
{
24-
// Normalize options
2527
ksort($options);
26-
$optionsString = json_encode($options);
27-
return hash("sha256", $user . $optionsString);
28+
try {
29+
$keyData = json_encode(["user" => $user, "options" => $options], JSON_THROW_ON_ERROR);
30+
} catch (JsonException $e) {
31+
// Fallback to simple concatenation if JSON encoding fails
32+
error_log("Cache key JSON encoding failed: " . $e->getMessage());
33+
$keyData = $user . serialize($options);
34+
}
35+
return hash("sha256", $keyData);
2836
}
2937

3038
/**
@@ -68,17 +76,32 @@ function getCachedStats(string $user, array $options = [], int $maxAge = CACHE_D
6876
return null;
6977
}
7078

71-
$fileAge = time() - filemtime($filePath);
79+
$mtime = @filemtime($filePath);
80+
if ($mtime === false) {
81+
return null;
82+
}
83+
84+
$fileAge = time() - $mtime;
7285
if ($fileAge > $maxAge) {
73-
// Cache expired, delete the file
74-
if (file_exists($filePath)) {
75-
unlink($filePath);
76-
}
86+
@unlink($filePath);
7787
return null;
7888
}
7989

80-
$contents = file_get_contents($filePath);
81-
if ($contents === false) {
90+
$handle = @fopen($filePath, "r");
91+
if ($handle === false) {
92+
return null;
93+
}
94+
95+
if (!flock($handle, LOCK_SH)) {
96+
fclose($handle);
97+
return null;
98+
}
99+
100+
$contents = @stream_get_contents($handle);
101+
flock($handle, LOCK_UN);
102+
fclose($handle);
103+
104+
if ($contents === false || $contents === "") {
82105
return null;
83106
}
84107

@@ -101,18 +124,26 @@ function getCachedStats(string $user, array $options = [], int $maxAge = CACHE_D
101124
function setCachedStats(string $user, array $options, array $stats): bool
102125
{
103126
if (!ensureCacheDir()) {
127+
error_log("Failed to create cache directory: " . CACHE_DIR);
104128
return false;
105129
}
106130

107131
$key = getCacheKey($user, $options);
108132
$filePath = getCacheFilePath($key);
109133

110-
$data = json_encode($stats, JSON_PRETTY_PRINT);
134+
$data = json_encode($stats);
111135
if ($data === false) {
136+
error_log("Failed to encode stats to JSON for user: " . $user);
112137
return false;
113138
}
114139

115-
return file_put_contents($filePath, $data, LOCK_EX) !== false;
140+
$result = file_put_contents($filePath, $data, LOCK_EX);
141+
if ($result === false) {
142+
error_log("Failed to write cache file: " . $filePath);
143+
return false;
144+
}
145+
146+
return true;
116147
}
117148

118149
/**
@@ -135,9 +166,13 @@ function clearExpiredCache(int $maxAge = CACHE_DURATION): int
135166
}
136167

137168
foreach ($files as $file) {
138-
$fileAge = time() - filemtime($file);
169+
$mtime = @filemtime($file);
170+
if ($mtime === false) {
171+
continue;
172+
}
173+
$fileAge = time() - $mtime;
139174
if ($fileAge > $maxAge) {
140-
if (file_exists($file) && unlink($file)) {
175+
if (@unlink($file)) {
141176
$deleted++;
142177
}
143178
}
@@ -149,22 +184,25 @@ function clearExpiredCache(int $maxAge = CACHE_DURATION): int
149184
/**
150185
* Clear cache for a specific user
151186
*
187+
* Note: This function only clears the cache for the user with empty/default options.
188+
* Cache entries with non-empty options (starting_year, mode, exclude_days) will NOT
189+
* be cleared. This is a limitation of the hash-based cache key system - we cannot
190+
* enumerate all possible option combinations without storing additional metadata.
191+
*
152192
* @param string $user GitHub username
153-
* @return bool True if cache was cleared
193+
* @return bool True if cache was cleared (or didn't exist)
154194
*/
155195
function clearUserCache(string $user): bool
156196
{
157197
if (!is_dir(CACHE_DIR)) {
158198
return true;
159199
}
160200

161-
// Since we use a hash, we need to check all files
162-
// For simplicity, just clear the cache with empty options
163201
$key = getCacheKey($user, []);
164202
$filePath = getCacheFilePath($key);
165203

166204
if (file_exists($filePath)) {
167-
return unlink($filePath);
205+
return @unlink($filePath);
168206
}
169207

170208
return true;

‎src/index.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@
5151

5252
if ($cachedStats !== null) {
5353
// Use cached stats - instant response!
54-
renderOutput($cachedStats);
54+
$stats = $cachedStats;
5555
} else {
5656
// Fetch fresh data from GitHub API
5757
$contributionGraphs = getContributionGraphs($user, $startingYear);
@@ -67,9 +67,9 @@
6767

6868
// Cache the stats for 24 hours
6969
setCachedStats($user, $cacheOptions, $stats);
70-
71-
renderOutput($stats);
7270
}
71+
72+
renderOutput($stats);
7373
} catch (InvalidArgumentException | AssertionError $error) {
7474
error_log("Error {$error->getCode()}: {$error->getMessage()}");
7575
if ($error->getCode() >= 500) {

‎tests/CacheTest.php‎

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
use PHPUnit\Framework\TestCase;
6+
7+
// load functions
8+
require_once dirname(__DIR__, 1) . "/vendor/autoload.php";
9+
require_once "src/cache.php";
10+
11+
final class CacheTest extends TestCase
12+
{
13+
private string $testCacheDir;
14+
15+
protected function setUp(): void
16+
{
17+
// Use a temp directory for tests to avoid interfering with production cache
18+
$this->testCacheDir = sys_get_temp_dir() . "/github-streak-cache-test-" . uniqid();
19+
if (!is_dir($this->testCacheDir)) {
20+
mkdir($this->testCacheDir, 0755, true);
21+
}
22+
}
23+
24+
protected function tearDown(): void
25+
{
26+
// Clean up test cache directory
27+
if (is_dir($this->testCacheDir)) {
28+
$files = glob($this->testCacheDir . "/*.json");
29+
if ($files) {
30+
foreach ($files as $file) {
31+
@unlink($file);
32+
}
33+
}
34+
@rmdir($this->testCacheDir);
35+
}
36+
}
37+
38+
/**
39+
* Test cache key generation produces consistent results
40+
*/
41+
public function testCacheKeyConsistency(): void
42+
{
43+
$key1 = getCacheKey("testuser", ["mode" => "weekly"]);
44+
$key2 = getCacheKey("testuser", ["mode" => "weekly"]);
45+
$this->assertEquals($key1, $key2, "Same inputs should produce same cache key");
46+
}
47+
48+
/**
49+
* Test cache key generation produces different results for different inputs
50+
*/
51+
public function testCacheKeyDifferentInputs(): void
52+
{
53+
$key1 = getCacheKey("user1", []);
54+
$key2 = getCacheKey("user2", []);
55+
$this->assertNotEquals($key1, $key2, "Different users should produce different cache keys");
56+
57+
$key3 = getCacheKey("testuser", ["mode" => "weekly"]);
58+
$key4 = getCacheKey("testuser", ["mode" => "daily"]);
59+
$this->assertNotEquals($key3, $key4, "Different options should produce different cache keys");
60+
}
61+
62+
/**
63+
* Test that cache key prevents hash collisions
64+
* e.g., user "ab" with options containing "cd" should not collide with user "abc" with options containing "d"
65+
*/
66+
public function testCacheKeyNoCollisions(): void
67+
{
68+
// This tests the fix for the hash collision vulnerability
69+
$key1 = getCacheKey("ab", ["option" => "cd"]);
70+
$key2 = getCacheKey("abc", ["option" => "d"]);
71+
$this->assertNotEquals($key1, $key2, "Similar concatenated strings should not produce same hash");
72+
73+
$key3 = getCacheKey("ab", ["x" => "cd"]);
74+
$key4 = getCacheKey("abcd", []);
75+
$this->assertNotEquals($key3, $key4, "User + options should not collide with user alone");
76+
}
77+
78+
/**
79+
* Test cache key generation sorts options for consistency
80+
*/
81+
public function testCacheKeyOptionOrdering(): void
82+
{
83+
$key1 = getCacheKey("testuser", ["a" => "1", "b" => "2"]);
84+
$key2 = getCacheKey("testuser", ["b" => "2", "a" => "1"]);
85+
$this->assertEquals($key1, $key2, "Option order should not affect cache key");
86+
}
87+
88+
/**
89+
* Test cache key is filename-safe (SHA256 hex)
90+
*/
91+
public function testCacheKeyFormat(): void
92+
{
93+
$key = getCacheKey("testuser", ["mode" => "weekly"]);
94+
$this->assertMatchesRegularExpression("/^[a-f0-9]{64}$/", $key, "Cache key should be 64-character hex string");
95+
}
96+
97+
/**
98+
* Test cache file path generation
99+
*/
100+
public function testGetCacheFilePath(): void
101+
{
102+
$key = "abc123";
103+
$path = getCacheFilePath($key);
104+
$this->assertStringEndsWith("/cache/abc123.json", $path);
105+
}
106+
107+
/**
108+
* Test setCachedStats and getCachedStats roundtrip
109+
*/
110+
public function testCacheRoundtrip(): void
111+
{
112+
$user = "roundtripuser";
113+
$options = ["mode" => "weekly", "starting_year" => 2020];
114+
$stats = [
115+
"totalContributions" => 100,
116+
"currentStreak" => ["start" => "2024-01-01", "end" => "2024-01-10", "length" => 10],
117+
"longestStreak" => ["start" => "2023-06-01", "end" => "2023-07-15", "length" => 45],
118+
"firstContribution" => "2020-01-15",
119+
];
120+
121+
// Write to cache
122+
$result = setCachedStats($user, $options, $stats);
123+
$this->assertTrue($result, "setCachedStats should return true on success");
124+
125+
// Read back from cache
126+
$cached = getCachedStats($user, $options);
127+
$this->assertNotNull($cached, "getCachedStats should return cached data");
128+
$this->assertEquals($stats, $cached, "Cached data should match original");
129+
}
130+
131+
/**
132+
* Test getCachedStats returns null for non-existent cache
133+
*/
134+
public function testGetCachedStatsNotFound(): void
135+
{
136+
$result = getCachedStats("nonexistentuser12345", []);
137+
$this->assertNull($result, "getCachedStats should return null for non-existent cache");
138+
}
139+
140+
/**
141+
* Test setCachedStats handles invalid data gracefully
142+
*/
143+
public function testSetCachedStatsWithEmptyStats(): void
144+
{
145+
$result = setCachedStats("emptyuser", [], []);
146+
$this->assertTrue($result, "setCachedStats should handle empty stats array");
147+
148+
$cached = getCachedStats("emptyuser", []);
149+
$this->assertEquals([], $cached, "Empty stats should be cached and retrieved");
150+
}
151+
152+
/**
153+
* Test clearUserCache clears cache for user with default options
154+
*/
155+
public function testClearUserCache(): void
156+
{
157+
$user = "clearableuser";
158+
$stats = ["totalContributions" => 50];
159+
160+
// Set cache
161+
setCachedStats($user, [], $stats);
162+
$this->assertNotNull(getCachedStats($user, []));
163+
164+
// Clear cache
165+
$result = clearUserCache($user);
166+
$this->assertTrue($result);
167+
168+
// Verify cleared
169+
$this->assertNull(getCachedStats($user, []));
170+
}
171+
172+
/**
173+
* Test clearUserCache returns true for non-existent user
174+
*/
175+
public function testClearUserCacheNonExistent(): void
176+
{
177+
$result = clearUserCache("definitelynotauser999");
178+
$this->assertTrue($result, "clearUserCache should return true for non-existent cache");
179+
}
180+
181+
/**
182+
* Test ensureCacheDir creates directory
183+
*/
184+
public function testEnsureCacheDir(): void
185+
{
186+
$result = ensureCacheDir();
187+
$this->assertTrue($result);
188+
$this->assertTrue(is_dir(CACHE_DIR));
189+
}
190+
}

0 commit comments

Comments
 (0)