Bug 31699 - Add a generic way to redirect back to the page you were on at login for modal logins
Summary: Add a generic way to redirect back to the page you were on at login for modal...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks: 21135 30979 31028 32125
  Show dependency treegraph
 
Reported: 2022-10-06 13:36 UTC by Martin Renvoize
Modified: 2023-12-28 20:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
**Sponsored by** *The European Southern Observatory* This enhancement adds the ability to redirect users back to where they were when using the modal type logins in place of an action that requires login on the OPAC. Example: On the OPAC detail page you can add comments if logged in. Prior to this patch, clicking the link to add a comment prior to being logged in would expose the login modal and then re-direct you to your OPAC user page, and thus lose the context of your action. With this enhancement, you are redirected back to the record you were looking at and can then post your comment.
Version(s) released in:
23.05.00,22.11.03


Attachments
Bug 31699: Add 'return' option to opac modal login (1.85 KB, patch)
2022-10-06 13:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: Impliment modal context return on opac-detail. (1.89 KB, patch)
2022-10-06 13:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: Add 'return' option to opac modal login (1.84 KB, patch)
2022-10-19 18:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: Impliment modal context return on opac-detail. (1.89 KB, patch)
2022-10-19 18:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: Add 'return' option to opac modal login (1.84 KB, patch)
2022-10-19 19:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 31699: Impliment modal context return on opac-detail. (1.94 KB, patch)
2022-10-19 19:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 31699: Add 'return' option to opac modal login (1.90 KB, patch)
2022-10-27 21:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31699: Implement modal context return on opac-detail (1.99 KB, patch)
2022-10-27 21:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31699: Add 'return' option to opac modal login (2.01 KB, patch)
2022-11-07 17:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: (follow-up) Protect against unauthorized redirects (1.54 KB, patch)
2022-11-09 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: Add 'return' option to opac modal login (2.01 KB, patch)
2022-11-09 08:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: (follow-up) Protect against unauthorized redirects (1.54 KB, patch)
2022-11-09 08:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: Add 'return' option to opac modal login (2.01 KB, patch)
2022-11-09 08:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: (follow-up) Protect against unauthorized redirects (1.55 KB, patch)
2022-11-09 08:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: (follow-up) Protect more against open redirects (1.63 KB, patch)
2022-11-10 00:03 UTC, David Cook
Details | Diff | Splinter Review
Bug 31699: (follow-up) Protect more against open redirects (1.69 KB, patch)
2022-11-10 12:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: Add 'return' option to opac modal login (2.01 KB, patch)
2022-12-16 11:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: (follow-up) Protect against unauthorized redirects (1.56 KB, patch)
2022-12-16 11:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 31699: (follow-up) Protect more against open redirects (1.69 KB, patch)
2022-12-16 11:28 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 Martin Renvoize 2022-10-06 13:36:25 UTC
Currently, we have a few extra buttons in the OPAC that trigger the login modal.. however, in most cases those don't do anything clever to keep the users context and instead they result with the jarring shift to the opac-user page once logged in.

We should implement a general redirect option to allow such modal trigger to specify that context should be maintained.

