Skip to content

Commit 9430470

Browse files
jorikfonclaude
andcommitted
Fix CDR recording file handling for missing files
Ensure consistent behavior when recording files are missing: - Clear recordingfile, playback_url, and download_url when file doesn't exist - Use empty strings instead of null for better API consistency - Fix test assertions to match new response structure (data.data.pagination) Changes: - DataStructure.php: Clear all recording fields when file_exists() fails - test_43_cdr_security.py: Fix extract_cdr_data() calls and pagination checks 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent 52b6d47 commit 9430470

File tree

2 files changed

+40
-12
lines changed

2 files changed

+40
-12
lines changed

src/PBXCoreREST/Lib/Cdr/DataStructure.php

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ public static function createFromModel($model): array
7878
// Add recording URLs if file exists
7979
// WHY: Provides secure token-based access without exposing file paths
8080
// Two URLs: playback (for inline streaming) and download (for file download)
81+
// IMPORTANT: If file doesn't exist, clear recordingfile, playback_url, and download_url
8182
if (!empty($model->recordingfile) && file_exists($model->recordingfile)) {
8283
$token = self::generatePlaybackToken($model->id);
8384

@@ -87,12 +88,17 @@ public static function createFromModel($model): array
8788
$data['playback_url'] = "/pbxcore/api/v3/cdr:playback?token={$token}";
8889
$data['download_url'] = "/pbxcore/api/v3/cdr:download?token={$token}";
8990
} else {
90-
$data['playback_url'] = null;
91-
$data['download_url'] = null;
91+
// Token generation failed (Redis unavailable?)
92+
// Keep recordingfile but clear URLs
93+
$data['playback_url'] = '';
94+
$data['download_url'] = '';
9295
}
9396
} else {
94-
$data['playback_url'] = null;
95-
$data['download_url'] = null;
97+
// File doesn't exist or recordingfile is empty
98+
// WHY: Consistency - if no file, all 3 fields should be empty
99+
$data['recordingfile'] = '';
100+
$data['playback_url'] = '';
101+
$data['download_url'] = '';
96102
}
97103

98104
// Apply OpenAPI schema formatting to convert types automatically

tests/api/test_43_cdr_security.py

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,21 @@
1515
from datetime import datetime, timedelta
1616

1717

18+
def extract_cdr_data(response):
19+
"""Extract CDR data and pagination from response handling new format."""
20+
data_wrapper = response.get('data', {})
21+
22+
if isinstance(data_wrapper, dict):
23+
if 'records' in data_wrapper and 'pagination' in data_wrapper:
24+
return data_wrapper['records'], data_wrapper['pagination']
25+
26+
# Fallback for legacy format
27+
if isinstance(data_wrapper, list):
28+
return data_wrapper, response.get('pagination', {})
29+
30+
return [], {}
31+
32+
1833
@pytest.mark.security
1934
class TestCDRSQLInjectionProtection:
2035
"""
@@ -48,7 +63,8 @@ def test_sort_field_sql_injection_drop_table(self, api_client):
4863

4964
# Critical: Table must still exist (pagination works)
5065
assert 'data' in data, "Response missing data field"
51-
assert 'pagination' in data, \
66+
data_wrapper = data['data']
67+
assert 'pagination' in data_wrapper, \
5268
"CRITICAL: Table compromised - pagination missing"
5369

5470
# Verify no error messages about SQL syntax
@@ -289,7 +305,7 @@ def test_auto_date_range_no_filters(self, api_client):
289305
f"Expected 200, got {response.status_code}"
290306

291307
data = response.json()
292-
cdr_data = data.get('data', [])
308+
cdr_data, pagination = extract_cdr_data(data)
293309

294310
# Verify returned records are recent (within auto-applied range)
295311
if cdr_data:
@@ -331,7 +347,7 @@ def test_limit_parameter_exceeds_maximum(self, api_client):
331347
f"Expected 200, got {response.status_code}"
332348

333349
data = response.json()
334-
cdr_data = data.get('data', [])
350+
cdr_data, pagination = extract_cdr_data(data)
335351

336352
# Should cap at 1000
337353
assert len(cdr_data) <= 1000, \
@@ -357,7 +373,7 @@ def test_negative_limit_rejected_or_defaulted(self, api_client):
357373
# Either reject or use safe default
358374
if response.status_code == 200:
359375
data = response.json()
360-
cdr_data = data.get('data', [])
376+
cdr_data, pagination = extract_cdr_data(data)
361377
# Should use safe default, not negative value
362378
assert len(cdr_data) >= 0, \
363379
"Negative limit should not return negative records"
@@ -459,7 +475,8 @@ def test_order_parameter_sql_injection(self, api_client):
459475
data = response.json()
460476
# Table must still exist
461477
assert 'data' in data, "Response should contain data"
462-
assert 'pagination' in data, \
478+
data_wrapper = data['data']
479+
assert 'pagination' in data_wrapper, \
463480
"Table should still exist (pagination present)"
464481

465482
def test_order_parameter_valid_directions(self, api_client):
@@ -511,9 +528,14 @@ def test_large_result_set_pagination_required(self, api_client):
511528
f"Expected 200, got {response.status_code}"
512529

513530
data = response.json()
514-
pagination = data.get('pagination', {})
515531

516-
# Should have pagination
532+
# API should always include pagination metadata
533+
data_wrapper = data['data']
534+
assert 'pagination' in data_wrapper, "Response should include pagination"
535+
pagination = data_wrapper.get('pagination', {})
536+
assert isinstance(pagination, dict), "Pagination should be a dictionary"
537+
538+
# Should have pagination metadata
517539
assert 'total' in pagination, "Pagination should include total"
518540
assert 'limit' in pagination, "Pagination should include limit"
519541
assert 'hasMore' in pagination, "Pagination should include hasMore"
@@ -539,7 +561,7 @@ def test_grouped_pagination_performance(self, api_client):
539561
f"Expected 200, got {response.status_code}"
540562

541563
data = response.json()
542-
cdr_data = data.get('data', [])
564+
cdr_data, pagination = extract_cdr_data(data)
543565

544566
# Each group should have linkedid
545567
for group in cdr_data[:5]: # Check first 5 groups

0 commit comments

Comments
 (0)