Skip to content

Conversation

@nguyents-edu
Copy link

@nguyents-edu nguyents-edu commented Aug 9, 2025

User description

anh xem ổn không


PR Type

Enhancement


Description

  • Add new PHP logging endpoint for speedtest results

  • Capture download/upload speeds, ping, and client IP

  • Store measurements in timestamped log file

  • Return JSON success response


Diagram Walkthrough

flowchart LR
  A["HTTP POST Request"] --> B["logResult.php"]
  B --> C["Parse JSON Data"]
  C --> D["Extract Speed Metrics"]
  D --> E["Log to File"]
  E --> F["Return JSON Response"]
Loading

File Walkthrough

Relevant files
Enhancement
logResult.php
Add speedtest logging endpoint                                                     

logResult.php

  • Create new PHP endpoint to receive speedtest data via POST
  • Extract download/upload speeds, ping, and client IP address
  • Log timestamped results to speedtest_results.log file
  • Return JSON success response to client
[link]   

@qodo-merge-for-open-source
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Sensitive information exposure:
Logging raw client IPs may be considered personal data in some jurisdictions. Evaluate necessity, add masking/anonymization if not required, and document retention. Also, no authentication is enforced on the logging endpoint—if exposed publicly, anyone can spam the log; consider restricting by origin, token, or server-side rate limiting.

⚡ Recommended focus areas for review

Possible BOM

The first character appears to be a BOM (byte order mark). This can cause headers to be sent prematurely in PHP, potentially breaking JSON responses or causing "headers already sent" warnings. Consider removing BOM and ensuring UTF-8 without BOM.

<?php
// logResult.php - lưu kết quả đo vào file log
Input Validation

Incoming JSON fields (dl, ul, ping) are used without validation or type casting. Invalid or malicious input could corrupt logs. Consider validating presence, numeric type, and reasonable ranges before logging.

$dl   = isset($data["dl"])   ? $data["dl"]   : 0; // Mbps
$ul   = isset($data["ul"])   ? $data["ul"]   : 0; // Mbps
$ping = isset($data["ping"]) ? $data["ping"] : 0; // ms
$ip   = $_SERVER['REMOTE_ADDR'];

// Đường dẫn file log
$logFile = __DIR__ . "/speedtest_results.log";

// Chuỗi log
$logEntry = date("Y-m-d H:i:s") . " | IP: $ip | Download: {$dl} Mbps | Upload: {$ul} Mbps | Ping: {$ping} ms" . PHP_EOL;
Concurrency/Permissions

Writing to a shared log file without locking may interleave entries under concurrent requests, and file permissions may fail silently. Consider using FILE_APPEND | LOCK_EX and handling write errors.

// Ghi log
file_put_contents($logFile, $logEntry, FILE_APPEND);

@qodo-merge-for-open-source
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Sanitize values and handle I/O errors

Sanitize and cast values before logging to prevent log injection and ensure
numeric formatting. Also handle write failures and return an error if the log
cannot be written.

logResult.php [16-19]

-$logEntry = date("Y-m-d H:i:s") . " | IP: $ip | Download: {$dl} Mbps | Upload: {$ul} Mbps | Ping: {$ping} ms" . PHP_EOL;
+$ipSafe = filter_var($ip, FILTER_VALIDATE_IP) ?: 'unknown';
+$logEntry = sprintf(
+    "%s | IP: %s | Download: %.2f Mbps | Upload: %.2f Mbps | Ping: %.2f ms%s",
+    date("Y-m-d H:i:s"),
+    $ipSafe,
+    $dl,
+    $ul,
+    $ping,
+    PHP_EOL
+);
 
-// Ghi log
-file_put_contents($logFile, $logEntry, FILE_APPEND);
+if (file_put_contents($logFile, $logEntry, FILE_APPEND | LOCK_EX) === false) {
+    http_response_code(500);
+    header('Content-Type: application/json');
+    echo json_encode(["status" => "error", "message" => "Failed to write log"]);
+    exit;
+}
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion addresses a potential log injection vulnerability by sanitizing the IP address and improves file writing by adding locking (LOCK_EX) and error handling, which are critical for a reliable logging mechanism.

High
Possible issue
Validate and guard input JSON

Validate JSON decoding and required fields before use to avoid PHP warnings and
logging invalid data. Return an error response when input is missing or
malformed to prevent corrupt log entries.

logResult.php [5-9]

-$data = json_decode(file_get_contents("php://input"), true);
+$raw = file_get_contents("php://input");
+$data = json_decode($raw, true);
 
-$dl   = isset($data["dl"])   ? $data["dl"]   : 0; // Mbps
-$ul   = isset($data["ul"])   ? $data["ul"]   : 0; // Mbps
-$ping = isset($data["ping"]) ? $data["ping"] : 0; // ms
+if ($data === null || !is_array($data) || !isset($data['dl'], $data['ul'], $data['ping'])) {
+    http_response_code(400);
+    header('Content-Type: application/json');
+    echo json_encode(["status" => "error", "message" => "Invalid or missing JSON fields"]);
+    exit;
+}
 
+$dl   = floatval($data["dl"]);   // Mbps
+$ul   = floatval($data["ul"]);   // Mbps
+$ping = floatval($data["ping"]); // ms
+
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the input JSON is not validated, which could lead to logging incorrect data or PHP warnings. Adding validation and returning a 400 error is a significant improvement in robustness.

Medium
General
Safely determine client IP

Guard against missing server variables and proxies by safely reading the client
IP. Prefer a validated IP from trusted headers while avoiding spoofing when
headers are untrusted.

logResult.php [10]

-$ip   = $_SERVER['REMOTE_ADDR'];
+$ip = $_SERVER['REMOTE_ADDR'] ?? '';
+// Optional: if behind a trusted proxy, uncomment and validate forwarded header
+// $forwarded = $_SERVER['HTTP_X_FORWARDED_FOR'] ?? '';
+// if ($forwarded) { $parts = array_map('trim', explode(',', $forwarded)); $ip = $parts[0] ?: $ip; }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly points out that $_SERVER['REMOTE_ADDR'] might not be set, and using the null coalescing operator (??) is a good practice. The added comments about proxies are also helpful for future development.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant