[Bug]: Workflow non-regexp passes regexp-validation sometimes, UX issue

PVince81 commented 1 year ago

⚠️ This issue respects the following points: ⚠️

[X] This is a bug, not a question or a configuration/webserver/proxy issue.
[X] This issue is not already reported on Github (I've searched it).
[X] Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
[X] Nextcloud Server is running on 64bit capable CPU, PHP and OS.
[X] I agree to follow Nextcloud's Code of Conduct.

Bug description

When selecting "Folder" in it says "The given regular expression is invalid". I've expanded the error message and saw that the string is "httpd/unix-directory" which is technically not a regexp.

In the code, I see that it calls preg_match() to validate it: https://github.com/nextcloud/server/blob/v24.0.8/apps/workflowengine/lib/Check/AbstractStringCheck.php#L93

According to @mahibi the validation accepted that string on PHP 8.0.26. However for me on PHP 8.1.13 the validation failed.

I suspect that perhaps PHP 8.1 got more strict with regular expressions.

Steps to reproduce

Setup NC 24.0.8 with PHP 8.1
Install the files_accessoncontrol workflow
Create a workflow and select "File MIME type", "does not match" and then select "Folder".
Click Save

Expected behavior

Folder rule can be saved and works.

Installation method

Community Manual installation with Archive

Operating system

Other

PHP engine version

PHP 8.1

Web server

Apache (supported)

Database engine version

MariaDB

Is this bug present after an update or on a fresh install?

Fresh Nextcloud Server install

Are you using the Nextcloud Server Encryption module?

Encryption is Disabled

What user-backends are you using?

[X] Default user-backend (database)
[ ] LDAP/ Active Directory
[ ] SSO - SAML
[ ] Other

Configuration report

{
    "system": {
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "vvortex-release.local"
        ],
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "mysql",
        "version": "24.0.8.3",
        "overwrite.cli.url": "https:\/\/vvortex-release.local",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbport": "",
        "dbtableprefix": "oc_",
        "mysql.utf8mb4": true,
        "loglevel": 0,
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "installed": true,
        "objectstore": {
            "class": "OC\\Files\\ObjectStore\\S3",
            "arguments": {
                "bucket": "nextcloud-dev",
                "key": "***REMOVED SENSITIVE VALUE***",
                "secret": "***REMOVED SENSITIVE VALUE***",
                "hostname": "localhost",
                "port": "9000",
                "use_ssl": false,
                "use_path_style": true
            }
        }
    }
}

List of activated Apps

Enabled:
  - accessibility: 1.10.0
  - activity: 2.16.0
  - bruteforcesettings: 2.4.0
  - calendar: 3.5.3
  - circles: 24.0.1
  - cloud_federation_api: 1.7.0
  - comments: 1.14.0
  - contacts: 4.2.3
  - contactsinteraction: 1.5.0
  - dashboard: 7.4.0
  - dav: 1.22.0
  - federatedfilesharing: 1.14.0
  - federation: 1.14.0
  - files: 1.19.0
  - files_accesscontrol: 1.14.1
  - files_pdfviewer: 2.5.0
  - files_retention: 1.13.2
  - files_rightclick: 1.3.0
  - files_sharing: 1.16.2
  - files_trashbin: 1.14.0
  - files_versions: 1.17.0
  - files_videoplayer: 1.13.0
  - firstrunwizard: 2.13.0
  - logreader: 2.9.0
  - lookup_server_connector: 1.12.0
  - mail: 1.14.5
  - nextcloud_announcements: 1.13.0
  - notifications: 2.12.1
  - oauth2: 1.12.0
  - password_policy: 1.14.0
  - photos: 1.6.0
  - privacy: 1.8.0
  - provisioning_api: 1.14.0
  - recommendations: 1.3.0
  - richdocuments: 6.3.2
  - richdocumentscode: 22.5.802
  - serverinfo: 1.14.0
  - settings: 1.6.0
  - sharebymail: 1.14.0
  - spreed: 14.0.7
  - support: 1.7.0
  - survey_client: 1.12.0
  - systemtags: 1.14.0
  - text: 3.5.1
  - theming: 1.15.0
  - twofactor_backupcodes: 1.13.0
  - updatenotification: 1.14.0
  - user_status: 1.4.0
  - viewer: 1.8.0
  - weather_status: 1.4.0
  - workflowengine: 2.6.0
Disabled:
  - admin_audit
  - approval
  - encryption
  - files_external
  - files_lock
  - globalsiteselector
  - user_ldap
  - user_saml

Nextcloud Signing status

No response

Nextcloud Logs

No related log entries.

Additional info

The presets for the dropdown are here: https://github.com/nextcloud/server/blob/v24.0.8/apps/workflowengine/src/components/Checks/FileMimeType.vue#L71

Should we convert these all to regular expressions, even for strict matching or should we loosen the validation code ? For folders specifically, I saw code in the workflow engine where it checked if the string is strictly "httpd/unix-directory" so adjusting the presets for regexp might need a more expanded fix.

@nickvergessen @juliushaertl @come-nc thoughts ?

I suspect that this fell under the radar when we added PHP 8.1 support

nickvergessen commented 1 year ago

Same issue with pdfs then? https://github.com/nextcloud/server/blob/v24.0.8/apps/workflowengine/src/components/Checks/FileMimeType.vue#L86

nickvergessen commented 1 year ago

So actually the problem is that it's a string and then should not use the "matches" but the "is" equation?

PVince81 commented 1 year ago

yes, likely same issue

and just now I noticed that we actually officially allow non-regexp input, at least the placeholder says so:

PVince81 commented 1 year ago

So actually the problem is that it's a string and then should not use the "matches" but the "is" equation?

ouch, I didn't notice that.

so it's more of a UX issue, we shouldn't allow selecting those presets when using "matches" and always having an input field

PVince81 commented 1 year ago

still, it looks like some people managed to pass validation despite using the presets and using "matches", it only did not work for me.

it might be best to fix the UI and leave the engine as is and think about what to do with setups where validation passed where it shouldn't

nextcloud / server