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

sync-collection (v4.1.1) is returning a propstat with 418 (teapot) in it #1436

Open
castaway opened this issue Jan 16, 2023 · 7 comments
Open

Comments

@castaway
Copy link

I think because this line:

if ($empty) {
doesn't check if $status was set.

It looks like REPORT for sync-collection should not return a PROPSTAT if the item was deleted (according to https://www.rfc-editor.org/rfc/rfc6578#page-7 :

For members that have been removed, the DAV:response MUST contain one DAV:status with a value set to '404 Not Found' and MUST NOT contain any DAV:propstat element.

I found #829 and the associated PRs, one of which did add a check: if ($empty && !$status), but the fix seems to have stalled somewhere. Any fixes for that still lurking?

@evert
Copy link
Member

evert commented Apr 19, 2023

In the linked code you can read in the comment why this was done. It's an inconsistency between the WebDAV and the sync collection spec.

The workaround (a dummy status code) seemed to most obvious way to go. I picked 418 to indicate 'something is weird here' but anyone investigating would be able to find the comment in the source

@castaway
Copy link
Author

I saw the comment, however we're actually hitting that section somehow. The client parsing the response is not a big fan of getting a 418 there. Above I pointed out how we could make it not get one, any comment on that please?

@evert
Copy link
Member

evert commented Apr 20, 2023

Not sure if you understood, but this was done by design. What client is having an issue with this?

@castaway
Copy link
Author

I understood you to mean "we had to put something there, but it should never actually happen in practice".

We're using Net::DAVTalk, which is a Perl module, using the XML interface. I can hunt out the exact bit that complains, tomorrow.

@castaway
Copy link
Author

castaway commented Apr 20, 2023

Ah found it:
https://metacpan.org/dist/Net-CardDAVTalk/source/lib/Net/CardDAVTalk.pm#L637

logs:

Request: GET rest/v1/contacts/sync/longidthing= {}
Error: $VAR1 = [
          'Odd status in propstat response /addressbooks/12/contacts/6B076548-AB48-11EA-9715-ADCC0DF720E2.vcf: HTTP/1.1 418 I\'m a teapot'
        ];

@evert
Copy link
Member

evert commented Apr 20, 2023

I don't remember which client had an issue with the absense of a propstat, but originally this was added in as a workaround. So removing it will likely also result in some issue somewhere else.

So probably a better fix for this might be to remove the lone propstat, but only if this was a sync-collection response.

@castaway
Copy link
Author

My though was that its only an issue when items have been removed (see original RFC quote) .. it looks like someone started a patch for it a while back, any idea if that patch is any good?

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

No branches or pull requests

2 participants