Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Validator infrastructure #27961

Closed
wants to merge 10 commits into from
Closed

Conversation

CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Jul 14, 2021

Originally this PR tried to fix the validation of URLs, but the scope of this
changed to instead provide a generic interface for validating user input,
configs, ... The theming API was ported to it as example and to fix the
original bug.

The API is based on Symfony Validator component but the implementation
is simplified a lot since we don't support (yet) validating class with annotation
and other more complicated (and useful) stuff.

The filter_var function is unfortunately not perfect and doesn't support
domain with unicode as well as url with underscores. Replace usage with
a regex.

See https://bugs.php.net/search.php?cmd=display&search_for=FILTER_VALIDATE_URL

Closes #27906

Signed-off-by: Carl Schwan [email protected]

@CarlSchwan CarlSchwan force-pushed the work/carl/idn-url-theming branch from d5d86b1 to e1e549e Compare July 14, 2021 09:45
@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Jul 14, 2021
@szaimen szaimen added this to the Nextcloud 23 milestone Jul 14, 2021
@juliusknorr
Copy link
Member

Mind to squash to a single commit? :)

@CarlSchwan CarlSchwan force-pushed the work/carl/idn-url-theming branch from 10a4cec to abfe172 Compare July 14, 2021 19:52
@blizzz
Copy link
Member

blizzz commented Jul 14, 2021

CI is unhappy tho

@CarlSchwan CarlSchwan force-pushed the work/carl/idn-url-theming branch from abfe172 to 24db036 Compare July 14, 2021 21:40
@CarlSchwan
Copy link
Member Author

It's a bit weird that the ci report a failure for continuous-integration/drone/pr but in https://drone.nextcloud.com/nextcloud/server/7311 everything is green

@juliusknorr
Copy link
Member

It's a bit weird that the ci report a failure for continuous-integration/drone/pr but in drone.nextcloud.com/nextcloud/server/7311 everything is green

What about https://drone.nextcloud.com/nextcloud/server/7311/9/4 😉


There were 4 failures:

1) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheetSuccess with data set #1 ('url', 'https://nextcloud.com/aaaaaaa...aaaaaa', 'Saved')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\DataResponse Object (
     'data' => Array (
         'data' => Array (
-            'message' => 'Saved'
-            'serverCssUrl' => '/nextcloudWebroot/core/css/so...s.scss'
+            'message' => 'The given web address is not a valid URL'
         )
-        'status' => 'success'
+        'status' => 'error'
     )
     'headers' => Array (...)
     'cookies' => Array ()
-    'status' => 200
+    'status' => 400
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => OCP\AppFramework\Http\EmptyContentSecurityPolicy Object (...)

/drone/src/apps/theming/tests/Controller/ThemingControllerTest.php:168

2) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheetSuccess with data set #6 ('imprintUrl', 'https://nextcloud.com/aaaaaaa...aaaaaa', 'Saved')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\DataResponse Object (
     'data' => Array (
         'data' => Array (
-            'message' => 'Saved'
-            'serverCssUrl' => '/nextcloudWebroot/core/css/so...s.scss'
+            'message' => 'The given legal notice address is not a valid URL'
         )
-        'status' => 'success'
+        'status' => 'error'
     )
     'headers' => Array (...)
     'cookies' => Array ()
-    'status' => 200
+    'status' => 400
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => OCP\AppFramework\Http\EmptyContentSecurityPolicy Object (...)

/drone/src/apps/theming/tests/Controller/ThemingControllerTest.php:168

3) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheetSuccess with data set #7 ('privacyUrl', 'https://nextcloud.com/aaaaaaa...aaaaaa', 'Saved')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\DataResponse Object (
     'data' => Array (
         'data' => Array (
-            'message' => 'Saved'
-            'serverCssUrl' => '/nextcloudWebroot/core/css/so...s.scss'
+            'message' => 'The given privacy policy address is not a valid URL'
         )
-        'status' => 'success'
+        'status' => 'error'
     )
     'headers' => Array (...)
     'cookies' => Array ()
-    'status' => 200
+    'status' => 400
     'lastModified' => null
     'ETag' => null
     'contentSecurityPolicy' => OCP\AppFramework\Http\EmptyContentSecurityPolicy Object (...)

/drone/src/apps/theming/tests/Controller/ThemingControllerTest.php:168

4) OCA\Theming\Tests\Controller\ThemingControllerTest::testUpdateStylesheetError with data set #1 ('url', 'http://example.com/aaaaaaaaaa...aaaaaa', 'The given web address is too long')
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 OCP\AppFramework\Http\DataResponse Object (
     'data' => Array (
         'data' => Array (
-            'message' => 'The given web address is too long'
+            'message' => 'The given web address is not a valid URL'
         )
         'status' => 'error'
     )

/drone/src/apps/theming/tests/Controller/ThemingControllerTest.php:217

--

@CarlSchwan
Copy link
Member Author

It seems idn_to_ascii is kinda evil too and just abort when the URL is too long :(

A better solution would be either to just copy the UrlValidator::validate from symfony/validator or just depends on symfony/validator and use it for all kind of validations in Nextcloud (URL, email, date ...). For reference, this is the method: https://github.com/symfony/validator/blob/14337bdf9e2e0b2e3385c9e90f13325f0c95a4f9/Constraints/UrlValidator.php#L24

@tcitworld
Copy link
Member

It seems idn_to_ascii is kinda evil too and just abort when the URL is too long :(

That's probably because it's made to handle domains, not full URIs. You could extract the hostname with parse_url, convert it, then substr_replace it in the URL.

But using the regex from SF Validator makes sense too.

@CarlSchwan CarlSchwan force-pushed the work/carl/idn-url-theming branch from b8d57ec to e693979 Compare July 20, 2021 08:28
lib/public/Util.php Outdated Show resolved Hide resolved
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from that, code looks good. Didn't try it though.

apps/oauth2/lib/Controller/SettingsController.php Outdated Show resolved Hide resolved
apps/theming/lib/ThemingDefaults.php Outdated Show resolved Hide resolved
lib/private/Setup.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan force-pushed the work/carl/idn-url-theming branch 4 times, most recently from e62819d to 1fd5538 Compare August 6, 2021 13:10
@szaimen
Copy link
Contributor

szaimen commented Aug 31, 2021

What is missing here?

@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@CarlSchwan CarlSchwan force-pushed the work/carl/idn-url-theming branch from 1fd5538 to bc4d71b Compare October 21, 2021 16:03
@CarlSchwan CarlSchwan added the bug label Oct 21, 2021
@CarlSchwan CarlSchwan force-pushed the work/carl/idn-url-theming branch from bc4d71b to 604ee19 Compare October 22, 2021 10:06
@blizzz blizzz mentioned this pull request Sep 9, 2022
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
This was referenced Sep 20, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz mentioned this pull request Feb 1, 2023
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
@blizzz blizzz mentioned this pull request Mar 7, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 17, 2023
@szaimen
Copy link
Contributor

szaimen commented Apr 17, 2023

conflicts :/

This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
This was referenced Nov 14, 2023
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv skjnldsv closed this Mar 10, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Mar 10, 2024
@skjnldsv skjnldsv deleted the work/carl/idn-url-theming branch March 14, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug pending documentation This pull request needs an associated documentation update technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IDN domain name not allowed in /settings/admin/theming
10 participants