Fixes various QA identified issues, #PG-5091

API.php

@@ -262,13 +262,7 @@ private function buildValidatedClientData(
     ): array {
         $type = $type === 'public' ? 'public' : 'confidential';
 
-        $redirects = is_array($redirectUris) ? $redirectUris : preg_split('/[\r\n]+/', (string) $redirectUris);
-        if ($redirects === false) {
-            $redirects = [];
-        }
-        $redirects = array_values(array_filter(array_map('trim', $redirects), static function ($value) {
-            return $value !== '';
-        }));
+        $redirects = $this->parseRedirectUris($redirectUris);
 
         $grantTypes = array_values(array_filter(array_map('trim', (array) $grantTypes), static function ($value) {
             return $value !== '';
@@ -298,6 +292,45 @@ private function buildValidatedClientData(
         ];
     }
 
+    private function parseRedirectUris($redirectUris): array
+    {
+        if (is_array($redirectUris)) {
+            $rawRedirects = $redirectUris;
+        } else {
+            $rawRedirects = preg_split("/\r\n|\n|\r/", (string) $redirectUris);
+            if ($rawRedirects === false) {
+                $rawRedirects = [];
+            }
+        }
+
+        $redirects = [];
+
+        foreach ($rawRedirects as $rawRedirectUri) {
+            $rawRedirectUri = is_string($rawRedirectUri) ? $rawRedirectUri : (string) $rawRedirectUri;
+
+            if (trim($rawRedirectUri) === '') {
+                continue;
+            }
+
+            $parsedRedirectUri = parse_url($rawRedirectUri);
+
+            if (
+                $rawRedirectUri !== trim($rawRedirectUri)
+                || preg_match('/\s/', $rawRedirectUri) === 1
+                || str_contains($rawRedirectUri, '\\')
+                || $parsedRedirectUri === false
+                || preg_match('~https?://~i', (string) ($parsedRedirectUri['path'] ?? '')) === 1
+                || preg_match('~https?://~i', (string) ($parsedRedirectUri['fragment'] ?? '')) === 1
+            ) {
+                throw new \InvalidArgumentException(Piwik::translate('OAuth2_InvalidRedirectUri') . ': ' . $rawRedirectUri);
+            }
+
+            $redirects[] = $rawRedirectUri;
+        }
+
+        return array_values($redirects);
+    }
+
     private function validateRedirectUris(array $redirectUris, array $grantTypes): void
     {
         if (!in_array('authorization_code', $grantTypes, true)) {

CHANGELOG.md

@@ -1,5 +1,6 @@
 5.0.3 - 2026-05-11
-- Added code to show token and authorize URL at the top of list screen 
+- Added code to show token and authorize URL at the top of list screen
+- Added better validation check for redirect URL, setting values and show client secret along with success message
 
 5.0.2 - 2026-04-27
 - Updated API documentation

OAuth2.php

@@ -165,6 +165,7 @@ public function getClientSideTranslationKeys(&$translationKeys)
         $translationKeys[] = 'OAuth2_AdminResumeConfirm';
         $translationKeys[] = 'OAuth2_ClientSecretMaskedHelp';
         $translationKeys[] = 'OAuth2_ClientSecretVisibleHelp';
+        $translationKeys[] = 'OAuth2_ClientSecretDisplayedNotification';
         $translationKeys[] = 'OAuth2_AdminDescriptionPlaceholder';
         $translationKeys[] = 'OAuth2_AdminRotatedNotification';
         $translationKeys[] = 'OAuth2_ScopeSuperUserShort';

SystemSettings.php

@@ -32,8 +32,8 @@ protected function init()
             $field->description = Piwik::translate('OAuth2_SystemSettingOAuthAccessTokenLifetimeDescription');
             $field->uiControl = FieldConfig::UI_CONTROL_TEXT;
             $field->validate = function ($value) {
-                if ($value <= 0) {
-                    throw new \Exception(Piwik::translate('OAuth2_InvalidValueException'));
+                if ($value <= 0 || !is_numeric($value)) {
+                    throw new \Exception(Piwik::translate('OAuth2_InvalidNumericValueException'));
                 }
             };
         });
@@ -43,8 +43,8 @@ protected function init()
             $field->description = Piwik::translate('OAuth2_SystemSettingOAuthRefreshTokenLifetimeDescription');
             $field->uiControl = FieldConfig::UI_CONTROL_TEXT;
             $field->validate = function ($value) {
-                if ($value <= 0) {
-                    throw new \Exception(Piwik::translate('OAuth2_InvalidValueException'));
+                if ($value <= 0 || !is_numeric($value)) {
+                    throw new \Exception(Piwik::translate('OAuth2_InvalidNumericValueException'));
                 }
             };
         });
@@ -54,8 +54,8 @@ protected function init()
             $field->description = Piwik::translate('OAuth2_SystemSettingOAuthAuthorizationCodeLifetimeDescription');
             $field->uiControl = FieldConfig::UI_CONTROL_TEXT;
             $field->validate = function ($value) {
-                if ($value <= 0) {
-                    throw new \Exception(Piwik::translate('OAuth2_InvalidValueException'));
+                if ($value <= 0 || !is_numeric($value)) {
+                    throw new \Exception(Piwik::translate('OAuth2_InvalidNumericValueException'));
                 }
             };
         });

lang/en.json

@@ -69,6 +69,7 @@
     "AuthorizationCodeGrantDisabled": "Authorization code grant is disabled.",
     "ClientSecret": "Client secret",
     "ClientSecretHelp": "Copy the client secret now, it will not be shown again.",
+    "ClientSecretDisplayedNotification": "The client secret is %1$s.",
     "ClientSecretVisibleHelp": "Use this secret to authenticate the client securely. It is only shown in full when it is created or rotated.",
     "ClientSecretMaskedHelp": "This secret is hidden after it is created or rotated. If you did not copy it, rotate the secret to generate a new one.",
     "ClientCredentialsExceptionPublicClient": "Public clients cannot use the client_credentials grant type",
@@ -102,6 +103,7 @@
     "SystemSettingOAuthEnableAClientCredentialsTitle": "Enable client credentials grant",
     "SystemSettingOAuthEnableRefreshTokenTitle": "Enable refresh tokens",
     "InvalidValueException": "Invalid value, it should be greater than 0.",
+    "InvalidNumericValueException": "Invalid value, it should be a numeric value, greater than 0.",
     "TokenEndpointException": "Token endpoint only accepts POST"
   }
 }

