From f313ca92e7defc045d046ab8e5eaf57909d5a212 Mon Sep 17 00:00:00 2001 From: Faraz Samapoor Date: Mon, 26 Jun 2023 13:28:09 +0330 Subject: [PATCH] Refactors lib/private/Security. Mainly using PHP8's constructor property promotion. Signed-off-by: Faraz Samapoor --- .../Security/Bruteforce/CleanupJob.php | 11 +- lib/private/Security/Bruteforce/Throttler.php | 3 - .../Security/CSP/ContentSecurityPolicy.php | 107 +++--------------- .../CSP/ContentSecurityPolicyManager.php | 24 ++-- .../CSP/ContentSecurityPolicyNonceManager.php | 25 +--- lib/private/Security/CSRF/CsrfToken.php | 14 +-- .../Security/CSRF/CsrfTokenGenerator.php | 12 +- .../Security/CSRF/CsrfTokenManager.php | 28 ++--- .../CSRF/TokenStorage/SessionStorage.php | 26 ++--- .../FeaturePolicy/FeaturePolicyManager.php | 16 +-- 10 files changed, 65 insertions(+), 201 deletions(-) diff --git a/lib/private/Security/Bruteforce/CleanupJob.php b/lib/private/Security/Bruteforce/CleanupJob.php index 45cfe572acb0a..13628dd300d82 100644 --- a/lib/private/Security/Bruteforce/CleanupJob.php +++ b/lib/private/Security/Bruteforce/CleanupJob.php @@ -32,19 +32,18 @@ use OCP\IDBConnection; class CleanupJob extends TimedJob { - /** @var IDBConnection */ - private $connection; - - public function __construct(ITimeFactory $time, IDBConnection $connection) { + public function __construct( + ITimeFactory $time, + private IDBConnection $connection, + ) { parent::__construct($time); - $this->connection = $connection; // Run once a day $this->setInterval(3600 * 24); $this->setTimeSensitivity(IJob::TIME_INSENSITIVE); } - protected function run($argument) { + protected function run($argument): void { // Delete all entries more than 48 hours old $time = $this->time->getTime() - (48 * 3600); diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php index 2803373e8ba33..5316071f25cfc 100644 --- a/lib/private/Security/Bruteforce/Throttler.php +++ b/lib/private/Security/Bruteforce/Throttler.php @@ -106,9 +106,6 @@ public function registerAttempt(string $action, /** * Check if the IP is whitelisted - * - * @param string $ip - * @return bool */ public function isBypassListed(string $ip): bool { if (isset($this->ipIsWhitelisted[$ip])) { diff --git a/lib/private/Security/CSP/ContentSecurityPolicy.php b/lib/private/Security/CSP/ContentSecurityPolicy.php index e2d115cf34ec6..eca3e2b6b2940 100644 --- a/lib/private/Security/CSP/ContentSecurityPolicy.php +++ b/lib/private/Security/CSP/ContentSecurityPolicy.php @@ -34,33 +34,22 @@ * @package OC\Security\CSP */ class ContentSecurityPolicy extends \OCP\AppFramework\Http\ContentSecurityPolicy { - /** - * @return boolean - */ public function isInlineScriptAllowed(): bool { return $this->inlineScriptAllowed; } - /** - * @param boolean $inlineScriptAllowed - */ - public function setInlineScriptAllowed(bool $inlineScriptAllowed) { + public function setInlineScriptAllowed(bool $inlineScriptAllowed): void { $this->inlineScriptAllowed = $inlineScriptAllowed; } - /** - * @return boolean - */ public function isEvalScriptAllowed(): bool { return $this->evalScriptAllowed; } /** - * @param boolean $evalScriptAllowed - * * @deprecated 17.0.0 Unsafe eval should not be used anymore. */ - public function setEvalScriptAllowed(bool $evalScriptAllowed) { + public function setEvalScriptAllowed(bool $evalScriptAllowed): void { $this->evalScriptAllowed = $evalScriptAllowed; } @@ -72,134 +61,79 @@ public function setEvalWasmAllowed(bool $evalWasmAllowed): void { $this->evalWasmAllowed = $evalWasmAllowed; } - /** - * @return array - */ public function getAllowedScriptDomains(): array { return $this->allowedScriptDomains; } - /** - * @param array $allowedScriptDomains - */ - public function setAllowedScriptDomains(array $allowedScriptDomains) { + public function setAllowedScriptDomains(array $allowedScriptDomains): void { $this->allowedScriptDomains = $allowedScriptDomains; } - /** - * @return boolean - */ public function isInlineStyleAllowed(): bool { return $this->inlineStyleAllowed; } - /** - * @param boolean $inlineStyleAllowed - */ - public function setInlineStyleAllowed(bool $inlineStyleAllowed) { + public function setInlineStyleAllowed(bool $inlineStyleAllowed): void { $this->inlineStyleAllowed = $inlineStyleAllowed; } - /** - * @return array - */ public function getAllowedStyleDomains(): array { return $this->allowedStyleDomains; } - /** - * @param array $allowedStyleDomains - */ - public function setAllowedStyleDomains(array $allowedStyleDomains) { + public function setAllowedStyleDomains(array $allowedStyleDomains): void { $this->allowedStyleDomains = $allowedStyleDomains; } - /** - * @return array - */ public function getAllowedImageDomains(): array { return $this->allowedImageDomains; } - /** - * @param array $allowedImageDomains - */ - public function setAllowedImageDomains(array $allowedImageDomains) { + public function setAllowedImageDomains(array $allowedImageDomains): void { $this->allowedImageDomains = $allowedImageDomains; } - /** - * @return array - */ public function getAllowedConnectDomains(): array { return $this->allowedConnectDomains; } - /** - * @param array $allowedConnectDomains - */ - public function setAllowedConnectDomains(array $allowedConnectDomains) { + public function setAllowedConnectDomains(array $allowedConnectDomains): void { $this->allowedConnectDomains = $allowedConnectDomains; } - /** - * @return array - */ public function getAllowedMediaDomains(): array { return $this->allowedMediaDomains; } - /** - * @param array $allowedMediaDomains - */ - public function setAllowedMediaDomains(array $allowedMediaDomains) { + public function setAllowedMediaDomains(array $allowedMediaDomains): void { $this->allowedMediaDomains = $allowedMediaDomains; } - /** - * @return array - */ public function getAllowedObjectDomains(): array { return $this->allowedObjectDomains; } - /** - * @param array $allowedObjectDomains - */ - public function setAllowedObjectDomains(array $allowedObjectDomains) { + public function setAllowedObjectDomains(array $allowedObjectDomains): void { $this->allowedObjectDomains = $allowedObjectDomains; } - /** - * @return array - */ public function getAllowedFrameDomains(): array { return $this->allowedFrameDomains; } - /** - * @param array $allowedFrameDomains - */ - public function setAllowedFrameDomains(array $allowedFrameDomains) { + public function setAllowedFrameDomains(array $allowedFrameDomains): void { $this->allowedFrameDomains = $allowedFrameDomains; } - /** - * @return array - */ public function getAllowedFontDomains(): array { return $this->allowedFontDomains; } - /** - * @param array $allowedFontDomains - */ - public function setAllowedFontDomains($allowedFontDomains) { + public function setAllowedFontDomains($allowedFontDomains): void { $this->allowedFontDomains = $allowedFontDomains; } /** - * @return array * @deprecated 15.0.0 use FrameDomains and WorkerSrcDomains */ public function getAllowedChildSrcDomains(): array { @@ -210,13 +144,10 @@ public function getAllowedChildSrcDomains(): array { * @param array $allowedChildSrcDomains * @deprecated 15.0.0 use FrameDomains and WorkerSrcDomains */ - public function setAllowedChildSrcDomains($allowedChildSrcDomains) { + public function setAllowedChildSrcDomains($allowedChildSrcDomains): void { $this->allowedChildSrcDomains = $allowedChildSrcDomains; } - /** - * @return array - */ public function getAllowedFrameAncestors(): array { return $this->allowedFrameAncestors; } @@ -224,7 +155,7 @@ public function getAllowedFrameAncestors(): array { /** * @param array $allowedFrameAncestors */ - public function setAllowedFrameAncestors($allowedFrameAncestors) { + public function setAllowedFrameAncestors($allowedFrameAncestors): void { $this->allowedFrameAncestors = $allowedFrameAncestors; } @@ -232,7 +163,7 @@ public function getAllowedWorkerSrcDomains(): array { return $this->allowedWorkerSrcDomains; } - public function setAllowedWorkerSrcDomains(array $allowedWorkerSrcDomains) { + public function setAllowedWorkerSrcDomains(array $allowedWorkerSrcDomains): void { $this->allowedWorkerSrcDomains = $allowedWorkerSrcDomains; } @@ -249,21 +180,15 @@ public function getReportTo(): array { return $this->reportTo; } - public function setReportTo(array $reportTo) { + public function setReportTo(array $reportTo): void { $this->reportTo = $reportTo; } - /** - * @return boolean - */ public function isStrictDynamicAllowed(): bool { return $this->strictDynamicAllowed; } - /** - * @param boolean $strictDynamicAllowed - */ - public function setStrictDynamicAllowed(bool $strictDynamicAllowed) { + public function setStrictDynamicAllowed(bool $strictDynamicAllowed): void { $this->strictDynamicAllowed = $strictDynamicAllowed; } } diff --git a/lib/private/Security/CSP/ContentSecurityPolicyManager.php b/lib/private/Security/CSP/ContentSecurityPolicyManager.php index 4930dcb759c56..503933ef98021 100644 --- a/lib/private/Security/CSP/ContentSecurityPolicyManager.php +++ b/lib/private/Security/CSP/ContentSecurityPolicyManager.php @@ -35,25 +35,21 @@ class ContentSecurityPolicyManager implements IContentSecurityPolicyManager { /** @var ContentSecurityPolicy[] */ - private $policies = []; + private array $policies = []; - /** @var IEventDispatcher */ - private $dispatcher; - - public function __construct(IEventDispatcher $dispatcher) { - $this->dispatcher = $dispatcher; + public function __construct( + private IEventDispatcher $dispatcher, + ) { } /** {@inheritdoc} */ - public function addDefaultPolicy(EmptyContentSecurityPolicy $policy) { + public function addDefaultPolicy(EmptyContentSecurityPolicy $policy): void { $this->policies[] = $policy; } /** * Get the configured default policy. This is not in the public namespace * as it is only supposed to be used by core itself. - * - * @return ContentSecurityPolicy */ public function getDefaultPolicy(): ContentSecurityPolicy { $event = new AddContentSecurityPolicyEvent($this); @@ -68,13 +64,11 @@ public function getDefaultPolicy(): ContentSecurityPolicy { /** * Merges the first given policy with the second one - * - * @param ContentSecurityPolicy $defaultPolicy - * @param EmptyContentSecurityPolicy $originalPolicy - * @return ContentSecurityPolicy */ - public function mergePolicies(ContentSecurityPolicy $defaultPolicy, - EmptyContentSecurityPolicy $originalPolicy): ContentSecurityPolicy { + public function mergePolicies( + ContentSecurityPolicy $defaultPolicy, + EmptyContentSecurityPolicy $originalPolicy, + ): ContentSecurityPolicy { foreach ((object)(array)$originalPolicy as $name => $value) { $setter = 'set'.ucfirst($name); if (\is_array($value)) { diff --git a/lib/private/Security/CSP/ContentSecurityPolicyNonceManager.php b/lib/private/Security/CSP/ContentSecurityPolicyNonceManager.php index 1167b3358d2ee..6573007a45947 100644 --- a/lib/private/Security/CSP/ContentSecurityPolicyNonceManager.php +++ b/lib/private/Security/CSP/ContentSecurityPolicyNonceManager.php @@ -38,27 +38,16 @@ * @package OC\Security\CSP */ class ContentSecurityPolicyNonceManager { - /** @var CsrfTokenManager */ - private $csrfTokenManager; - /** @var IRequest */ - private $request; - /** @var string */ - private $nonce = ''; + private string $nonce = ''; - /** - * @param CsrfTokenManager $csrfTokenManager - * @param IRequest $request - */ - public function __construct(CsrfTokenManager $csrfTokenManager, - IRequest $request) { - $this->csrfTokenManager = $csrfTokenManager; - $this->request = $request; + public function __construct( + private CsrfTokenManager $csrfTokenManager, + private IRequest $request, + ) { } /** - * Returns the current CSP nounce - * - * @return string + * Returns the current CSP nonce */ public function getNonce(): string { if ($this->nonce === '') { @@ -74,8 +63,6 @@ public function getNonce(): string { /** * Check if the browser supports CSP v3 - * - * @return bool */ public function browserSupportsCspV3(): bool { $browserWhitelist = [ diff --git a/lib/private/Security/CSRF/CsrfToken.php b/lib/private/Security/CSRF/CsrfToken.php index a76e169e5b9e9..45e628b3f3cc7 100644 --- a/lib/private/Security/CSRF/CsrfToken.php +++ b/lib/private/Security/CSRF/CsrfToken.php @@ -36,23 +36,19 @@ * @package OC\Security\CSRF */ class CsrfToken { - /** @var string */ - private $value; - /** @var string */ - private $encryptedValue = ''; + private string $encryptedValue = ''; /** * @param string $value Value of the token. Can be encrypted or not encrypted. */ - public function __construct(string $value) { - $this->value = $value; + public function __construct( + private string $value, + ) { } /** * Encrypted value of the token. This is used to mitigate BREACH alike * vulnerabilities. For display measures do use this functionality. - * - * @return string */ public function getEncryptedValue(): string { if ($this->encryptedValue === '') { @@ -66,8 +62,6 @@ public function getEncryptedValue(): string { /** * The unencrypted value of the token. Used for decrypting an already * encrypted token. - * - * @return string */ public function getDecryptedValue(): string { $token = explode(':', $this->value); diff --git a/lib/private/Security/CSRF/CsrfTokenGenerator.php b/lib/private/Security/CSRF/CsrfTokenGenerator.php index 0576fda9e067f..c3d89247de1c1 100644 --- a/lib/private/Security/CSRF/CsrfTokenGenerator.php +++ b/lib/private/Security/CSRF/CsrfTokenGenerator.php @@ -34,21 +34,15 @@ * @package OC\Security\CSRF */ class CsrfTokenGenerator { - /** @var ISecureRandom */ - private $random; - - /** - * @param ISecureRandom $random - */ - public function __construct(ISecureRandom $random) { - $this->random = $random; + public function __construct( + private ISecureRandom $random, + ) { } /** * Generate a new CSRF token. * * @param int $length Length of the token in characters. - * @return string */ public function generateToken(int $length = 32): string { return $this->random->generate($length); diff --git a/lib/private/Security/CSRF/CsrfTokenManager.php b/lib/private/Security/CSRF/CsrfTokenManager.php index 2c6dd45866d86..dceacf45e2a7c 100644 --- a/lib/private/Security/CSRF/CsrfTokenManager.php +++ b/lib/private/Security/CSRF/CsrfTokenManager.php @@ -34,27 +34,18 @@ * @package OC\Security\CSRF */ class CsrfTokenManager { - /** @var CsrfTokenGenerator */ - private $tokenGenerator; - /** @var SessionStorage */ - private $sessionStorage; - /** @var CsrfToken|null */ - private $csrfToken = null; + private SessionStorage $sessionStorage; + private ?CsrfToken $csrfToken = null; - /** - * @param CsrfTokenGenerator $tokenGenerator - * @param SessionStorage $storageInterface - */ - public function __construct(CsrfTokenGenerator $tokenGenerator, - SessionStorage $storageInterface) { - $this->tokenGenerator = $tokenGenerator; + public function __construct( + private CsrfTokenGenerator $tokenGenerator, + SessionStorage $storageInterface, + ) { $this->sessionStorage = $storageInterface; } /** * Returns the current CSRF token, if none set it will create a new one. - * - * @return CsrfToken */ public function getToken(): CsrfToken { if (!\is_null($this->csrfToken)) { @@ -74,8 +65,6 @@ public function getToken(): CsrfToken { /** * Invalidates any current token and sets a new one. - * - * @return CsrfToken */ public function refreshToken(): CsrfToken { $value = $this->tokenGenerator->generateToken(); @@ -87,16 +76,13 @@ public function refreshToken(): CsrfToken { /** * Remove the current token from the storage. */ - public function removeToken() { + public function removeToken(): void { $this->csrfToken = null; $this->sessionStorage->removeToken(); } /** * Verifies whether the provided token is valid. - * - * @param CsrfToken $token - * @return bool */ public function isTokenValid(CsrfToken $token): bool { if (!$this->sessionStorage->hasToken()) { diff --git a/lib/private/Security/CSRF/TokenStorage/SessionStorage.php b/lib/private/Security/CSRF/TokenStorage/SessionStorage.php index ab05d5b149302..0ffe043e2f992 100644 --- a/lib/private/Security/CSRF/TokenStorage/SessionStorage.php +++ b/lib/private/Security/CSRF/TokenStorage/SessionStorage.php @@ -35,27 +35,18 @@ * @package OC\Security\CSRF\TokenStorage */ class SessionStorage { - /** @var ISession */ - private $session; - - /** - * @param ISession $session - */ - public function __construct(ISession $session) { - $this->session = $session; + public function __construct( + private ISession $session, + ) { } - /** - * @param ISession $session - */ - public function setSession(ISession $session) { + public function setSession(ISession $session): void { $this->session = $session; } /** * Returns the current token or throws an exception if none is found. * - * @return string * @throws \Exception */ public function getToken(): string { @@ -69,23 +60,20 @@ public function getToken(): string { /** * Set the valid current token to $value. - * - * @param string $value */ - public function setToken(string $value) { + public function setToken(string $value): void { $this->session->set('requesttoken', $value); } /** * Removes the current token. */ - public function removeToken() { + public function removeToken(): void { $this->session->remove('requesttoken'); } + /** * Whether the storage has a storage. - * - * @return bool */ public function hasToken(): bool { return $this->session->exists('requesttoken'); diff --git a/lib/private/Security/FeaturePolicy/FeaturePolicyManager.php b/lib/private/Security/FeaturePolicy/FeaturePolicyManager.php index 3aa93ac3da48f..bb9fc41332f33 100644 --- a/lib/private/Security/FeaturePolicy/FeaturePolicyManager.php +++ b/lib/private/Security/FeaturePolicy/FeaturePolicyManager.php @@ -32,13 +32,11 @@ class FeaturePolicyManager { /** @var EmptyFeaturePolicy[] */ - private $policies = []; + private array $policies = []; - /** @var IEventDispatcher */ - private $dispatcher; - - public function __construct(IEventDispatcher $dispatcher) { - $this->dispatcher = $dispatcher; + public function __construct( + private IEventDispatcher $dispatcher, + ) { } public function addDefaultPolicy(EmptyFeaturePolicy $policy): void { @@ -60,8 +58,10 @@ public function getDefaultPolicy(): FeaturePolicy { * Merges the first given policy with the second one * */ - public function mergePolicies(FeaturePolicy $defaultPolicy, - EmptyFeaturePolicy $originalPolicy): FeaturePolicy { + public function mergePolicies( + FeaturePolicy $defaultPolicy, + EmptyFeaturePolicy $originalPolicy, + ): FeaturePolicy { foreach ((object)(array)$originalPolicy as $name => $value) { $setter = 'set' . ucfirst($name); if (\is_array($value)) {