Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tests-Only]make escaping/encoding case insensitive #38374

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

swoichha
Copy link
Contributor

@swoichha swoichha commented Feb 3, 2021

Description

This PR makes tests of escaped file and folder names with special characters more flexible.

Related Issue

How Has This Been Tested?

  • CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@@ -331,7 +331,6 @@ Feature: checksums
Then the HTTP status code should be "400"
And the content of file "/textfile0.txt" for user "Brian" should be "ownCloud test text file 0" plus end-of-line

@issue-ocis-reva-214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not delete this line this changes line number of other tests

Comment on lines 682 to 683
$value = \strtolower($xmlPart[0]->__toString());
$pattern = \strtolower($this->featureContext->substituteInLineCodes($pattern, $user, ['preg_quote' => ['/']]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we only want to do this for some properties such as d:href, you can do this only for such properties otherwise do case sensitive comparisioin

@swoichha swoichha requested a review from dpakach February 5, 2021 05:55
Copy link
Contributor

@dpakach dpakach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I'm happy with that, because now we are silently changing the regex and what the codes checks is not what the feature files says.
What about changing the regular expression in the places where its an issue?

@swoichha
Copy link
Contributor Author

swoichha commented Feb 8, 2021

I'm not sure if I'm happy with that, because now we are silently changing the regex and what the codes checks is not what the feature files says.
What about changing the regular expression in the places where its an issue?

@individual-it It was a problem for

| old | /file ?2.txt | webdav\/file%20%3f2\.txt |

So, this only changes the regex if we are checking response for /d:href like //d:response/d:href in this case. But for all the other cases it does not change the regex into lowercase.

@individual-it
Copy link
Member

@swoichha what about changing the regex itself to something like webdav\/file%20%3[fF]2\.txt or adding the case-insensitive modifier i in line 34

And the value of the item "//d:response/d:href" in the response to user "Alice" should match "/remote\.php\/<expected_href>/"

/webdav\/file%20%3f2\.txt/i

@swoichha
Copy link
Contributor Author

swoichha commented Feb 8, 2021

@swoichha what about changing the regex itself to something like webdav\/file%20%3[fF]2\.txt or adding the case-insensitive modifier i in line 34

And the value of the item "//d:response/d:href" in the response to user "Alice" should match "/remote\.php\/<expected_href>/"

/webdav\/file%20%3f2\.txt/i

@individual-it Yes, we can do that and it will work but in future we must have to remember doing the same for encoding case-insensitive characters.

@individual-it
Copy link
Member

the quoted RFC is for all URIs https://tools.ietf.org/html/rfc3986#section-2.1 so we would have to make the substitution cleverer to match all percent encoding but then we have to be careful not to change if we want to check real % in any item-name

I think the original solution would also match wEBdav\/FiLe%20%3f2\.tXT

@individual-it
Copy link
Member

after verbal discussion with @swoichha

/i will also match too much because if the response comes back with the filename with changed cases the test would not catch the bug if we make the regex completely case insensitive
checking the decoded filename is problematic because that would not catch the original bug reported in owncloud/ocis#1296
meaning:

  1. we have to check the raw value
  2. we have to be explicit what the step does and it should not manipulate the regex or values underneath

so the only real solution is to refactor all regex to be in the form of file%20%3[fF]2

@sonarcloud
Copy link

sonarcloud bot commented Feb 8, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@individual-it individual-it merged commit cb3f99b into master Feb 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the href-url-encoding branch February 8, 2021 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make tests of escaped file and folder names more flexible.
3 participants