Example  - Trying to add a comment on the opac-detail page when not logged in.  It'll trigger a login modal and then redirect you to opac-user upon success losing the context of the item you were trying to comment on.
Comment 1 Martin Renvoize 2022-10-06 13:45:01 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.
Comment 2 Martin Renvoize 2022-10-06 13:45:05 UTC
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.
Comment 3 David Nind 2022-10-06 19:11:36 UTC
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
Comment 4 Martin Renvoize 2022-10-19 18:57:28 UTC
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.
Comment 5 Martin Renvoize 2022-10-19 18:57:32 UTC
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.
Comment 6 Martin Renvoize 2022-10-19 18:58:52 UTC
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.
Comment 7 David Nind 2022-10-19 19:18:58 UTC
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.
Comment 8 David Nind 2022-10-19 19:19:03 UTC
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>
Comment 9 David Nind 2022-10-19 19:20:50 UTC
(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).
Comment 10 Katrin Fischer 2022-10-27 20:58:01 UTC
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 :)
Comment 11 Katrin Fischer 2022-10-27 21:00:18 UTC
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>
Comment 12 Katrin Fischer 2022-10-27 21:00:24 UTC
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>
Comment 13 Martin Renvoize 2022-10-28 08:09:18 UTC
(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.
Comment 14 Martin Renvoize 2022-11-07 17:17:10 UTC
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>
Comment 15 Martin Renvoize 2022-11-07 17:18:52 UTC
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.
Comment 16 Tomás Cohen Arazi 2022-11-08 17:50:54 UTC
Do we need some form of validation for $return?
Comment 17 Martin Renvoize 2022-11-08 19:52:16 UTC
Hmm, probably a good idea... You thinking something like a test that at the base matches OpacBaseURL?
Comment 18 Tomás Cohen Arazi 2022-11-08 19:55:40 UTC
(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).
Comment 19 David Cook 2022-11-08 22:38:42 UTC
(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
Comment 20 David Cook 2022-11-08 22:39:37 UTC
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
Comment 21 Martin Renvoize 2022-11-09 07:10:29 UTC
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.
Comment 22 Martin Renvoize 2022-11-09 08:28:26 UTC Comment hidden (obsolete)
Comment 23 Martin Renvoize 2022-11-09 08:28:57 UTC Comment hidden (obsolete)
Comment 24 Martin Renvoize 2022-11-09 08:29:02 UTC Comment hidden (obsolete)
Comment 25 Martin Renvoize 2022-11-09 08:48:57 UTC
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>
Comment 26 Martin Renvoize 2022-11-09 08:49:03 UTC
Created attachment 143469 [details] [review]
Bug 31699: (follow-up) Protect against unauthorized redirects
Comment 27 David Cook 2022-11-09 23:02:03 UTC
(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 28 David Cook 2022-11-09 23:22:18 UTC
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.
Comment 29 David Nind 2022-11-09 23:27:17 UTC
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.
Comment 30 David Cook 2022-11-09 23:45:58 UTC
(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...)
Comment 31 David Cook 2022-11-09 23:47:14 UTC
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...
Comment 32 David Cook 2022-11-09 23:48:05 UTC
(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.
Comment 33 David Cook 2022-11-09 23:58:55 UTC
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.)
Comment 34 David Cook 2022-11-10 00:03:32 UTC
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.
Comment 35 David Cook 2022-11-10 00:05:57 UTC
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.
Comment 36 David Cook 2022-11-10 00:18:42 UTC
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)
Comment 37 Martin Renvoize 2022-11-10 12:13:20 UTC
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>
Comment 38 Martin Renvoize 2022-11-10 12:18:05 UTC
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,
Comment 39 Tomás Cohen Arazi 2022-11-10 13:53:19 UTC
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.
Comment 40 Martin Renvoize 2022-11-10 18:34:35 UTC
We use SetEnvIf to overcome the OpacBaseUrl thing already here.. your already setting up a host to get the different domains..
Comment 41 Tomás Cohen Arazi 2022-11-10 19:59:04 UTC
(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.
Comment 42 David Cook 2022-11-10 22:10:58 UTC
(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!
Comment 43 Martin Renvoize 2022-12-16 11:28:23 UTC
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>
Comment 44 Martin Renvoize 2022-12-16 11:28:28 UTC
Created attachment 144645 [details] [review]
Bug 31699: (follow-up) Protect against unauthorized redirects
Comment 45 Martin Renvoize 2022-12-16 11:28:33 UTC
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>
Comment 46 Martin Renvoize 2022-12-16 11:44:15 UTC
Rebased
Comment 47 Tomás Cohen Arazi 2023-02-09 13:03:40 UTC
No more rebases needed :-D
Comment 48 Tomás Cohen Arazi 2023-02-09 13:22:53 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 49 Jacob O'Mara 2023-02-13 09:46:00 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 50 Lucas Gass 2023-02-15 22:11:19 UTC
Enhancement will not be backported to 22.05.x
Comment 51 Caroline Cyr La Rose 2023-04-12 13:41:10 UTC
Could this be used to solve bug 33175?