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

Fix redundant urldecode() #13137

Closed
wants to merge 1 commit into from
Closed

Fix redundant urldecode() #13137

wants to merge 1 commit into from

Conversation

soudis
Copy link

@soudis soudis commented Dec 18, 2018

Fixes #6822

@skjnldsv
Copy link
Member

Thanks for your contribution, please sign your work when committing!

https://github.com/nextcloud/appstore/blob/master/CONTRIBUTING.md#sign-your-work

@soudis soudis changed the title Fix redundant urldecode() Fix redundant urldecode() Signed-off-by: Random J Developer <random@developer.example.org> Dec 19, 2018
@soudis soudis changed the title Fix redundant urldecode() Signed-off-by: Random J Developer <random@developer.example.org> Fix redundant urldecode() Dec 19, 2018
@soudis
Copy link
Author

soudis commented Dec 19, 2018

is it sufficient to do this here?

Signed-off-by: Florian Humer soudis@gmx.at

@skjnldsv
Copy link
Member

@soudis for such small change, yes! 👍 😉

@nickvergessen nickvergessen requested review from rullzer, MorrisJobke and ChristophWurst and removed request for rullzer and MorrisJobke January 8, 2019 14:28
@MorrisJobke
Copy link
Member

Unit tests fail:

1) Tests\Core\Controller\LoginControllerTest::testLoginWithoutPassedCsrfCheckAndLoggedIn
Expectation failed for method name is equal to "getAbsoluteURL" when invoked 1 time(s)
Parameter 0 for invocation OCP\IURLGenerator::getAbsoluteURL('another%20url'): string does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'another url'
+'another%20url'

/drone/src/github.com/nextcloud/server/core/Controller/LoginController.php:254
/drone/src/github.com/nextcloud/server/core/Controller/LoginController.php:287
/drone/src/github.com/nextcloud/server/tests/Core/Controller/LoginControllerTest.php:558

2) Tests\Core\Controller\LoginControllerTest::testLoginWithValidCredentialsAndRedirectUrl
Expectation failed for method name is equal to "getAbsoluteURL" when invoked 1 time(s)
Parameter 0 for invocation OCP\IURLGenerator::getAbsoluteURL('another%20url'): string does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'another url'
+'another%20url'

/drone/src/github.com/nextcloud/server/core/Controller/LoginController.php:254
/drone/src/github.com/nextcloud/server/core/Controller/LoginController.php:373
/drone/src/github.com/nextcloud/server/tests/Core/Controller/LoginControllerTest.php:595

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 7, 2019
@ChristophWurst
Copy link
Member

Apparently this was submitted twice, so I closed the other PR #10681.

This was referenced Mar 4, 2019
@rullzer rullzer mentioned this pull request Mar 26, 2019
9 tasks
@MorrisJobke MorrisJobke mentioned this pull request Apr 3, 2019
2 tasks
@MorrisJobke MorrisJobke modified the milestones: Nextcloud 16, Nextcloud 17 Apr 3, 2019
@leonklingele-work
Copy link

This fixes an issue where logged out users accessing
/ocs/v2.php/cloud/users/details?search=leon%3Ftest are redirected to
/ocs/v2.php/cloud/users/details?search=leon?test after logging in whereas
/ocs/v2.php/cloud/users/details?search=leon%3Ftest is the correct redirect URL.

@soudis
Copy link
Author

soudis commented Apr 21, 2019

I do not really know the whole unit test area of this project very well, e.g. how they come about and who makes them, but it seems that the unit test that is failing is tailored to the current wrong behaviour of this function. Can a more experienced person take on this and have a look? The unit test is the only thing blocking this change to finally get in. @ChristophWurst maybe?

Also @leonklingele-work now also found a case where this is a problem without the discoursesso app.

Thanks in advance!

@nickvergessen
Copy link
Member

Since the origin repo does not exist anymore, someone needs to redo the patch

@leonklingele-work
Copy link

$ wget -qO- https://github.com/nextcloud/server/pull/13137.patch | git am

?

@nickvergessen
Copy link
Member

Feel free to do it and also fix the tests while you are on it:
#13137 (comment)

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

redirect_url seems to be double URL-decoded after login
6 participants