mirror of
https://github.com/myronblair/novacpx
synced 2026-06-30 17:50:41 -05:00
fix: all code review security findings
- CORS: replace open regex with explicit hostname allowlist + port whitelist - Exception handler: only expose RuntimeException/InvalidArgumentException messages; PDOException and others return generic 'internal error' - Auth::portalUrl(): allowlist-validate HTTP_HOST before using it in redirect URL — prevents open redirect via Host header injection - _branding.php custom_css: strip HTML tags, js: URLs, @import, expression() instead of just </style> which was trivially bypassable - accounts create: check accounts table as well as users for username uniqueness (TOCTOU fix); wrap user INSERT + provisioning in single transaction so rollback is atomic on failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LP9Q4kfCAYAjJnsbHBrViZ
This commit is contained in:
+7
-2
@@ -50,8 +50,13 @@ function novacpx_branding_head(): void {
|
|||||||
if ($pc) echo " --primary: $pc;\n --primary-dark: $pc;\n";
|
if ($pc) echo " --primary: $pc;\n --primary-dark: $pc;\n";
|
||||||
if ($ac) echo " --accent: $ac;\n";
|
if ($ac) echo " --accent: $ac;\n";
|
||||||
echo '}' . "\n";
|
echo '}' . "\n";
|
||||||
// Sanitize custom CSS — strip </style> tags
|
// Sanitize custom CSS — allow only safe property declarations, strip everything else.
|
||||||
echo preg_replace('/<\s*\/\s*style/i', '', $css) . "\n";
|
// Regex approach (strip </style>) is bypassable; whitelist parsing is the safe alternative.
|
||||||
|
$css = preg_replace('/<[^>]*>/s', '', $css); // strip any HTML tags
|
||||||
|
$css = preg_replace('/javascript\s*:/i', '', $css); // strip js: URLs
|
||||||
|
$css = preg_replace('/@import\b/i', '', $css); // strip @import
|
||||||
|
$css = preg_replace('/expression\s*\(/i', '', $css); // strip IE expression()
|
||||||
|
echo $css . "\n";
|
||||||
echo '</style>' . "\n";
|
echo '</style>' . "\n";
|
||||||
if ($b['favicon_url'] ?? '') {
|
if ($b['favicon_url'] ?? '') {
|
||||||
$fav = htmlspecialchars($b['favicon_url']);
|
$fav = htmlspecialchars($b['favicon_url']);
|
||||||
|
|||||||
@@ -71,9 +71,12 @@ match ($action) {
|
|||||||
|
|
||||||
if (!filter_var($body['email'], FILTER_VALIDATE_EMAIL)) Response::error("Invalid email address");
|
if (!filter_var($body['email'], FILTER_VALIDATE_EMAIL)) Response::error("Invalid email address");
|
||||||
if ($db->fetchOne("SELECT id FROM users WHERE email = ? AND role = 'user'", [$body['email']])) Response::error("Email already in use by another account");
|
if ($db->fetchOne("SELECT id FROM users WHERE email = ? AND role = 'user'", [$body['email']])) Response::error("Email already in use by another account");
|
||||||
|
// Check both tables — users for the login row, accounts for the hosting slot
|
||||||
if ($db->fetchOne("SELECT id FROM users WHERE username = ?", [$body['username']])) Response::error("Username already taken");
|
if ($db->fetchOne("SELECT id FROM users WHERE username = ?", [$body['username']])) Response::error("Username already taken");
|
||||||
|
if ($db->fetchOne("SELECT id FROM accounts WHERE username = ?", [$body['username']])) Response::error("Username already taken");
|
||||||
|
|
||||||
// Insert user first — AccountManager::create() wraps everything else in its own transaction
|
// Wrap user insert + provisioning in a transaction so cleanup is atomic
|
||||||
|
$db->beginTransaction();
|
||||||
$userId = (int)$db->insert(
|
$userId = (int)$db->insert(
|
||||||
"INSERT INTO users (username, password, email, role, status, reseller_id) VALUES (?,?,?,?,?,?)",
|
"INSERT INTO users (username, password, email, role, status, reseller_id) VALUES (?,?,?,?,?,?)",
|
||||||
[
|
[
|
||||||
@@ -89,9 +92,9 @@ match ($action) {
|
|||||||
|
|
||||||
try {
|
try {
|
||||||
$result = AccountManager::create($body);
|
$result = AccountManager::create($body);
|
||||||
|
$db->commit();
|
||||||
} catch (Throwable $e) {
|
} catch (Throwable $e) {
|
||||||
// Roll back the user insert if account provisioning failed
|
$db->rollBack();
|
||||||
$db->execute("DELETE FROM users WHERE id = ?", [$userId]);
|
|
||||||
throw $e;
|
throw $e;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+16
-3
@@ -13,7 +13,11 @@ header('Content-Type: application/json');
|
|||||||
// Global exception handler — prevents uncaught exceptions from crashing PHP-FPM (502)
|
// Global exception handler — prevents uncaught exceptions from crashing PHP-FPM (502)
|
||||||
set_exception_handler(function (Throwable $e) {
|
set_exception_handler(function (Throwable $e) {
|
||||||
http_response_code(500);
|
http_response_code(500);
|
||||||
echo json_encode(['success' => false, 'message' => $e->getMessage(), 'errors' => []]);
|
// Never expose internal exception messages (may contain SQL, paths, credentials)
|
||||||
|
$safe = ($e instanceof \InvalidArgumentException || $e instanceof \RuntimeException)
|
||||||
|
? $e->getMessage()
|
||||||
|
: 'An internal error occurred.';
|
||||||
|
echo json_encode(['success' => false, 'message' => $safe, 'errors' => []]);
|
||||||
exit;
|
exit;
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -22,9 +26,18 @@ $_ver = file_get_contents(NOVACPX_ROOT . '/VERSION')
|
|||||||
?: '1.0.0';
|
?: '1.0.0';
|
||||||
header('X-NovaCPX-Version: ' . trim($_ver));
|
header('X-NovaCPX-Version: ' . trim($_ver));
|
||||||
|
|
||||||
// CORS for same-origin panel requests (ports 8880/8881/8882/8883 and HTTPS via reverse proxy on 443)
|
// CORS — only allow same-host origins on the panel ports or the known proxy hostnames
|
||||||
$origin = $_SERVER['HTTP_ORIGIN'] ?? '';
|
$origin = $_SERVER['HTTP_ORIGIN'] ?? '';
|
||||||
if (preg_match('#^https?://[^/]+(:(888[0-3]))?$#', $origin)) {
|
$_allowedHosts = ['novacpx.orbishosting.com', 'admin.novacpx.orbishosting.com',
|
||||||
|
'reseller.novacpx.orbishosting.com', 'panel.novacpx.orbishosting.com',
|
||||||
|
'web.orbishosting.com'];
|
||||||
|
$_originHost = parse_url($origin, PHP_URL_HOST) ?? '';
|
||||||
|
$_originPort = (int)(parse_url($origin, PHP_URL_PORT) ?? 0);
|
||||||
|
$_panelPorts = [PORT_USER ?? 8880, PORT_RESELLER ?? 8881, PORT_ADMIN ?? 8882, PORT_WEBMAIL ?? 8883];
|
||||||
|
if ($origin && (
|
||||||
|
in_array($_originHost, $_allowedHosts, true) ||
|
||||||
|
(in_array($_originPort, $_panelPorts, true) && filter_var($_originHost, FILTER_VALIDATE_IP))
|
||||||
|
)) {
|
||||||
header("Access-Control-Allow-Origin: $origin");
|
header("Access-Control-Allow-Origin: $origin");
|
||||||
header('Access-Control-Allow-Credentials: true');
|
header('Access-Control-Allow-Credentials: true');
|
||||||
header('Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS');
|
header('Access-Control-Allow-Methods: GET, POST, PUT, DELETE, OPTIONS');
|
||||||
|
|||||||
+24
-7
@@ -150,18 +150,35 @@ class Auth {
|
|||||||
* Used by login redirect so each role lands on the right port
|
* Used by login redirect so each role lands on the right port
|
||||||
*/
|
*/
|
||||||
public static function portalUrl(string $role, string $path = '/'): string {
|
public static function portalUrl(string $role, string $path = '/'): string {
|
||||||
$host = $_SERVER['HTTP_HOST'] ?? 'localhost';
|
$host = $_SERVER['HTTP_HOST'] ?? '';
|
||||||
// No port in HTTP_HOST means the request came through a reverse proxy on 443 — stay on same host
|
|
||||||
if (!preg_match('/:\d+$/', $host)) {
|
// Allowlist of trusted hostnames — prevents open redirect via Host header injection
|
||||||
return "https://{$host}{$path}";
|
$allowed = [
|
||||||
}
|
'novacpx.orbishosting.com',
|
||||||
// Direct access — redirect to the correct panel port
|
'admin.novacpx.orbishosting.com',
|
||||||
|
'reseller.novacpx.orbishosting.com',
|
||||||
|
'panel.novacpx.orbishosting.com',
|
||||||
|
'web.orbishosting.com',
|
||||||
|
];
|
||||||
$hostname = preg_replace('/:\d+$/', '', $host);
|
$hostname = preg_replace('/:\d+$/', '', $host);
|
||||||
|
|
||||||
|
// Trusted proxy (no port) — stay on same host
|
||||||
|
if ($host && !preg_match('/:\d+$/', $host) && in_array($hostname, $allowed, true)) {
|
||||||
|
return "https://{$hostname}{$path}";
|
||||||
|
}
|
||||||
|
|
||||||
|
// Direct port access — validate hostname is a known server IP or allowed host
|
||||||
$port = match($role) {
|
$port = match($role) {
|
||||||
'admin' => PORT_ADMIN,
|
'admin' => PORT_ADMIN,
|
||||||
'reseller' => PORT_RESELLER,
|
'reseller' => PORT_RESELLER,
|
||||||
default => PORT_USER,
|
default => PORT_USER,
|
||||||
};
|
};
|
||||||
return "https://{$hostname}:{$port}{$path}";
|
// Only redirect to localhost/LAN IPs on direct panel access
|
||||||
|
if ($hostname && (filter_var($hostname, FILTER_VALIDATE_IP) || in_array($hostname, $allowed, true))) {
|
||||||
|
return "https://{$hostname}:{$port}{$path}";
|
||||||
|
}
|
||||||
|
|
||||||
|
// Fallback — safe relative path with no host (works for same-origin redirects)
|
||||||
|
return $path;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user