Skip to content

Conversation

@alanEG
Copy link

@alanEG alanEG commented Jul 14, 2025

Security improvement

@Reqrefusion
Copy link
Member

@alanEG Thanks for your initial contribution. I had to do something like this because of cors. Actually, this code doesn't do anything. But I don't believe there should be any security for this.

@alanEG
Copy link
Author

alanEG commented Aug 2, 2025

Hi, I added the whitelist to prevent SSRF and file system access via file://, which is possible with the original code. Even if it's only used for CORS, adding basic security helps prevent the server from being exposed to potential attacks.

@chennes chennes requested a review from Copilot August 25, 2025 19:46
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements security improvements to the XML proxy by adding URL validation and whitelisting functionality to prevent potential SSRF (Server-Side Request Forgery) attacks.

  • Introduces a whitelist-based approach to restrict allowed domains
  • Adds URL safety validation function that checks both hostname and IP address against the whitelist
  • Replaces direct URL validation with comprehensive security checks

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

proxy-xml.php Outdated
return true;
}

$ip = gethostbyname($host);
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gethostbyname() function can be exploited for DNS rebinding attacks and may resolve to internal IP addresses. Consider using a more secure approach that validates against private IP ranges (127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16) and blocks localhost resolution.

Copilot uses AI. Check for mistakes.
proxy-xml.php Outdated
if (is_url_safe($url, $whitelist)) {
$response = file_get_contents($url);
header('Content-Type: application/xml');
echo $response;
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using file_get_contents() without timeout or context options can lead to denial of service attacks through slow responses or large file downloads. Consider adding a stream context with timeout and size limits, or use cURL with proper configuration.

Suggested change
echo $response;
// Set a timeout of 5 seconds
$context = stream_context_create([
'http' => [
'timeout' => 5
]
]);
// Limit response size to 2MB
$maxSize = 2 * 1024 * 1024; // 2MB
$response = '';
$handle = @fopen($url, 'r', false, $context);
if ($handle) {
while (!feof($handle) && strlen($response) < $maxSize) {
$response .= fread($handle, 8192);
}
fclose($handle);
if (strlen($response) >= $maxSize) {
header('HTTP/1.1 413 Payload Too Large');
echo "Response too large";
} else {
header('Content-Type: application/xml');
echo $response;
}
} else {
header('HTTP/1.1 502 Bad Gateway');
echo "Failed to fetch URL";
}

Copilot uses AI. Check for mistakes.
@maxwxyz
Copy link
Contributor

maxwxyz commented Oct 26, 2025

is this good to go?

alanEG and others added 3 commits October 26, 2025 14:02
Co-authored-by: Copilot <[email protected]>
Enhanced URL safety checks by validating the scheme and added error handling for missing URLs.
@alanEG
Copy link
Author

alanEG commented Oct 26, 2025

Hey @maxwxyz,

Yeah, I made a few changes, and I would say it’s looking good to go.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants