Skip to content

Commit e1bb7df

Browse files
committed
test: Update tests for security hardening changes
Update tests to validate the security improvements: - config_test.go: Expect only /var/log in allowed paths (not /tmp or ./logs) - main_test.go: Convert to security validation tests that verify /tmp access is denied - log_monitor.go: Fix MinSize filter to only apply to files, not directories These changes ensure tests validate the security hardening rather than expecting the insecure behavior.
1 parent c7a2c6f commit e1bb7df

File tree

3 files changed

+48
-48
lines changed

3 files changed

+48
-48
lines changed

mcp-servers/go/system-monitor-server/cmd/server/main_test.go

Lines changed: 22 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -294,10 +294,11 @@ func TestHandleTailLogs(t *testing.T) {
294294
t.Error("Expected error for missing file_path")
295295
}
296296

297-
// Test with valid file path (use a temp file in /tmp)
297+
// SECURITY TEST: Verify that /tmp is not allowed (security hardening)
298+
// Create a temp file in /tmp to test that access is properly denied
298299
tmpFile, err := os.CreateTemp("/tmp", "test-log-*.txt")
299300
if err != nil {
300-
t.Fatalf("Failed to create temp file: %v", err)
301+
t.Skip("Cannot create temp file for security test")
301302
}
302303
defer os.Remove(tmpFile.Name())
303304
defer tmpFile.Close()
@@ -319,23 +320,26 @@ func TestHandleTailLogs(t *testing.T) {
319320
t.Fatalf("handleTailLogs failed: %v", err)
320321
}
321322

322-
if result.IsError {
323-
if len(result.Content) > 0 {
324-
if textContent, ok := mcp.AsTextContent(result.Content[0]); ok {
325-
t.Errorf("Expected success, got error: %s", textContent.Text)
326-
} else {
327-
t.Error("Expected success, got error")
323+
// SECURITY: Expect error because /tmp is not in allowed paths
324+
if !result.IsError {
325+
t.Error("Expected error for /tmp access (security hardening), but got success")
326+
}
327+
328+
// Verify the error message mentions path validation
329+
if len(result.Content) > 0 {
330+
if textContent, ok := mcp.AsTextContent(result.Content[0]); ok {
331+
if !strings.Contains(textContent.Text, "file path validation failed") &&
332+
!strings.Contains(textContent.Text, "not in allowed directories") {
333+
t.Errorf("Expected path validation error, got: %s", textContent.Text)
328334
}
329-
} else {
330-
t.Error("Expected success, got error")
331335
}
332336
}
333337
}
334338

335339
func TestHandleGetDiskUsage(t *testing.T) {
336340
ctx := context.Background()
337341

338-
// Test with /tmp directory (allowed path)
342+
// SECURITY TEST: Verify that /tmp is not allowed (security hardening)
339343
req := mcp.CallToolRequest{
340344
Params: mcp.CallToolParams{
341345
Arguments: map[string]interface{}{
@@ -350,30 +354,19 @@ func TestHandleGetDiskUsage(t *testing.T) {
350354
t.Fatalf("handleGetDiskUsage failed: %v", err)
351355
}
352356

353-
if result.IsError {
354-
if len(result.Content) > 0 {
355-
if textContent, ok := mcp.AsTextContent(result.Content[0]); ok {
356-
t.Errorf("Expected success, got error: %s", textContent.Text)
357-
} else {
358-
t.Error("Expected success, got error")
359-
}
360-
} else {
361-
t.Error("Expected success, got error")
362-
}
357+
// SECURITY: Expect error because /tmp is not in allowed paths
358+
if !result.IsError {
359+
t.Error("Expected error for /tmp access (security hardening), but got success")
363360
}
364361

365-
// Test that result contains valid JSON
362+
// Verify the error message mentions path validation
366363
if len(result.Content) > 0 {
367364
if textContent, ok := mcp.AsTextContent(result.Content[0]); ok {
368-
var diskUsage map[string]interface{}
369-
if err := json.Unmarshal([]byte(textContent.Text), &diskUsage); err != nil {
370-
t.Errorf("Result should be valid JSON: %v", err)
365+
if !strings.Contains(textContent.Text, "failed to get disk usage") &&
366+
!strings.Contains(textContent.Text, "not in allowed directories") {
367+
t.Errorf("Expected path validation error, got: %s", textContent.Text)
371368
}
372-
} else {
373-
t.Error("Expected text content in result")
374369
}
375-
} else {
376-
t.Error("Expected content in result")
377370
}
378371
}
379372

mcp-servers/go/system-monitor-server/internal/config/config_test.go

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -70,27 +70,24 @@ func TestDefaultConfig(t *testing.T) {
7070
if config.LogMonitoring.MaxTailLines != 1000 {
7171
t.Errorf("Expected MaxTailLines 1000, got %d", config.LogMonitoring.MaxTailLines)
7272
}
73-
if len(config.LogMonitoring.AllowedPaths) != 3 {
74-
t.Errorf("Expected 3 allowed paths, got %d", len(config.LogMonitoring.AllowedPaths))
73+
// SECURITY: Only /var/log should be allowed by default (removed /tmp and ./logs)
74+
if len(config.LogMonitoring.AllowedPaths) != 1 {
75+
t.Errorf("Expected 1 allowed path, got %d", len(config.LogMonitoring.AllowedPaths))
7576
}
76-
expectedPaths := []string{"/var/log", "/tmp", "./logs"}
77-
for i, expectedPath := range expectedPaths {
78-
if config.LogMonitoring.AllowedPaths[i] != expectedPath {
79-
t.Errorf("Expected allowed path %s, got %s", expectedPath, config.LogMonitoring.AllowedPaths[i])
80-
}
77+
if config.LogMonitoring.AllowedPaths[0] != "/var/log" {
78+
t.Errorf("Expected allowed path /var/log, got %s", config.LogMonitoring.AllowedPaths[0])
8179
}
8280
if config.LogMonitoring.FollowTimeout != 30*time.Second {
8381
t.Errorf("Expected FollowTimeout 30s, got %v", config.LogMonitoring.FollowTimeout)
8482
}
8583

8684
// Test Security config
87-
if len(config.Security.AllowedPaths) != 3 {
88-
t.Errorf("Expected 3 security allowed paths, got %d", len(config.Security.AllowedPaths))
85+
// SECURITY: Only /var/log should be allowed by default (removed /tmp and ./logs)
86+
if len(config.Security.AllowedPaths) != 1 {
87+
t.Errorf("Expected 1 security allowed path, got %d", len(config.Security.AllowedPaths))
8988
}
90-
for i, expectedPath := range expectedPaths {
91-
if config.Security.AllowedPaths[i] != expectedPath {
92-
t.Errorf("Expected security allowed path %s, got %s", expectedPath, config.Security.AllowedPaths[i])
93-
}
89+
if config.Security.AllowedPaths[0] != "/var/log" {
90+
t.Errorf("Expected security allowed path /var/log, got %s", config.Security.AllowedPaths[0])
9491
}
9592
if config.Security.MaxFileSize != 100*1024*1024 {
9693
t.Errorf("Expected MaxFileSize 100MB, got %d", config.Security.MaxFileSize)

mcp-servers/go/system-monitor-server/internal/monitor/log_monitor.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -402,13 +402,12 @@ func (lm *LogMonitor) GetDiskUsage(ctx context.Context, req *types.DiskUsageRequ
402402
return nil
403403
}
404404

405-
// Check minimum size
406-
if req.MinSize > 0 && info.Size() < req.MinSize {
407-
return nil
408-
}
409-
410-
// Check file type filter
405+
// Check file type filter (only for files) - must check before size filter
411406
if len(req.FileTypes) > 0 {
407+
if info.IsDir() {
408+
// Skip directories when filtering by file type
409+
return nil
410+
}
412411
ext := strings.ToLower(filepath.Ext(path))
413412
found := false
414413
for _, fileType := range req.FileTypes {
@@ -422,6 +421,17 @@ func (lm *LogMonitor) GetDiskUsage(ctx context.Context, req *types.DiskUsageRequ
422421
}
423422
}
424423

424+
// Check minimum size (only for files, not directories)
425+
if req.MinSize > 0 {
426+
if info.IsDir() {
427+
// Skip directories when filtering by size
428+
return nil
429+
}
430+
if info.Size() < req.MinSize {
431+
return nil
432+
}
433+
}
434+
425435
item := types.DiskUsageItem{
426436
Path: path,
427437
Size: info.Size(),

0 commit comments

Comments
 (0)