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

fix content-dispostion header in dav #4498

Merged

Conversation

dragonchaser
Copy link

Bugfix: Fix Content-Disposition header in dav

We have added missing quotes to the Content-Disposition header in the dav service. This fixes an issue with files containing special characters in their names.

owncloud/ocis#8361

Signed-off-by: Christian Richter <crichter@owncloud.com>
@dragonchaser dragonchaser force-pushed the fix-content-disposition-header-in-dav branch from 78d3f34 to a432c00 Compare February 6, 2024 09:29
@dragonchaser dragonchaser force-pushed the fix-content-disposition-header-in-dav branch from 782fd59 to 4d1020b Compare February 6, 2024 10:55
Co-authored-by: Victor Scharf <vscharf@owncloud.com>

Signed-off-by: Christian Richter <crichter@owncloud.com>
@dragonchaser dragonchaser force-pushed the fix-content-disposition-header-in-dav branch from 4d1020b to a0cd76a Compare February 6, 2024 10:57
Comment on lines 290 to 293
### [Fix Content-Dispositon header for download requests](
- [coreApiVersions/fileVersions.feature:158](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiVersions/fileVersions.feature#L158)
- [coreApiVersions/fileVersions.feature:176](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiVersions/fileVersions.feature#L176)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

this should fix the expected-failure file

Suggested change
### [Fix Content-Dispositon header for download requests](
- [coreApiVersions/fileVersions.feature:158](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiVersions/fileVersions.feature#L158)
- [coreApiVersions/fileVersions.feature:176](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiVersions/fileVersions.feature#L176)
)
### [Fix Content-Dispositon header for download requests](https://github.com/cs3org/reva/pull/4498)
- [coreApiVersions/fileVersions.feature:158](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiVersions/fileVersions.feature#L158)
- [coreApiVersions/fileVersions.feature:176](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/coreApiVersions/fileVersions.feature#L176)

@phil-davis
Copy link
Contributor

phil-davis commented Feb 6, 2024

https://drone.owncloud.com/cs3org/reva/3995/15/6

  Scenario Outline: downloading a file should serve security headers     # /drone/src/tmp/testrunner/tests/acceptance/features/coreApiWebdavOperations/downloadFile.feature:266
    Given using <dav-path-version> DAV path                              # FeatureContext::usingOldOrNewDavPath()
    When user "Alice" downloads file "/welcome.txt" using the WebDAV API # FeatureContext::userDownloadsFileUsingTheAPI()
    Then the HTTP status code should be "200"                            # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And the following headers should be set                              # FeatureContext::theFollowingHeadersShouldBeSet()
      | header                            | value                                                            |
      | Content-Disposition               | attachment; filename*=UTF-8''welcome.txt; filename="welcome.txt" |
      | Content-Security-Policy           | default-src 'none';                                              |
      | X-Content-Type-Options            | nosniff                                                          |
      | X-Download-Options                | noopen                                                           |
      | X-Frame-Options                   | SAMEORIGIN                                                       |
      | X-Permitted-Cross-Domain-Policies | none                                                             |
      | X-Robots-Tag                      | none                                                             |
      | X-XSS-Protection                  | 1; mode=block                                                    |
    And the downloaded content should start with "Welcome"               # FeatureContext::downloadedContentShouldStartWith()

    Examples:
      | dav-path-version |
      | old              |
        Failed step: And the following headers should be set
        WebDav::theFollowingHeadersShouldBeSet Expected value for header 'Content-Disposition' was 'attachment; filename*=UTF-8''welcome.txt; filename="welcome.txt"', but got 'attachment; filename*=UTF-8''"welcome.txt"; filename="welcome.txt"' instead.
        Failed asserting that two strings are equal.
        --- Expected
        +++ Actual
        @@ @@
        -'attachment; filename*=UTF-8''welcome.txt; filename="welcome.txt"'
        +'attachment; filename*=UTF-8''"welcome.txt"; filename="welcome.txt"'
      | new              |
        Failed step: And the following headers should be set
        WebDav::theFollowingHeadersShouldBeSet Expected value for header 'Content-Disposition' was 'attachment; filename*=UTF-8''welcome.txt; filename="welcome.txt"', but got 'attachment; filename*=UTF-8''"welcome.txt"; filename="welcome.txt"' instead.
        Failed asserting that two strings are equal.
        --- Expected
        +++ Actual
        @@ @@
        -'attachment; filename*=UTF-8''welcome.txt; filename="welcome.txt"'
        +'attachment; filename*=UTF-8''"welcome.txt"; filename="welcome.txt"'
      | spaces           |
        Failed step: And the following headers should be set
        WebDav::theFollowingHeadersShouldBeSet Expected value for header 'Content-Disposition' was 'attachment; filename*=UTF-8''welcome.txt; filename="welcome.txt"', but got 'attachment; filename*=UTF-8''"welcome.txt"; filename="welcome.txt"' instead.
        Failed asserting that two strings are equal.
        --- Expected
        +++ Actual
        @@ @@
        -'attachment; filename*=UTF-8''welcome.txt; filename="welcome.txt"'
        +'attachment; filename*=UTF-8''"welcome.txt"; filename="welcome.txt"'
runsh: Total unexpected failed scenarios throughout the test run:
coreApiWebdavOperations/downloadFile.feature:283
coreApiWebdavOperations/downloadFile.feature:284
coreApiWebdavOperations/downloadFile.feature:289

Those also need to be added to the expected-failures files. (The test expectations are no longer quite correct. The test expectations will need to be adjusted in ocis when this is brought into ocis, then the updated tests can be used back here in reva - the pull request to do that will be able to delete the entries from expected failures)

@phil-davis
Copy link
Contributor

@dragonchaser I can push the needed expected-failures changes to here, if you like.

@phil-davis
Copy link
Contributor

expected-failures has been adjusted. Let's see what CI thinks.

@phil-davis
Copy link
Contributor

Passing - I will merge.
@dragonchaser you or someone can bump reva in ocis.

@phil-davis phil-davis merged commit bf89f7a into cs3org:edge Feb 6, 2024
9 checks passed
@dragonchaser
Copy link
Author

Out to lunch and suddenly my failing tests/expectations are being fixed, thx @saw-jan && @phil-davis

@dragonchaser dragonchaser deleted the fix-content-disposition-header-in-dav branch February 6, 2024 12:26
@micbar micbar mentioned this pull request Feb 26, 2024
71 tasks
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.

None yet

4 participants