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: return 204 instead of 404 #49839

Merged

Conversation

SebastianKrupinski
Copy link
Contributor

@SebastianKrupinski SebastianKrupinski commented Dec 13, 2024

Summary

Changed status return to 204 "no content" instead of 404 when a photo does not exist

204 make more sense as it signals that there was no image, instead that a error occurred.

Checklist

@SebastianKrupinski
Copy link
Contributor Author

/backport to stable30

@SebastianKrupinski
Copy link
Contributor Author

/backport to stable29

@ChristophWurst

This comment was marked as resolved.

@arnowelzel
Copy link
Contributor

arnowelzel commented Dec 13, 2024

Eventhough this fix solves the issue of having a lot of 404-responses - the root cause is still in place: instead of checking, if there is an image at all and not requesting it, the contacts app still tries to load an image for each contact. There will still be a lot of exceptions which trigger the 204-response. It would be better, if a contact had a flag, that there is no image, so the frontend won't even try to load it.

@nextcloud nextcloud deleted a comment from SebastianKrupinski Dec 13, 2024
@kesselb
Copy link
Contributor

kesselb commented Dec 13, 2024

We rejected similar "don't use 404 for fail2ban" requests in the past.
For example: #26793.


I think using 204 is a good choice.
The request was successful, there's just no content to return.


As written by Arno, changing the response code is a workaround for those fail2ban users. The underlying problem is that those requests are happening. The response for the address book contains a has-photo attribute (empty/1).

<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:card="urn:ietf:params:xml:ns:carddav">
	<d:response>
		<d:href>
			/remote.php/dav/addressbooks/users/admin/contacts/F1D36433-6CC3-48B9-942F-8E7E12A69D4E.vcf
		</d:href>
		<d:propstat>
			<d:prop>
				<d:getetag>&quot;4e748fcff71af1c2a024fb19b9a3812e&quot;</d:getetag>
				<d:getcontenttype>text/vcard; charset=utf-8</d:getcontenttype>
				<d:resourcetype/>
				<card:address-data>BEGIN:VCARD&#13;
					VERSION:3.0&#13;
					PRODID:-//Sabre//Sabre VObject 4.5.6//EN&#13;
					UID:58236d02-66e5-4374-bdaa-5d4f04ce29f4&#13;
					FN:Test 1000&#13;
					EMAIL;TYPE=HOME:&#13;
					END:VCARD&#13;
				</card:address-data>
				<x1:has-photo xmlns:x1="http://nextcloud.com/ns"></x1:has-photo>
			</d:prop>
			<d:status>HTTP/1.1 200 OK</d:status>
		</d:propstat>
	</d:response>
</d:multistatus>

The following change for contacts should take the hasPhotos property into account:

Index: src/models/contact.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/models/contact.js b/src/models/contact.js
--- a/src/models/contact.js	(revision b91012f60c1b011a3ce173c8613b05a221646ae4)
+++ b/src/models/contact.js	(date 1734087428669)
@@ -119,6 +119,16 @@
 		return ''
 	}
 
+	/**
+	 * Return whether a photo is available
+	 *
+	 * @readonly
+	 * @memberof Contact
+	 */
+	get hasPhoto() {
+		return this.dav && this.dav.hasphoto
+	}
+
 	/**
 	 * Return the version
 	 *
Index: src/components/ContactsList/ContactsListItem.vue
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/components/ContactsList/ContactsListItem.vue b/src/components/ContactsList/ContactsListItem.vue
--- a/src/components/ContactsList/ContactsListItem.vue	(revision b91012f60c1b011a3ce173c8613b05a221646ae4)
+++ b/src/components/ContactsList/ContactsListItem.vue	(date 1734087333973)
@@ -141,7 +141,7 @@
 					return
 				}
 				this.avatarUrl = photoUrl
-			} else if (this.source.url) {
+			} else if (this.source.hasPhoto && this.source.url) {
 				this.avatarUrl = `${this.source.url}?photo`
 			}
 		},

@SebastianKrupinski
Copy link
Contributor Author

The following change for contacts should take the hasPhotos property into account:

Sure, we can add this also, makes sense.

Copy link
Contributor

@oleksandr-nc oleksandr-nc left a comment

Choose a reason for hiding this comment

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

really good changes

@SebastianKrupinski
Copy link
Contributor Author

/backport to stable28

See Wiki: Maintenance and Release Schedule. 28 has reached EOL.

No worries, ill cancel the 28 backport.

FYI, a EOL date not just the month would be a good idea.

image

This can be interpreted as Dec 31st instead of Dec 1st

Signed-off-by: SebastianKrupinski <[email protected]>
@SebastianKrupinski SebastianKrupinski force-pushed the fix/issue-3021-return-no-content-instead-of-error branch from 0d8d3b1 to 0628eb6 Compare December 13, 2024 16:47
@SebastianKrupinski SebastianKrupinski merged commit afc4b0a into master Dec 13, 2024
185 of 188 checks passed
@SebastianKrupinski SebastianKrupinski deleted the fix/issue-3021-return-no-content-instead-of-error branch December 13, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lot of 404 errors / CrowdSec ban me
6 participants