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

check name for illegal values during uploads and moves #1900

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented Jul 19, 2021

The code was not checking for invalid file names during uploads and moves.

@update-docs
Copy link

update-docs bot commented Jul 19, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@C0rby C0rby self-assigned this Jul 19, 2021
@C0rby C0rby force-pushed the tus-illegal-names branch 2 times, most recently from 90e8c08 to 80e20e5 Compare July 19, 2021 15:32
@C0rby C0rby requested review from butonic, ishank011, labkode and refs July 19, 2021 15:49
@C0rby C0rby marked this pull request as ready for review July 19, 2021 15:49
@C0rby C0rby force-pushed the tus-illegal-names branch from 80e20e5 to bc85663 Compare July 20, 2021 14:18
@C0rby C0rby marked this pull request as draft July 20, 2021 14:18
@C0rby
Copy link
Contributor Author

C0rby commented Jul 20, 2021

@phil-davis, are those supposed to be issue demonstrations?

apiOcisSpecific/apiWebdavMove1-moveFolder.feature:20
apiOcisSpecific/apiWebdavMove1-moveFolder.feature:31
apiOcisSpecific/apiWebdavMove1-moveFolder.feature:42
apiOcisSpecific/apiWebdavMove1-moveFolder.feature:53
apiOcisSpecific/apiWebdavMove1-moveFolder.feature:64
apiOcisSpecific/apiWebdavMove2-moveFile.feature:22

What should I do with those tests now? Delete them?

@C0rby C0rby changed the title check name for illegal values during tus uploads check name for illegal values during uploads Jul 21, 2021
@C0rby C0rby changed the title check name for illegal values during uploads check name for illegal values during uploads and moves Jul 21, 2021
@phil-davis
Copy link
Contributor

phil-davis commented Jul 21, 2021

What should I do with those tests now? Delete them?

https://drone.cernbox.cern.ch/cs3org/reva/2304/6/6
HTTP status code 400 is not one of the expected values 201 or 500

I guess those tests were returning 500 and now return 400 - an improvement. But it looks like they should be able to really work and return 201.

IMO adjust the expectation to Then the HTTP status code should be "400" to "record" the current behavior.

Then someone needs to review all those "local" API tests to see what use they are. We have expected-failures files these days, that keep track of what core tests are known to be failing. I raised issue owncloud/ocis#2313 for reviewing that.

@C0rby
Copy link
Contributor Author

C0rby commented Jul 21, 2021

HTTP status code 400 is not one of the expected values 201 or 500

I guess those tests were returning 500 and now return 400 - an improvement. But it looks like they should be able to really work and return 201.

Not quite right. They were returning 201 (ocis storage) or 500 (eos storage) but should have been returning 400 because the scenarios are testing illegal behavior like renaming files/folders to contain invalid characters.

So I think those tests can be removed now, right?

This is the linked issue: owncloud/ocis#1295

@phil-davis
Copy link
Contributor

So I think those tests can be removed now, right?

True - there are core scenarios in core apiWebdavMove1/moveFolder.feature like:
Scenario Outline: Renaming a folder to a backslash should return an error
Those get run in the usual core API test pipelines.

Remove these "local" API tests.

@C0rby C0rby force-pushed the tus-illegal-names branch from bc85663 to 242a903 Compare July 21, 2021 09:26
@C0rby C0rby marked this pull request as ready for review July 21, 2021 09:45
@C0rby C0rby force-pushed the tus-illegal-names branch from 242a903 to a313666 Compare July 21, 2021 10:00
phil-davis
phil-davis previously approved these changes Jul 21, 2021
@ishank011
Copy link
Contributor

@C0rby should we also add these checks for copy?

@C0rby
Copy link
Contributor Author

C0rby commented Jul 22, 2021

That's a good point. Let me check if copy is also affected.

@C0rby C0rby force-pushed the tus-illegal-names branch from b76cbcd to 047fb2c Compare July 22, 2021 09:25
@C0rby
Copy link
Contributor Author

C0rby commented Jul 22, 2021

Yes, I added the check to COPY and MKCOL.

@ishank011 ishank011 merged commit 4475f43 into cs3org:master Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants