Skip to content

Validate overwrite.cli.url to be a url in setup check - #31430

Merged
szaimen merged 1 commit into
masterfrom
enh/31429/improve-overwrite-cli-url-check
Mar 10, 2022
Merged

Validate overwrite.cli.url to be a url in setup check#31430
szaimen merged 1 commit into
masterfrom
enh/31429/improve-overwrite-cli-url-check

Conversation

@szaimen

@szaimenszaimen commented Mar 3, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Close#31429

Signed-off-by: szaimen szaimen@e.mail.de

@szaimenszaimen added this to the Nextcloud 24 milestone Mar 3, 2022
@szaimen
szaimenforce-pushed the enh/31429/improve-overwrite-cli-url-check branch 10 times, most recently from dec1fec to 141177cCompareMarch 3, 2022 19:11
@nextcloud-command
nextcloud-commandforce-pushed the enh/31429/improve-overwrite-cli-url-check branch from 141177c to 225cc2eCompareMarch 3, 2022 22:17
@szaimen

szaimen commented Mar 3, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable23

@szaimen

szaimen commented Mar 3, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable22

@szaimen
szaimen marked this pull request as ready for review March 3, 2022 22:33
@szaimen
szaimen requested review from a team, CarlSchwan, nickvergessen and skjnldsv and removed request for a teamMarch 3, 2022 22:33
@szaimenszaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 3, 2022
@szaimen
szaimen requested a review from MichaIngMarch 3, 2022 22:34
szaimen
szaimen commented Mar 3, 2022
View reviewed changes
Comment threadapps/settings/lib/Controller/CheckSetupController.php
nickvergessen
nickvergessen previously requested changes Mar 4, 2022
View reviewed changes

@nickvergessennickvergessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you state what the improvement is?
From reading the code the improvement is that you can now have false positives for a warning?

Comment threadapps/settings/lib/Controller/CheckSetupController.php Outdated
Comment threadapps/settings/lib/Controller/CheckSetupController.php Outdated
@szaimenszaimen removed the 3. to review Waiting for reviews label Mar 4, 2022
@szaimenszaimen added the 2. developing Work in progress label Mar 4, 2022
szaimen
szaimen commented Mar 4, 2022
View reviewed changes
Comment threadapps/settings/lib/Controller/CheckSetupController.php Outdated
@nickvergessennickvergessen changed the title improve overwrite cli url setup checkValidate overwrite.cli.url to be a url in setup checkMar 4, 2022
@szaimenszaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 4, 2022
@szaimen
szaimenforce-pushed the enh/31429/improve-overwrite-cli-url-check branch from e0d48e9 to 1947ed3CompareMarch 4, 2022 09:51
juliusknorr
juliusknorr reviewed Mar 4, 2022
View reviewed changes
Comment threadapps/settings/lib/Controller/CheckSetupController.php
szaimen
szaimen commented Mar 4, 2022
View reviewed changes
Comment threadapps/settings/lib/Controller/CheckSetupController.php Outdated
@szaimen
improve overwrite cli url setup check
4191a17
Signed-off-by: szaimen <szaimen@e.mail.de>
@szaimen
szaimenforce-pushed the enh/31429/improve-overwrite-cli-url-check branch from e58122e to 4191a17CompareMarch 9, 2022 21:26
szaimen
szaimen commented Mar 9, 2022
View reviewed changes
Comment threadapps/settings/lib/Controller/CheckSetupController.php
$suggestedOverwriteCliUrl = $this->request->getServerProtocol() . '://' . $this->request->getInsecureServerHost() . \OC::$WEBROOT;

// Check correctness by checking if it is a valid URL
if (filter_var($currentOverwriteCliUrl, FILTER_VALIDATE_URL)) {

@szaimenszaimenMar 9, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Is everyone, especially @juliushaertl fine if we do it like this for now?
After merging, I will rebase Carls PR and change it there to use his validation logic but since I want to backport this we will not have the validation logic in place.
For me the most important change of this PR is to make admins aware of this config which the PR does. Since the warning is only shown in the admin panel, it is in my opinion no big problem if it is sometimes false-positive because of this unperfect filter and will not cause any harm.

@juliusknorrjuliusknorrMar 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, fine with me :)

@szaimenszaimenMar 10, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

great, thanks! :)

@PVince81PVince81Mar 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@CarlSchwanCarlSchwanMar 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@szaimen

szaimen commented Mar 10, 2022

Copy link
Copy Markdown
ContributorAuthor

Thanks everyone! Merging then :)

@szaimen
szaimen merged commit 3a0b934 into masterMar 10, 2022
@szaimen
szaimen deleted the enh/31429/improve-overwrite-cli-url-check branch March 10, 2022 11:41
@szaimen

szaimen commented Mar 10, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable23

@szaimen

szaimen commented Mar 10, 2022

Copy link
Copy Markdown
ContributorAuthor

/backport to stable22

@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Mar 10, 2022
Merged
@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Mar 10, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PVince81PVince81PVince81 left review comments
@juliusknorrjuliusknorrjuliusknorr approved these changes
@CarlSchwanCarlSchwanCarlSchwan approved these changes
@skjnldsvskjnldsvAwaiting requested review from skjnldsv
@MichaIngMichaIngAwaiting requested review from MichaIng
@nickvergessennickvergessenAwaiting requested review from nickvergessen
@julien-ncjulien-ncAwaiting requested review from julien-nc

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsbugenhancement

Projects

None yet

Milestone

Nextcloud 24

Development

Successfully merging this pull request may close these issues.

improve setup test that checks the overwrite cli url

5 participants

@szaimen@nickvergessen@PVince81@juliusknorr@CarlSchwan