Skip to content

Commit 162d75b

Browse files
KristofersOzolinsMagebitKristofersOzolinsMagebit
authored
Merge pull request #16 from magebitcom/bugfix/csp
fix(oauth): add oauth callback URL to CSP whitelist
2 parents a13b94c + 218807e commit 162d75b

4 files changed

Lines changed: 154 additions & 0 deletions

File tree

‎Controller/Adminhtml/Oauth/Authorize.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
use Magebit\Mcp\Model\OAuth\AuthMode;
1818
use Magebit\Mcp\Model\OAuth\AuthorizeHandoffStorage;
1919
use Magebit\Mcp\Model\OAuth\ConsentParamsResolver;
20+
use Magebit\Mcp\Model\OAuth\FormActionCspWhitelister;
2021
use Magebit\Mcp\Model\OAuth\InlineErrorRenderer;
2122
use Magebit\Mcp\Model\OAuth\PkceVerifier;
2223
use Magebit\Mcp\Model\OAuth\RedirectUriValidator;
@@ -61,6 +62,7 @@ class Authorize extends Action implements HttpGetActionInterface, HttpPostAction
6162
* @param ConsentParamsResolver $consentParamsResolver
6263
* @param InlineErrorRenderer $inlineErrorRenderer
6364
* @param AdminAuthorizationGate $adminAuthorizationGate
65+
* @param FormActionCspWhitelister $formActionCspWhitelister
6466
* @param LoggerInterface $logger
6567
*/
6668
public function __construct(
@@ -74,6 +76,7 @@ public function __construct(
7476
private readonly ConsentParamsResolver $consentParamsResolver,
7577
private readonly InlineErrorRenderer $inlineErrorRenderer,
7678
private readonly AdminAuthorizationGate $adminAuthorizationGate,
79+
private readonly FormActionCspWhitelister $formActionCspWhitelister,
7780
private readonly LoggerInterface $logger
7881
) {
7982
parent::__construct($context);
@@ -127,6 +130,13 @@ private function renderConsent(string $nonce): ResultInterface|HttpResponse
127130
}
128131
}
129132

133+
// The consent form POSTs back here, then 302s cross-origin to the client's
134+
// redirect_uri. form-action CSP governs that whole chain from this document,
135+
// so the (already-validated) redirect origin must be whitelisted here.
136+
$this->formActionCspWhitelister->whitelistRedirectTarget(
137+
ConsentParamsResolver::stringFromParams($params, 'redirect_uri')
138+
);
139+
130140
/** @var Page $page */
131141
$page = $this->pageFactory->create();
132142
$page->setHeader('Cache-Control', 'private, no-store, no-cache, must-revalidate', true);

‎Model/OAuth/FormActionCspWhitelister.php‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
<?php
2+
/**
3+
* @author Magebit <info@magebit.com>
4+
* @copyright Copyright (c) Magebit, Ltd. (https://magebit.com)
5+
* @license MIT
6+
*/
7+
declare(strict_types=1);
8+
9+
namespace Magebit\Mcp\Model\OAuth;
10+
11+
use Magento\Csp\Model\Collector\DynamicCollector;
12+
use Magento\Csp\Model\Policy\FetchPolicyFactory;
13+
14+
class FormActionCspWhitelister
15+
{
16+
/**
17+
* @param DynamicCollector $dynamicCollector
18+
* @param FetchPolicyFactory $fetchPolicyFactory
19+
*/
20+
public function __construct(
21+
private readonly DynamicCollector $dynamicCollector,
22+
private readonly FetchPolicyFactory $fetchPolicyFactory
23+
) {
24+
}
25+
26+
/**
27+
* @param string $redirectUri
28+
* @return void
29+
*/
30+
public function whitelistRedirectTarget(string $redirectUri): void
31+
{
32+
$origin = $this->originOf($redirectUri);
33+
if ($origin === null) {
34+
return;
35+
}
36+
37+
// selfAllowed is OR-merged into the existing form-action policy, so it stays
38+
// safe to set even when this is the only contributor to the directive.
39+
$this->dynamicCollector->add(
40+
$this->fetchPolicyFactory->create([
41+
'id' => 'form-action',
42+
'noneAllowed' => false,
43+
'hostSources' => [$origin],
44+
'selfAllowed' => true,
45+
])
46+
);
47+
}
48+
49+
/**
50+
* @param string $redirectUri
51+
* @return string|null A `scheme://host[:port]` origin, or null when unparseable.
52+
*/
53+
private function originOf(string $redirectUri): ?string
54+
{
55+
// phpcs:ignore Magento2.Functions.DiscouragedFunction
56+
$parts = parse_url($redirectUri);
57+
if (!is_array($parts) || empty($parts['scheme']) || empty($parts['host'])) {
58+
return null;
59+
}
60+
61+
$origin = $parts['scheme'] . '://' . $parts['host'];
62+
if (isset($parts['port'])) {
63+
$origin .= ':' . $parts['port'];
64+
}
65+
return $origin;
66+
}
67+
}

