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

Handle Nginx DAV PROPFIND responses correctly #554

Merged
merged 3 commits into from Jun 10, 2018

Conversation

Projects
None yet
2 participants
@bradmcl
Contributor

bradmcl commented May 23, 2018

Parse the d:status field for d:propstat sections within a d:multistatus response to a DAV PROPFIND because Nginx returns 404 statuses wrapped in an overall 207

Proposed fix for #523

Handle Nginx DAV PROPFIND responses correctly
Parse the d:status field for d:propstat sections within a d:multistatus response to a DAV PROPFIND because Nginx returns 404 statuses wrapped in an overall 207
@laurent22

This comment has been minimized.

Owner

laurent22 commented May 26, 2018

The WebDAV XML is often quite messy and changes from one server to the next, so I think this line might cause problem if something is not defined or defined differently:

propStat[0]['d:status'][0].split(' ')[1]

Is there any way to get it working with stringFromJson() or similar (as these functions have many checks to support various formats)?

@bradmcl

This comment has been minimized.

Contributor

bradmcl commented May 27, 2018

Sure, here's a rewrite taking it that direction.

@laurent22

This comment has been minimized.

Owner

laurent22 commented Jun 9, 2018

Could you confirm it indeed works with your Nginx server (as I can't test myself)?

Also this line httpStatusLine.split(' ')[1] === '404' could be an issue if the http status text is non-standard and doesn't have spaces or if the 404 is not in the second position. So to be safe I think it'd be better to just check the presence of the string "404" anywhere in the status code.

@bradmcl

This comment has been minimized.

Contributor

bradmcl commented Jun 10, 2018

I can definitely confirm that it works with Nginx for me; I've been using a local build and ignoring updates since I posted this PR, and I've had no issues.

On the split concern, I'd done a quick check ( I am not a Javascript native, this is a bit of a drive-by, and I'll be back to Python, C, and assembly as soon as I solve this itch), which caused me to think this was likely okay:

Bradleys-MacBook-Pro:joplin brad$ node

x = "blah 101";
'blah 101'
y = "";
''
z = 2;
2
x.split(' ')[1];
'101'
y.split(' ')[1];
undefined
z.split(' ')[1];
TypeError: z.split is not a function
x.split(' ')[1] === '404';
false
x.split(' ')[1] === '101';
true
y.split(' ')[1] === '101';
false
z.split(' ')[1] === '101';
TypeError: z.split is not a function

If that's not good, I'm happy with any other interpretation that works. I don't think the 404 can be anywhere else since that's a standard regulated string.

@laurent22

This comment has been minimized.

Owner

laurent22 commented Jun 10, 2018

I thought accessing an array out of bonds would throw an error, but it seems it indeed doesn't.

Just to be completely safe though I would prefer something like httpStatusLine.indexOf('404') >= 0. Since we are sure httpStatusLine is a string, we know it will have the indexOf function, and then we don't need to worry where the 404 is going to be in that string.

I agree there's a standard but many implementations do their own thing so it's better to be safe, since that class is used to access many different services.

@bradmcl

This comment has been minimized.

Contributor

bradmcl commented Jun 10, 2018

Continues to work fine with that change.

@laurent22

This comment has been minimized.

Owner

laurent22 commented Jun 10, 2018

That's great, thank for the update.

@laurent22 laurent22 merged commit e3314c8 into laurent22:master Jun 10, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

laurent22 added a commit that referenced this pull request Jun 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment