Bug 21973 - CAS URL escaped twice, preventing login
Summary: CAS URL escaped twice, preventing login
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 21526
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-07 10:25 UTC by Stefan Berndtsson
Modified: 2018-12-17 12:06 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21973: Do not url escape casServerUrl (1.84 KB, patch)
2018-12-07 13:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21973: Do not url escape casServerUrl (1.83 KB, patch)
2018-12-10 18:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21973: Do not url escape casServerUrl (1.91 KB, patch)
2018-12-13 14:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21973: Do not url escape casServerUrl (1.95 KB, patch)
2018-12-13 16:08 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Berndtsson 2018-12-07 10:25:40 UTC
Bug 21526 applies the "url" filter on the CAS login link despite it being escaped already in C4/Auth_with_cas.pm, resulting in a double escape and a non-functioning login via CAS.
Comment 1 Jonathan Druart 2018-12-07 13:29:35 UTC
Created attachment 82978 [details] [review]
Bug 21973: Do not url escape casServerUrl

It is already escaped correctly in
C4::Auth_with_cas::_url_with_get_params using URI::Escape::uri_escape

Note that shibbolethLoginUrl is not and must be url escaped in template
("be consistent, they said")
Comment 2 Owen Leonard 2018-12-10 18:02:25 UTC
Created attachment 83022 [details] [review]
Bug 21973: Do not url escape casServerUrl

It is already escaped correctly in
C4::Auth_with_cas::_url_with_get_params using URI::Escape::uri_escape

Note that shibbolethLoginUrl is not and must be url escaped in template
("be consistent, they said")
Comment 3 Kyle M Hall 2018-12-13 14:23:12 UTC
Created attachment 83169 [details] [review]
Bug 21973: Do not url escape casServerUrl

It is already escaped correctly in
C4::Auth_with_cas::_url_with_get_params using URI::Escape::uri_escape

Note that shibbolethLoginUrl is not and must be url escaped in template
("be consistent, they said")

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Martin Renvoize 2018-12-13 16:08:29 UTC
Created attachment 83178 [details] [review]
Bug 21973: Do not url escape casServerUrl

It is already escaped correctly in
C4::Auth_with_cas::_url_with_get_params using URI::Escape::uri_escape

Note that shibbolethLoginUrl is not and must be url escaped in template
("be consistent, they said")

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2018-12-13 16:09:27 UTC
Be nice to fix this in with Auth_with_cas or Auth_with_shibboleth to be consistent.. but that work shouldn't hold this patch up.
Comment 6 Nick Clemens 2018-12-14 15:33:28 UTC
Awesome work all!

Pushed to master for 19.05
Comment 7 Martin Renvoize 2018-12-17 12:06:17 UTC
Pushed to 18.11.x for 18.11.01