Does not play nice with nginx WebDAV

[bradmcl]

Operating system

• macOS
• Linux

Application

• Desktop

Two issues so far:

The first is that the MKCOL requests for the .sync and .resourcedo not have a trailing / ( meaning .sync/ and the nginx WebDAV module is very picky. A work around for this is to add a rewrite like if ($request_method = MKCOL) { rewrite ^(.*[^/])$ $1/ break; } to the nginx config, but it would be nice if Joplin was more conservative in its adherence to the RFC.

The second issue is that Joplin does not like to parse PROPFIND responses of the form

Bradleys-MacBook-Pro:documentation brad$ curl -i --basic --user '' -X PROPFIND https:///joplin/.sync --upload-file - -H "Depth: 1" <<end

end
HTTP/1.1 100 Continue

HTTP/1.1 207 Multi-Status
Server: nginx/1.12.1 (Ubuntu)
Date: Mon, 14 May 2018 12:45:28 GMT
Transfer-Encoding: chunked
Connection: keep-alive

<D:multistatus xmlns:D="DAV:">
<D:response>
<D:href>/joplin/.sync</D:href>
<D:propstat>
<D:prop>
<D:creationdate>2018-05-12T21:49:03Z</D:creationdate>
<D:displayname>.sync</D:displayname>
<D:getcontentlanguage/>
<D:getcontentlength>4096</D:getcontentlength>
<D:getcontenttype/>
<D:getetag/>
<D:getlastmodified>Sat, 12 May 2018 21:48:52 GMT</D:getlastmodified>
<D:lockdiscovery/>
<D:resourcetype><D:collection/></D:resourcetype>
<D:source/>
<D:supportedlock/>
</D:prop>
<D:status>HTTP/1.1 200 OK</D:status>
</D:propstat>
</D:response>
</D:multistatus>

resulting in the error

2018-05-14 09:28:43: "Operations completed: "
2018-05-14 09:28:43: "Total folders: 1"
2018-05-14 09:28:43: "Total notes: 1"
2018-05-14 09:28:43: "Total resources: 0"
2018-05-14 09:28:43: "There was some errors:"
2018-05-14 09:28:43: "TypeError: Cannot read property '0' of null
TypeError: Cannot read property '0' of null
at WebDavApi.resourcePropByName (/Applications/Joplin.app/Contents/Resources/app/lib/WebDavApi.js:166:19)
at FileApiDriverWebDav.statFromResource_ (/Applications/Joplin.app/Contents/Resources/app/lib/file-api-driver-webdav.js:58:41)
at FileApiDriverWebDav.stat (/Applications/Joplin.app/Contents/Resources/app/lib/file-api-driver-webdav.js:33:16)
at
at process._tickCallback (internal/process/next_tick.js:109:7)"

[bradmcl]

I dug a little deeper into this, and Nginx returns the 404 status as a text field inside a multistatus response with a 207. Here's a full set of patches that gets Joplin to work with an unmodified nginx.

diff --git a/ReactNativeClient/lib/WebDavApi.js b/ReactNativeClient/lib/WebDavApi.js
index ce6dbb3d..a6f92c05 100644
--- a/ReactNativeClient/lib/WebDavApi.js
+++ b/ReactNativeClient/lib/WebDavApi.js
@@ -145,6 +145,7 @@ class WebDavApi {
                const propStats = resource['d:propstat'];
                let output = null;
                if (!Array.isArray(propStats)) throw new Error('Missing d:propstat property');
+
                for (let i = 0; i < propStats.length; i++) {
                        const props = propStats[i]['d:prop'];
                        if (!Array.isArray(props) || !props.length) continue;
@@ -324,8 +325,10 @@ class WebDavApi {
 
                // Check that we didn't get for example an HTML page (as an error) instead of the JSON response
                // null responses are possible, for example for DELETE calls
-               if (output !== null && typeof output === 'object' && !('d:multistatus' in output)) throw newError('Not a valid WebDAV response');
-
+               if (output !== null && typeof output === 'object' ) {
+                       if ( !('d:multistatus' in output)) throw newError('Not a valid WebDAV response');
+                       if ( this.stringFromJson(output, ['d:multistatus', 'd:response', 0, 'd:propstat',0,'d:status',0]).indexOf('404 Not Found') != -1 ) throw newError('Multistatus 404 Response',404);
+               }
                return output;
        }
 
diff --git a/ReactNativeClient/lib/file-api-driver-webdav.js b/ReactNativeClient/lib/file-api-driver-webdav.js
index cec088d5..f88df903 100644
--- a/ReactNativeClient/lib/file-api-driver-webdav.js
+++ b/ReactNativeClient/lib/file-api-driver-webdav.js
@@ -299,6 +299,7 @@ class FileApiDriverWebDav {
 
        async mkdir(path) {
                try {
+                       if ( !path.endsWith('/') ) path = path + '/'; // RFC wants this, and so does NGINX
                        await this.api().exec('MKCOL', path);
                } catch (error) {
                        if (error.code === 405) return; // 405 means that the collection already exists (Method Not Allowed)```

[bradmcl]

See #541 which isn't pretty :( On the plus side, it works.

[laurent22]

Hi, thanks for the patch, but I wonder if there's a better way to fix it? In particular I don't get why nginx responds with a multistatus when only a single request has been made. And if they can randomly respond with multistatus then the code should be made more generic to deal with this.

Do you have any test server that I could use by any chance to double-check all this?

[bradmcl]

Thought you might feel that way. Relevant code is https://github.com/arut/nginx-dav-ext-module/blob/master/ngx_http_dav_ext_module.c starting near line 524. It doesn't distinguish between single and multi requests. That seems consistent with https://tools.ietf.org/html/rfc4918#page-35 section 9.1 ( the examples there do the same thing ), where it appears that the request level status code only addresses whether or not the request was legal, and then the individual D:propstat sections each contain the D:status for that item.

Going back and looking at the Joplin code with that in mind, it appears the generic solution is to properly parse the status fields, examples: <D:status>HTTP/1.1 200 OK</D:status> or <D:status>HTTP/1.1 403 Forbidden</D:status> or <D:status>HTTP/1.1 404 Not Found</D:status> in order to retrieve the HTTP status for each file in all D:propstat sections of the response.

Because exec() cannot know if it has sent a request resulting in multiple results or not, parsing the 404 at that level is rather hackish - it was useful for proving this could work,

Would you prefer a solution in file-api-drive-webdav.js where statFromResource_ parses the 404s in the <D:status> responses?

I'm actually a little more concerned about the other section of the patch, adding the trailing '/' on the MKCOL. While it appears that it's closer to the RFC, and how NGINX works, I haven't (and couldn't) try testing it against all the other DAV servers that Joplin works with.

[laurent22]

@bradmcl, your fix should be part of the latest releases (Desktop and Android) so I guess we can close this issue?

[bradmcl]

Confirmed; release 1.0.100 synchronizes correctly with Nginx DAV running on Ubuntu from both an OSX desktop, and an Android phone. Thanks!