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

Consider status in propstat when doing stat #258

Merged
merged 2 commits into from
May 9, 2021

Conversation

jstastny
Copy link
Contributor

@jstastny jstastny commented May 5, 2021

Why

When PROPFIND is sent to Nginx webdav server on a non-existent file, it responds with 207 Multi-Status, but contains the 404 in the status element of propstat. This is an example response from Nginx:

<?xml version="1.0" encoding="utf-8" ?>
<D:multistatus xmlns:D="DAV:">
    <D:response>
        <D:href>/p-nice/does-not-exist</D:href>
        <D:propstat>
            <D:prop></D:prop>
            <D:status>HTTP/1.1 404 Not Found</D:status>
        </D:propstat>
    </D:response>
</D:multistatus>

The RFC 4918, Section 9.1.2. names the 404 status as valid value for the status element in propstat, but it does not have an example with this combination (207 response status and 404 in the status element).

Before

Before this PR, the client would treat the PROPFIND responses for non-existent resources from NGinx as if the resources existed. Not considering the status element of propstat in the response body.

After

Throw error when doing stat on non-existent resource even if the HTTP status of the response is 207, but the status in propstat element is >= 400.

Notes

We have been using this code at Deepnote as a package patch for couple months already without any issues, but this was still using the non-TypeScript version of this library. I have waited with creation of the PR before the TypeScript version was out (thank you very much for the TS version, very appreciated).

The same approach could in theory be applied on all Multi-Status responses to PROPFIND -- I can namely think of the getDirectoryContents function.

@perry-mitchell
Copy link
Owner

Thanks @jstastny - This is fantastic!

@perry-mitchell perry-mitchell merged commit 04632e7 into perry-mitchell:master May 9, 2021
@jstastny jstastny deleted the nginx-non-existent branch May 10, 2021 10:18
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.

2 participants