tests/Integration/APITest.php

@@ -196,6 +196,41 @@ public function test_createClient_rejectsPublicClientCredentialsGrant()
         );
     }
 
+    /**
+     * @dataProvider getInvalidRedirectUris
+     */
+    public function test_createClient_rejectsInvalidRedirectUris($redirectUris)
+    {
+        $this->expectException(\InvalidArgumentException::class);
+        $this->expectExceptionMessage('Invalid redirect_uri');
+
+        $this->api->createClient(
+            'Invalid redirect client',
+            ['authorization_code', 'refresh_token'],
+            'matomo:read',
+            $redirectUris,
+            'Invalid redirect test',
+            'confidential'
+        );
+    }
+
+    public function test_createClient_allowsRedirectUriWithUrlInQueryString()
+    {
+        $result = $this->api->createClient(
+            'Valid redirect client',
+            ['authorization_code', 'refresh_token'],
+            'matomo:read',
+            ['https://example.com/callback?redirect=https://other.example/path'],
+            'Valid redirect test',
+            'confidential'
+        );
+
+        $this->assertSame(
+            ['https://example.com/callback?redirect=https://other.example/path'],
+            $result['client']['redirect_uris']
+        );
+    }
+
     public function test_rotateSecret_invalidatesPreviousSecret()
     {
         $result = $this->createConfidentialClient();
@@ -355,6 +390,24 @@ private function setSuperUser(): void
         FakeAccess::clearAccess(true);
     }
 
+    public function getInvalidRedirectUris(): array
+    {
+        return [
+            ["https://example.com/callback\thttps://example.com/other"],
+            ["https://example.com/callback https://example.com/other"],
+            ["https://example.com/callback\\t"],
+            ["https://example.com/callback\\n"],
+            ["https://example.com/callbackhttps://example.org/other"],
+            ["https://example.com/callback,https://example.org/other"],
+            [" https://example.com/callback"],
+            ["https://example.com/callback "],
+            [["https://example.com/callback\t"]],
+            [["https://example.com/callback\\t"]],
+            [["https://example.com/callback https://example.com/other"]],
+            [["https://example.com/callbackhttps://example.org/other"]],
+        ];
+    }
+
     private function setRegularUser(): void
     {
         FakeAccess::clearAccess(false);

tests/UI/OAuth2_spec.js

@@ -60,12 +60,19 @@ describe("OAuth2Admin", function () {
         await page.waitForTimeout(100);
     }
 
-    async function submitForm()
+    async function submitForm(expectSecretMessage = false)
     {
         await page.waitForSelector('.oauth2-admin form button.btn', { visible: true });
         await page.click('.oauth2-admin form button.btn');
         await page.waitForNetworkIdle();
         await page.waitForTimeout(300);
+        if (expectSecretMessage) {
+            const successHtml = await page.$eval('.success-msg-created', (element) => element.innerHTML);
+
+            expect(successHtml).to.contain('The client secret is <code>');
+            expect(successHtml).to.contain('Copy the client secret now, it will not be shown again');
+        }
+
         await page.evaluate(function () {
             $('.success-msg-created').html('Client fixedValueForTest created');
         });
@@ -133,7 +140,7 @@ describe("OAuth2Admin", function () {
     it('should create a confidential client and show the secret once', async function () {
         await page.goto(createUrl);
         await fillClientForm('Confidential UI client', 'Confidential', 'https://confidential.example/callback');
-        await submitForm();
+        await submitForm(true);
         await capturePage('create_confidential_success');
     });