Test filters and searching with strings that need URI encoding [#3281209]

Problem/Motivation

There are potential bugs when searching for projects with characters like &

Steps to reproduce

TBD

Proposed resolution

Once #3280721: Create test fixture is in, we should be able to write tests for it.
See ProjectBrowser.svelte for the query string construction

Remaining tasks

✅ File an issue about this project
☐ Addition/Change/Update/Fix to this project
☐ Testing to ensure no regression
☐ Automated unit/functional testing coverage
☐ Developer Documentation support on feature change/addition
☐ User Guide Documentation support on feature change/addition
☐ Code review from 1 Drupal core team member
☐ Full testing and approval
☐ Credit contributors
☐ Review with the product owner
☐ Release

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork project_browser-3281209

Show commands

Start within a Git clone of the project using the version control instructions.

Add & fetch this issue fork’s repository

Or, if you do not have SSH keys set up on git.drupalcode.org:

Add & fetch this issue fork’s repository

3281209-filtering-and-searching changes, plain diff MR !114
Check out this branch for the first time

Check out existing branch, if you already have it locally
3281209-test-filters-and changes, plain diff MR !97
Check out this branch for the first time

Check out existing branch, if you already have it locally

About issue forks

Comments

Comment #1

18 May 2022 at 18:10

tim.plunkett created an issue. See original summary.

Comment #2

hooroomoo

they/them

nyc🗽

commented 19 May 2022 at 17:56

Assigned:

Unassigned

» hooroomoo

Comment #3

19 May 2022 at 18:00

hooroomoo opened merge request !97

Comment #4

hooroomoo

they/them

nyc🗽

commented 20 May 2022 at 21:45

Planning to wait till #3281686: Fix incorrect taxonomy term ID in test fixture is in (since it looks like it will be very soon) to rewrite test using the newly introduced helper method from the issue

Comment #5

fjgarlin commented 23 May 2022 at 15:58

Status:

Active

» Needs work

Comment #6

hooroomoo

they/them

nyc🗽

commented 23 May 2022 at 17:34

Assigned:	hooroomoo	» Unassigned
Status:	Needs work	» Needs review

Comment #7

fjgarlin commented 23 May 2022 at 18:10

Status:

Needs review

» Reviewed & tested by the community

Changes are simple and tests are provided. Marking as RTBC.

Comment #8

tim.plunkett

he/him

English

Philadelphia

commented 23 May 2022 at 22:35

Assigned:

Unassigned

» tim.plunkett

I'm reviewing this. I have a few questions that I'll try to post soon

Comment #9

tim.plunkett

he/him

English

Philadelphia

commented 24 May 2022 at 14:12

Status:

Reviewed & tested by the community

» Needs work

Why are we doing any XSS filtering on the query string? My understanding is that while it's important to sanitize anything being rendered by us on the front-end, doing string matching should be a 1:1 operation on the input.
For example, consider \Drupal\block\Controller\CategoryAutocompleteController::autocomplete() which does the comparison on the raw string, and only escapes it for storing the matches that will be rendered later.

Unless I'm missing something, I think we should remove those Xss::filter calls, and only escape things if they are being rendered by us, but leave the values intact for the source plugins to do their comparisons.

Comment #10

fjgarlin commented 24 May 2022 at 14:18

If the data is not manipulated and comes from the front-end it will always be ok. However, the endpoints are open and anybody could query them openly within the project and change the URL.

ie: Request URL: https://SITE-URL/drupal-org-proxy/project?page=0&limit=12&sort=usage_tot...

If we are ok passing the request "as is" to the plugins, then yeah, I guess we can remove all the Xss::filter calls from "ProjectBrowserEndpointController". I was probably being over-cautious.

Comment #11

tim.plunkett

he/him

English

Philadelphia

commented 24 May 2022 at 22:28

Assigned:

tim.plunkett

» hooroomoo

I think we want to do straight comparisons against the user input, no matter how malformed or even malicious it is. It's up to the plugins to handle that (for example, by using the database API, the MockDrupalDotOrg plugin is protected against SQL injection).

Reassigning to @hooroomoo to remove all of the uses of Xss::filter from this method (other places in the module shouldn't need adjusting)

Comment #12

hooroomoo

they/them

nyc🗽

commented 25 May 2022 at 14:44

Assigned:	hooroomoo	» Unassigned
Status:	Needs work	» Needs review

Comment #13

fjgarlin commented 25 May 2022 at 15:31

Status:

Needs review

» Reviewed & tested by the community

It looks good to me. All Xss::filter removed from the controller as mentioned in the review and tests provided.

Comment #14

27 May 2022 at 02:22

tim.plunkett committed e8d64d9 on 1.0.x authored by hooroomoo

Issue #3281209 by hooroomoo: Test filters and searching with strings...

Comment #15

tim.plunkett

he/him

English

Philadelphia

commented 27 May 2022 at 02:22

Status:

Reviewed & tested by the community

» Fixed

Merged, thanks!

Comment #16

tim.plunkett

he/him

English

Philadelphia

commented 27 May 2022 at 03:34

Status:

Fixed

» Needs work

Hmm, this caused failures on PHP 8.1
Sorting changed from PHP 7 to 8, but I'm not sure which part of it caused this problem.

Reverted for now, sorry

Comment #17

27 May 2022 at 03:34

tim.plunkett committed 8c19fe4 on 1.0.x

Revert "Issue #3281209 by hooroomoo: Test filters and searching with...

Comment #18

fjgarlin commented 27 May 2022 at 07:58

Assigned:

Unassigned

» fjgarlin

Comment #19

27 May 2022 at 08:15

fjgarlin opened merge request !114

Comment #20

fjgarlin commented 27 May 2022 at 08:33

I created a new branch and recreated the changes from the previous MR: https://git.drupalcode.org/project/project_browser/-/merge_requests/97/d...

The new MR is: https://git.drupalcode.org/project/project_browser/-/merge_requests/114/...

The only change compared to the previous is to add the `$ignore_order` parameter to the function that checks that arrays are the same as the order is only relevant when we're testing order of elements. I'm not sure this covers 100% the PHP8.1 case.

Comment #21

fjgarlin commented 27 May 2022 at 09:14

Running the test in PHP8.1 all is green, but it's not in PHP7.3 (see above). The highlighted difference is:

-    0 => '$?Vitamin&C;'
-    1 => '&Un:/written'
+    0 => '&Un:/written'
+    1 => '$?Vitamin&C;'

I'm not sure if it's the PHP version or the MySQL version tho.

Comment #22

fjgarlin commented 27 May 2022 at 10:40

As the order is not relevant for this test (and it is related to mysql version, which in turn is really related to how the plugin sorts things) I just changed the placement where the special characters appear.

The rest of the changes should be pretty much as in the original MR.
I'll trigger tests with PHP7.3 and PHP8.1.

Comment #23

fjgarlin commented 27 May 2022 at 10:52

Status:

Needs work

» Needs review

Comment #24

hooroomoo

they/them

nyc🗽

commented 27 May 2022 at 14:32

Status:

Needs review

» Reviewed & tested by the community

Change looks good to me

Comment #25

fjgarlin commented 27 May 2022 at 16:50

Assigned:

fjgarlin

» Unassigned

Comment #26

27 May 2022 at 17:01

tim.plunkett committed c499d78 on 1.0.x authored by fjgarlin

Issue #3281209 by hooroomoo, fjgarlin, tim.plunkett: Test filters and...

Comment #27

tim.plunkett

he/him

English

Philadelphia

commented 27 May 2022 at 17:01

Status:

Reviewed & tested by the community

» Fixed

I use sqlite locally, and that was the difference. Queued up sqlite with both MRs and it failed the old one and passed the new one!
Thanks for the fixes.

Comment #28

10 June 2022 at 17:04

Status:

Fixed

» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.