Skip to content

Commit ecb1b58

Browse files
danslodanslo
authored
Merge pull request #39 from sansecio/whitelist-remote-addr-only
fix: match whitelist against REMOTE_ADDR only
2 parents a499ef7 + 7621ee4 commit ecb1b58

6 files changed

Lines changed: 168 additions & 6 deletions

File tree

‎Model/IP.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,12 @@ private function isPrivateIP(string $ip): bool
2020
return filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE) === false;
2121
}
2222

23+
public function getRemoteAddr(): ?string
24+
{
25+
$remote = isset($_SERVER['REMOTE_ADDR']) ? trim((string) $_SERVER['REMOTE_ADDR']) : '';
26+
return $remote === '' ? null : $remote;
27+
}
28+
2329
public function collectRequestIPs(): array
2430
{
2531
if ($this->requestIPs === null) {

‎Plugin/Shield.php‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,11 @@ private function isRequestWhitelisted(): bool
5454
if (empty($whitelisted)) {
5555
return false;
5656
}
57-
foreach ($this->ip->collectRequestIPs() as $ip) {
58-
if (in_array($ip, $whitelisted, true)) {
59-
return true;
60-
}
57+
$remoteAddr = $this->ip->getRemoteAddr();
58+
if ($remoteAddr === null) {
59+
return false;
6160
}
62-
return false;
61+
return in_array($remoteAddr, $whitelisted, true);
6362
}
6463

6564
private function getAccessDeniedResponse(): ResponseInterface

‎README.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,12 @@ bin/magento sansec:shield:sync-rules
1515

1616
You can configure your license key and other settings via System → Configuration → Security → Sansec Shield.
1717

18+
### Whitelisted IP addresses
19+
20+
IPs listed under *Whitelisted IP Addresses* bypass all Shield checks. Matching is performed against the connecting peer (`REMOTE_ADDR`) only; proxy-forwarded headers such as `X-Forwarded-For` and `CF-Connecting-IP` are intentionally ignored because they are client-controlled and can be spoofed.
21+
22+
If your store sits behind a reverse proxy or CDN, configure your webserver to rewrite the trusted proxy header into `REMOTE_ADDR` ([`ngx_http_realip_module`](https://nginx.org/en/docs/http/ngx_http_realip_module.html) on nginx, [`mod_remoteip`](https://httpd.apache.org/docs/2.4/mod/mod_remoteip.html) on Apache). Once `REMOTE_ADDR` reflects the real client IP, the whitelist will match it correctly.
23+
1824
## Testing & live reports
1925

2026
Test it by visiting your store and add `?SANSEC-SHIELD-TEST` to your URL, it should give you "permission denied". You'll see your first blocked attack appear instantly on your [Shield Dashboard](https://dashboard.sansec.io/d/account/shield). If you do not want reports, you can disable it with:

‎Test/Model/IPTest.php‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
<?php
2+
3+
namespace Sansec\Shield\Test\Model;
4+
5+
use PHPUnit\Framework\TestCase;
6+
use Sansec\Shield\Model\IP;
7+
8+
class IPTest extends TestCase
9+
{
10+
/** @var array */
11+
private $serverBackup;
12+
13+
protected function setUp(): void
14+
{
15+
$this->serverBackup = $_SERVER;
16+
}
17+
18+
protected function tearDown(): void
19+
{
20+
$_SERVER = $this->serverBackup;
21+
}
22+
23+
public function testGetRemoteAddrReturnsRemoteAddr()
24+
{
25+
$_SERVER['REMOTE_ADDR'] = '203.0.113.42';
26+
$this->assertSame('203.0.113.42', (new IP())->getRemoteAddr());
27+
}
28+
29+
public function testGetRemoteAddrIgnoresForwardedHeaders()
30+
{
31+
unset($_SERVER['REMOTE_ADDR']);
32+
$_SERVER['HTTP_X_FORWARDED_FOR'] = '203.0.113.42';
33+
$_SERVER['HTTP_CF_CONNECTING_IP'] = '203.0.113.42';
34+
$this->assertNull((new IP())->getRemoteAddr());
35+
}
36+
37+
public function testGetRemoteAddrReturnsNullWhenBlank()
38+
{
39+
$_SERVER['REMOTE_ADDR'] = " \t";
40+
$this->assertNull((new IP())->getRemoteAddr());
41+
}
42+
}

‎Test/Plugin/ShieldTest.php‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
<?php
2+
3+
namespace Magento\Framework\App\Response {
4+
if (!class_exists(HttpFactory::class, false)) {
5+
class HttpFactory
6+
{
7+
public function create()
8+
{
9+
}
10+
}
11+
}
12+
}
13+
14+
namespace Magento\Framework\View\Element {
15+
if (!class_exists(TemplateFactory::class, false)) {
16+
class TemplateFactory
17+
{
18+
public function create()
19+
{
20+
}
21+
}
22+
}
23+
}
24+
25+
namespace Sansec\Shield\Test\Plugin {
26+
27+
use Magento\Framework\App\FrontControllerInterface;
28+
use Magento\Framework\App\Response\HttpFactory as HttpResponseFactory;
29+
use Magento\Framework\View\Element\TemplateFactory;
30+
use PHPUnit\Framework\MockObject\Rule\InvocationOrder;
31+
use PHPUnit\Framework\TestCase;
32+
use Sansec\Shield\Model\Config;
33+
use Sansec\Shield\Model\IP;
34+
use Sansec\Shield\Model\Report;
35+
use Sansec\Shield\Model\Waf;
36+
use Sansec\Shield\Plugin\Shield;
37+
use Sansec\Shield\Test\RequestStub;
38+
39+
class ShieldTest extends TestCase
40+
{
41+
/** @var array */
42+
private $serverBackup;
43+
44+
protected function setUp(): void
45+
{
46+
$this->serverBackup = $_SERVER;
47+
}
48+
49+
protected function tearDown(): void
50+
{
51+
$_SERVER = $this->serverBackup;
52+
}
53+
54+
private function buildPlugin(array $whitelistedIps, InvocationOrder $expectedWafCalls): Shield
55+
{
56+
$config = $this->createMock(Config::class);
57+
$config->method('isEnabled')->willReturn(true);
58+
$config->method('getWhitelistedIps')->willReturn($whitelistedIps);
59+
60+
$waf = $this->createMock(Waf::class);
61+
$waf->expects($expectedWafCalls)->method('matchRequest')->willReturn([]);
62+
63+
return new Shield(
64+
$config,
65+
$waf,
66+
$this->createMock(Report::class),
67+
new IP(),
68+
$this->createMock(HttpResponseFactory::class),
69+
$this->createMock(TemplateFactory::class)
70+
);
71+
}
72+
73+
private function dispatch(Shield $plugin): bool
74+
{
75+
$proceedCalled = false;
76+
$plugin->aroundDispatch(
77+
$this->createMock(FrontControllerInterface::class),
78+
function () use (&$proceedCalled) {
79+
$proceedCalled = true;
80+
},
81+
new RequestStub()
82+
);
83+
return $proceedCalled;
84+
}
85+
86+
public function testRemoteAddrInWhitelistBypassesWaf()
87+
{
88+
$_SERVER['REMOTE_ADDR'] = '203.0.113.42';
89+
$plugin = $this->buildPlugin(['203.0.113.42'], $this->never());
90+
$this->assertTrue($this->dispatch($plugin));
91+
}
92+
93+
public function testForwardedHeaderInWhitelistDoesNotBypassWaf()
94+
{
95+
$_SERVER['REMOTE_ADDR'] = '198.51.100.1';
96+
$_SERVER['HTTP_X_FORWARDED_FOR'] = '203.0.113.42';
97+
$_SERVER['HTTP_CF_CONNECTING_IP'] = '203.0.113.42';
98+
$plugin = $this->buildPlugin(['203.0.113.42'], $this->once());
99+
$this->dispatch($plugin);
100+
}
101+
102+
public function testEmptyWhitelistDoesNotBypassWaf()
103+
{
104+
$_SERVER['REMOTE_ADDR'] = '203.0.113.42';
105+
$plugin = $this->buildPlugin([], $this->once());
106+
$this->dispatch($plugin);
107+
}
108+
}
109+
}

‎etc/adminhtml/system.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
</field>
2323
<field id="whitelisted_ips" translate="label comment" type="textarea" sortOrder="30" showInDefault="1" showInWebsite="0" showInStore="0">
2424
<label>Whitelisted IP Addresses</label>
25-
<comment>One IP address per line. Requests from these IPs bypass Shield checks. Ensure client IP headers are correctly set by your proxy/webserver.</comment>
25+
<comment>One IP address per line. Requests from these IPs bypass Shield checks. Matches the connecting peer only (REMOTE_ADDR); proxy-forwarded headers are intentionally ignored.</comment>
2626
</field>
2727
</group>
2828
</section>

0 commit comments

Comments
 (0)