‎Test/Unit/Model/OAuth/FormActionCspWhitelisterTest.php‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
<?php
2+
/**
3+
* @author Magebit <info@magebit.com>
4+
* @copyright Copyright (c) Magebit, Ltd. (https://magebit.com)
5+
* @license MIT
6+
*/
7+
declare(strict_types=1);
8+
9+
namespace Magebit\Mcp\Test\Unit\Model\OAuth;
10+
11+
use Magebit\Mcp\Model\OAuth\FormActionCspWhitelister;
12+
use Magento\Csp\Model\Collector\DynamicCollector;
13+
use Magento\Csp\Model\Policy\FetchPolicy;
14+
use Magento\Csp\Model\Policy\FetchPolicyFactory;
15+
use PHPUnit\Framework\MockObject\MockObject;
16+
use PHPUnit\Framework\TestCase;
17+
18+
class FormActionCspWhitelisterTest extends TestCase
19+
{
20+
private DynamicCollector&MockObject $collector;
21+
private FetchPolicyFactory&MockObject $policyFactory;
22+
private FormActionCspWhitelister $whitelister;
23+
24+
protected function setUp(): void
25+
{
26+
$this->collector = $this->createMock(DynamicCollector::class);
27+
$this->policyFactory = $this->createMock(FetchPolicyFactory::class);
28+
// Build a real FetchPolicy from the requested args so assertions exercise the real DTO.
29+
$this->policyFactory->method('create')->willReturnCallback(
30+
static fn (array $data): FetchPolicy => new FetchPolicy(
31+
(string) $data['id'],
32+
(bool) ($data['noneAllowed'] ?? true),
33+
(array) ($data['hostSources'] ?? []),
34+
(array) ($data['schemeSources'] ?? []),
35+
(bool) ($data['selfAllowed'] ?? false)
36+
)
37+
);
38+
$this->whitelister = new FormActionCspWhitelister($this->collector, $this->policyFactory);
39+
}
40+
41+
public function testWhitelistsRedirectOriginForFormAction(): void
42+
{
43+
$this->collector->expects(self::once())->method('add')->with(
44+
self::callback(static function (FetchPolicy $policy): bool {
45+
return $policy->getId() === 'form-action'
46+
&& $policy->isSelfAllowed() === true
47+
&& $policy->getHostSources() === ['https://claude.ai'];
48+
})
49+
);
50+
51+
$this->whitelister->whitelistRedirectTarget('https://claude.ai/api/mcp/auth_callback');
52+
}
53+
54+
public function testIncludesNonDefaultPortInOrigin(): void
55+
{
56+
$this->collector->expects(self::once())->method('add')->with(
57+
self::callback(
58+
static fn (FetchPolicy $policy): bool => $policy->getHostSources() === ['http://localhost:33418']
59+
)
60+
);
61+
62+
$this->whitelister->whitelistRedirectTarget('http://localhost:33418/oauth/callback');
63+
}
64+
65+
public function testIgnoresEmptyRedirectUri(): void
66+
{
67+
$this->collector->expects(self::never())->method('add');
68+
$this->whitelister->whitelistRedirectTarget('');
69+
}
70+
71+
public function testIgnoresRedirectUriWithoutHost(): void
72+
{
73+
$this->collector->expects(self::never())->method('add');
74+
$this->whitelister->whitelistRedirectTarget('/relative/path');
75+
}
76+
}

‎etc/module.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
<module name="Magento_Config"/>
1111
<module name="Magento_Indexer"/>
1212
<module name="Magento_AdminNotification"/>
13+
<module name="Magento_Csp"/>
1314
</sequence>
1415
</module>
1516
</config>

0 commit comments

Comments
 (0)