Bug 27961 - External track clicks links should get uri filtered
Summary: External track clicks links should get uri filtered
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Aleisha Amohia
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 24699 27979
Blocks:
  Show dependency treegraph
 
Reported: 2021-03-15 21:52 UTC by Aleisha Amohia
Modified: 2021-10-06 05:42 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05


Attachments
Bug 27961: Ensure external URLs are filtered (5.03 KB, patch)
2021-03-15 22:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 27961: Ensure external URLs are filtered (5.08 KB, patch)
2021-04-01 23:27 UTC, David Nind
Details | Diff | Splinter Review
Bug 27961: Ensure external URLs are filtered (5.17 KB, patch)
2021-04-16 07:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 27961: [20.05.x] Ensure external URLs are filtered (5.16 KB, patch)
2021-05-04 04:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2021-03-15 21:52:28 UTC
TT variables are used to build the link in items.uri . This is a problem particularly when TrackClicks is enabled because the items.uri value doesn't get escaped, resulting in a 404
Comment 1 Aleisha Amohia 2021-03-15 22:11:24 UTC
Created attachment 118256 [details] [review]
Bug 27961: Ensure external URLs are filtered

TT variables are used to build the link in items.uri . This is a problem
particularly when TrackClicks is enabled because the items.uri value
doesn't get escaped, resulting in a 404.

To test:
1) Enable the TrackClicks system preference (set to Track or Track
    anonymously)
2) Find a record with an item
3) Edit the item and put an external URL under the Uniform Resource
Identifier field
4) Go to that record in the OPAC
5) Click on Link to resource
6) Confirm you are not redirected to the URL and instead see a 404
7) Apply the patch and refresh the page
8) Click on Link to resource
9) Confirm you are redirected as expected
10) Go back to the staff client to edit your item. This time put two
URLs in the Uniform Resource Identifier field, separated by ' | '.
(Don't forget the spaces around the pipe, and don't forget to add a
 trailing slash to the URL).
11) Go back to the OPAC and refresh the page.
12) Confirm both URLs redirect as expected.

Sponsored-by: Parliamentary Library New Zealand
Comment 2 David Cook 2021-03-15 22:22:41 UTC
I feel like I've had an issue related to this but don't have the time to look at it right now...
Comment 3 Henry Bolshaw 2021-03-17 16:54:39 UTC
Hi Aleisha, I'm still getting some 404 errors when I add two URLs:

e.g. 

https://bbc.co.uk/ | https://gu.com/

https://bbc.co.uk | https://gu.com

https://bbc.co.uk | https://gu.com/

Sometimes one link will work but not the other one.

Can you provide some examples of links that you think should work?

Also for the test plan, do we need to test "trackclicks:track" and "trackclicks:track anonymously" separately or doesn't it make a difference?
Comment 4 Aleisha Amohia 2021-03-17 20:24:14 UTC
(In reply to Henry Bolshaw from comment #3)
> Hi Aleisha, I'm still getting some 404 errors when I add two URLs:
> 
> e.g. 
> 
> https://bbc.co.uk/ | https://gu.com/
> 
> https://bbc.co.uk | https://gu.com
> 
> https://bbc.co.uk | https://gu.com/
> 
> Sometimes one link will work but not the other one.
> 
> Can you provide some examples of links that you think should work?

Actually this is a good point, it looks like it is broken in master as well (without applying this patch). I'll see if I can fix it with this patch.

> 
> Also for the test plan, do we need to test "trackclicks:track" and
> "trackclicks:track anonymously" separately or doesn't it make a difference?

From my understanding, it doesn't make a difference.
Comment 5 Aleisha Amohia 2021-03-17 20:54:27 UTC
I've written a patch for Bug 27979 to address the broken multiple URLs bug. This patch can be tested on top of that one
Comment 6 David Nind 2021-04-01 23:27:16 UTC
Created attachment 119091 [details] [review]
Bug 27961: Ensure external URLs are filtered

TT variables are used to build the link in items.uri . This is a problem
particularly when TrackClicks is enabled because the items.uri value
doesn't get escaped, resulting in a 404.

To test:
1) Enable the TrackClicks system preference (set to Track or Track
    anonymously)
