Description
Martin Renvoize
2022-10-06 13:36:25 UTC
Created attachment 141420 [details] [review] Bug 31699: Add 'return' option to opac modal login This patch adds the option to trigger a redirect back to the current context after successful login using modal logins triggered from various places in the OPAC. To make use of this, simply add 'data-return="true"' to the modal trigger link. Created attachment 141421 [details] [review] Bug 31699: Impliment modal context return on opac-detail. This patch utilises the generic return option introduced in the previous patch to prevent lose of context when clicking to login to add a comment to a biblio. Test plan 1/ Test that prior to this patch, upon clicking 'Log in to your account to post a comment' you are redirected to opac-user.pl and thus loose your context. 2/ Apply the patch 3/ Confirm that a login using the 'Log in to your account to post a comment' link now redirect immediatly back the correct record and thus maintains context. Hi Martin. This isn't working for me: - I tried in Firefox and Google Chrome - I applied the patch, flush_memcached, and restart_all - For browsers, I tried clearing the cache and opening in ingognito mode/private window - Visited http://127.0.0.1:8080/cgi-bin/koha/opac-detail.pl?biblionumber=262 from both the search results page and going to the URL directly - Link for "Log in to your account to post a comment." is showing the changes in the patch (<a class="login-link loginModal-trigger" role="button" data-toggle="modal" data-return="true" href="/cgi-bin/koha/opac-user.pl">Log in to your account</a> to post a comment.) David Created attachment 142200 [details] [review] Bug 31699: Add 'return' option to opac modal login This patch adds the option to trigger a redirect back to the current context after successful login using modal logins triggered from various places in the OPAC. To make use of this, simply add 'data-return="true"' to the modal trigger link. Created attachment 142201 [details] [review] Bug 31699: Impliment modal context return on opac-detail. This patch utilises the generic return option introduced in the previous patch to prevent lose of context when clicking to login to add a comment to a biblio. Test plan 1/ Test that prior to this patch, upon clicking 'Log in to your account to post a comment' you are redirected to opac-user.pl and thus loose your context. 2/ Apply the patch 3/ Confirm that a login using the 'Log in to your account to post a comment' link now redirect immediatly back the correct record and thus maintains context. Thanks for testing David.. something wierd had crept in.. I've amended the patch (checking for boolean truthyness instead of comparing to a string) and now it appears to be working again for me. Created attachment 142204 [details] [review] Bug 31699: Add 'return' option to opac modal login This patch adds the option to trigger a redirect back to the current context after successful login using modal logins triggered from various places in the OPAC. To make use of this, simply add 'data-return="true"' to the modal trigger link. Created attachment 142205 [details] [review] Bug 31699: Impliment modal context return on opac-detail. This patch utilises the generic return option introduced in the previous patch to prevent lose of context when clicking to login to add a comment to a biblio. Test plan 1/ Test that prior to this patch, upon clicking 'Log in to your account to post a comment' you are redirected to opac-user.pl and thus loose your context. 2/ Apply the patch 3/ Confirm that a login using the 'Log in to your account to post a comment' link now redirect immediatly back the correct record and thus maintains context. Signed-off-by: David Nind <david@davidnind.com> (In reply to Martin Renvoize from comment #6) > Thanks for testing David.. something wierd had crept in.. I've amended the > patch (checking for boolean truthyness instead of comparing to a string) and > now it appears to be working again for me. Thanks Martin - this now works. I'm redirected back to the details page for the record I was viewing (but not the comments tab). This works well and is an improvement. Tiny glitch: We return to the same page, but the active tab is 'holdings', instead of 'comments' now. I was wondering: Maybe it would be nice to be able to set the return URL in the data attribute? Then we could also put things like the anchor for the tab. I don't think this is worth failing this tho :) Created attachment 142733 [details] [review] Bug 31699: Add 'return' option to opac modal login This patch adds the option to trigger a redirect back to the current context after successful login using modal logins triggered from various places in the OPAC. To make use of this, simply add 'data-return="true"' to the modal trigger link. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 142734 [details] [review] Bug 31699: Implement modal context return on opac-detail This patch utilises the generic return option introduced in the previous patch to prevent lose of context when clicking to login to add a comment to a biblio. Test plan 1/ Test that prior to this patch, upon clicking 'Log in to your account to post a comment' you are redirected to opac-user.pl and thus loose your context. 2/ Apply the patch 3/ Confirm that a login using the 'Log in to your account to post a comment' link now redirect immediatly back the correct record and thus maintains context. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Katrin Fischer from comment #10) > This works well and is an improvement. > > Tiny glitch: We return to the same page, but the active tab is 'holdings', > instead of 'comments' now. I was wondering: Maybe it would be nice to be > able to set the return URL in the data attribute? Then we could also put > things like the anchor for the tab. > > I don't think this is worth failing this tho :) So this isn't actually about setting the return url.. we already do that.. it's that the # values in the URI are local to the browser and not taken into account on return. I actually added code to deal with this for my catalog concerns bug, but for re-triggering the modal rather than switching tab.. It will need custom JS per case though I believe. Example of what I did for catalog concerns: let urlParams = new URLSearchParams(window.location.search); if ( urlParams.has('modal') ) { let modal = urlParams.get('modal'); if ( modal == 'concern' ) { $("#addConcernModal").modal('show'); } } We'd need to do something similar for 'tab' to trigger the JS to select the right tab after page load. Created attachment 143378 [details] [review] Bug 31699: Add 'return' option to opac modal login This patch adds the option to trigger a redirect back to the current context after successful login using modal logins triggered from various places in the OPAC. To make use of this, simply add 'data-return="true"' to the modal trigger link. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> I decided to split this into two bugs.. one for the generic return code and one for the use of it in the biblio details comments tab. I also added a minor follow-up on the subsequent bug to ensure the correct tab is selected too. Keeping the QA stamp here. Do we need some form of validation for $return? Hmm, probably a good idea... You thinking something like a test that at the base matches OpacBaseURL? (In reply to Martin Renvoize from comment #17) > Hmm, probably a good idea... You thinking something like a test that at the > base matches OpacBaseURL? At least the domain used to retrieve the page (maybe calculated). (In reply to Tomás Cohen Arazi from comment #16) > Do we need some form of validation for $return? 100% The "return" param could either be window.location.pathname + window.location.search so the Perl could append that to OpacBaseURL, or "return" could be a full URL, and the Perl replaces the hostname with OpacBaseURL. This is the approach we use in C4::Auth_with_shibboleth, and it's something I've used elsewhere as well. The only downside I see is that it locks you into using the particular OpacBaseURL you have defined for that virtual host. Either way, we definitely need to validate/construct the URL, or else we're creating this problem: https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html I'd be tempted to mark this as Failed QA until it addresses the security hole, but I'll leave it up to the RM whether we do that or add a follow-up Sorry chaps, it was late last night when this discussion took place. I've marked it assigned and will work on it today. Bit disappointed with myself that I didn't spot this at initial development. Still, it should be a trivial follow-up. Created attachment 143463 [details] [review] Bug 31699: (follow-up) Protect against unauthorized redirects Created attachment 143464 [details] [review] Bug 31699: Add 'return' option to opac modal login This patch adds the option to trigger a redirect back to the current context after successful login using modal logins triggered from various places in the OPAC. To make use of this, simply add 'data-return="true"' to the modal trigger link. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143465 [details] [review] Bug 31699: (follow-up) Protect against unauthorized redirects Created attachment 143468 [details] [review] Bug 31699: Add 'return' option to opac modal login This patch adds the option to trigger a redirect back to the current context after successful login using modal logins triggered from various places in the OPAC. To make use of this, simply add 'data-return="true"' to the modal trigger link. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 143469 [details] [review] Bug 31699: (follow-up) Protect against unauthorized redirects (In reply to Martin Renvoize from comment #21) > Sorry chaps, it was late last night when this discussion took place. I've > marked it assigned and will work on it today. > > Bit disappointed with myself that I didn't spot this at initial development. > Still, it should be a trivial follow-up. No worries. We're all human and it's why it's always good to have another set of eyes on things! Comment on attachment 143469 [details] [review] Bug 31699: (follow-up) Protect against unauthorized redirects Review of attachment 143469 [details] [review]: ----------------------------------------------------------------- ::: opac/opac-user.pl @@ +428,5 @@ > # back to the page we triggered the login from > my $return = $query->param('return'); > if ( $return ) { > + my $uri = C4::Context->preference('OPACBaseURL'); > + $uri .= $return; If OPACBaseURL isn't set, which happens more often than I'd like, this would still be vulnerable to open redirects. I was thinking that we'd re-write the URL using the URI module. Today, I thought we could potentially get away with nuking the "scheme" and "authority", but then I figured out a URL trick to get it to redirect to a malicious URL. So I think we need to do a bit more validation. I'll write up a quick patch. The last follow-up has "broken" the redirect used for bug 31028 - Catalog concerns (see comment 323 (!!)), so it may pay to test with that bug. (In reply to David Nind from comment #29) > The last follow-up has "broken" the redirect used for bug 31028 - Catalog > concerns (see comment 323 (!!)), so it may pay to test with that bug. That's interesting. I don't think it would've been the follow-up, so much as the whole feature. How were you setting the data-return attribute? I think that might be part of your problem? (Interestingly, it looks like opac-reserve.pl might be broken in master as well on an unrelated note...) At the moment, with Martin's patches, the following will generate a "ERR_INVALID_REDIRECT": http://localhost:8080/cgi-bin/koha/opac-user.pl?return=http://koha-community.org/test If I erase OPACBaseURL, then I get the open redirect vulnerability again. So now I'll add a patch... (In reply to David Cook from comment #31) > If I erase OPACBaseURL, then I get the open redirect vulnerability again. Worth noting that this affects only logged in users. The redirect code won't trigger for anonymous users, since we short-circuit opac-user.pl for anonymous users and prompt with a regular login box. With my upcoming patch, Without OPACBaseURL, no redirection happens for the following: http://localhost:8080/cgi-bin/koha/opac-user.pl?return=http://koha-community.org/test?test=test With OPACBaseURL, the return URL is rewritten to use the scheme and authority of OPACBaseURL, so that the following: http://localhost:8080/cgi-bin/koha/opac-user.pl?return=http://koha-community.org/test?test=test#test Redirects to: http://localhost:8080/test?test=test#test -- So it shouldn't be possible to send a user anywhere other than http://localhost:8080. (An alternative would be to set the page visited in the user's database session and then juggle that around and try to return to that, but that would be a more difficult code change than Martin's patches here.) Created attachment 143608 [details] [review] Bug 31699: (follow-up) Protect more against open redirects This change checks that the OPACBaseURL exists, and uses its scheme and authority to rewrite the URL passed through the "return" param. Note that while the code checks for URI::http as the URI class type, this includes URI::https since it subclasses URI::http, so it works for OPACBaseURL using either HTTP or HTTPS URLs. I was doing some more thinking... On one hand, I was thinking maybe we should make a more consistent login box. For instance, if you go to http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=add_form, the opac-auth.tt template creates a login box that works as expected. But the modal sends you off to opac-user.pl. There's no reason the masthead.inc couldn't create the same login box as opac-auth.tt. On the other hand, I like the idea of login endpoints (like with SSO), which need to redirect you back, so maybe this redirect is fine (so long as we're rewriting/validating the redirect URLs) Created attachment 143643 [details] [review] Bug 31699: (follow-up) Protect more against open redirects This change checks that the OPACBaseURL exists, and uses its scheme and authority to rewrite the URL passed through the "return" param. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks for working on this David, your follow-up is indeed a much more thorough way of doing things.. my approach was a little lazy. Works great, Hi, I'm still not convinced, to be honest. In production it is common to use several domain names for the same Koha instance, and I don't think sticking to OpacBaseURL is enough for that use case. In my opinion, using OpacBaseURL as a fixed value (instead of an allow-list) is a design mistake because of what I mentioned above. We should somehow be using x-forwarded-for or similar header. I know solving it is not this bug's responsability. But I think we should be able to find a nicer approach. That said, this is a bugfix and we can still think about it and if it doesn't make it to the release date it will be backported for sure. We use SetEnvIf to overcome the OpacBaseUrl thing already here.. your already setting up a host to get the different domains.. (In reply to Martin Renvoize from comment #40) > We use SetEnvIf to overcome the OpacBaseUrl thing already here.. your > already setting up a host to get the different domains.. I'm sorry, I'm really overwhelmed by the amount of stuffs that need to be done/fixed for the release. This can be clearly considered a bugfix and pushed right after the release and backported if required. I can commit to that. I'm really sorry, but my brain bandwidth is exhausted right now. (In reply to Tomás Cohen Arazi from comment #39) > In my opinion, using OpacBaseURL as a fixed value (instead of an allow-list) > is a design mistake because of what I mentioned above. I was thinking about that a bit as well. With the OIDC server, they use an allow list to great effect. I figure Koha would possibly benefit in a few different places from URL redirect allow lists. (In reply to Martin Renvoize from comment #40) > We use SetEnvIf to overcome the OpacBaseUrl thing already here.. your > already setting up a host to get the different domains.. But I also agree with this in terms of addressing the practical problem. (In reply to Tomás Cohen Arazi from comment #41) > I'm really sorry, but my brain bandwidth is exhausted right now. You've been doing so much! We all understand! Created attachment 144644 [details] [review] Bug 31699: Add 'return' option to opac modal login This patch adds the option to trigger a redirect back to the current context after successful login using modal logins triggered from various places in the OPAC. To make use of this, simply add 'data-return="true"' to the modal trigger link. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 144645 [details] [review] Bug 31699: (follow-up) Protect against unauthorized redirects Created attachment 144646 [details] [review] Bug 31699: (follow-up) Protect more against open redirects This change checks that the OPACBaseURL exists, and uses its scheme and authority to rewrite the URL passed through the "return" param. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Rebased No more rebases needed :-D Pushed to master for 23.05. Nice work everyone, thanks! Nice work, thanks everyone! Pushed to 22.11.x for the next release. Enhancement will not be backported to 22.05.x Could this be used to solve bug 33175? |