2) Find a record with an item
3) Edit the item and put an external URL under the Uniform Resource
Identifier field
4) Go to that record in the OPAC
5) Click on Link to resource
6) Confirm you are not redirected to the URL and instead see a 404
7) Apply the patch and refresh the page
8) Click on Link to resource
9) Confirm you are redirected as expected
10) Go back to the staff client to edit your item. This time put two
URLs in the Uniform Resource Identifier field, separated by ' | '.
(Don't forget the spaces around the pipe, and don't forget to add a
 trailing slash to the URL).
11) Go back to the OPAC and refresh the page.
12) Confirm both URLs redirect as expected.

Sponsored-by: Parliamentary Library New Zealand

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Jonathan Druart 2021-04-14 10:05:08 UTC
Caused by 
  commit 26d4b7fd5e35f784f951c4f9e9bb084a9f4612a4
  Bug 24699: Split items.uri in OPAC detail view
?

Marcel, can you have a look at it please?
Comment 8 Marcel de Rooy 2021-04-14 11:02:33 UTC
(In reply to Jonathan Druart from comment #7)
> Caused by 
>   commit 26d4b7fd5e35f784f951c4f9e9bb084a9f4612a4
>   Bug 24699: Split items.uri in OPAC detail view
> ?
> 
> Marcel, can you have a look at it please?

Will do
Comment 9 Marcel de Rooy 2021-04-16 06:22:08 UTC
QAing
Comment 10 Marcel de Rooy 2021-04-16 07:31:19 UTC
Created attachment 119694 [details] [review]
Bug 27961: Ensure external URLs are filtered

TT variables are used to build the link in items.uri . This is a problem
particularly when TrackClicks is enabled because the items.uri value
doesn't get escaped, resulting in a 404.

To test:
1) Enable the TrackClicks system preference (set to Track or Track
    anonymously)
2) Find a record with an item
3) Edit the item and put an external URL under the Uniform Resource
Identifier field
4) Go to that record in the OPAC
5) Click on Link to resource
6) Confirm you are not redirected to the URL and instead see a 404
7) Apply the patch and refresh the page
8) Click on Link to resource
9) Confirm you are redirected as expected
10) Go back to the staff client to edit your item. This time put two
URLs in the Uniform Resource Identifier field, separated by ' | '.
(Don't forget the spaces around the pipe, and don't forget to add a
 trailing slash to the URL).
11) Go back to the OPAC and refresh the page.
12) Confirm both URLs redirect as expected.

Sponsored-by: Parliamentary Library New Zealand

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Jonathan Druart 2021-04-16 13:12:18 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 12 Fridolin Somers 2021-04-22 14:31:02 UTC
Pushed to 20.11.x for 20.11.05
Comment 13 Andrew Fuerste-Henry 2021-04-25 16:43:00 UTC
I'm not recreating the issue when following your test plan on 20.05. Additionally, the patch doesn't apply cleanly. Please rebase if needed.
Comment 14 Aleisha Amohia 2021-05-04 04:13:41 UTC
Created attachment 120426 [details] [review]
Bug 27961: [20.05.x] Ensure external URLs are filtered

TT variables are used to build the link in items.uri . This is a problem
particularly when TrackClicks is enabled because the items.uri value
doesn't get escaped, resulting in a 404.

To test:
1) Enable the TrackClicks system preference (set to Track or Track
    anonymously)
2) Find a record with an item
3) Edit the item and put an external URL under the Uniform Resource
Identifier field
4) Go to that record in the OPAC
5) Click on Link to resource
6) Confirm you are not redirected to the URL and instead see a 404
7) Apply the patch and refresh the page
8) Click on Link to resource
9) Confirm you are redirected as expected
10) Go back to the staff client to edit your item. This time put two
URLs in the Uniform Resource Identifier field, separated by ' | '.
(Don't forget the spaces around the pipe, and don't forget to add a
 trailing slash to the URL).
11) Go back to the OPAC and refresh the page.
12) Confirm both URLs redirect as expected.

Sponsored-by: Parliamentary Library New Zealand