Pi-hole 6 API: deleting session returns 410 instead of 200

[akordowski]

Pi-hole 6 API: deleting session returns 410 instead of 200

[DL6ER]

To my understaning the HTTP specification recommends that the 410 status code is returned at the access of a already deleted entity, not at preforming the delete action itself.

It seems our interpretations indeed differ, let's quote the essential of the link you posted:

The HyperText Transfer Protocol (HTTP) 410 Gone client error response code indicates that access to the target resource is no longer available at the origin server and that this condition is likely to be permanent.

To me a successful deletion event exactly fulfills this: The resource is already gone when this returns from the API.

---

Let's have a look at RFC 9110 - HTTP Semantics:

If a DELETE method is successfully applied, the origin server SHOULD send

• a 202 (Accepted) status code if the action will likely succeed but has not yet been enacted,
• a 204 (No Content) status code if the action has been enacted and no further information is to be supplied, or
• a 200 (OK) status code if the action has been enacted and the response message includes a representation describing the status.

Interestingly, nobody is talking about 410 Gone here. Does that mean we should return 204 for successful deletion and 404 if the resource is not there? This would require some more code as we currently don't check if the resource is there but simply always delete it a (basically a no-op if there was nothing to be deleted).

So I went to my book shelf and took out the book "The Design of Web APIs" by Arnaud Lauret but they don't say much. On page 145 we find:

The DELETE HTTP method can be used on a resource to delete, undo, or cancel the concept by a URL. The 410 Gone response is quite explicit; according to the standard it "... indicates that the resource is no longer available and will not be available again." And further, it "... should be used when a resource was intentionally removed and the resource should be purged."

TBH, to me this very much sounds like: Everything is allowed.

We can use 410 Gone to signal this resource does not exist in the server (which is true) or we could instead use 204 No Content (on successful deletion) and 404 Not Found (if not existing) or maybe a weird combination like 204 for just-deleted, 404 for has-never-been-there, and 410 for has-been-there-but-deleted-previously (hint: I really dislike the last one because it'd mean we'd need to memorize what has been there somewhere)

So far, so unclear?

I am absolutely open for further thoughts and possibly also some references how others are doing it.

[rdwebdesign]

I'm far from an expert on this matter, but as far as I know, 4xx codes are Error codes:

The 4xx (Client Error) class of status code indicates that the client seems to have erred. Except when responding to a HEAD request, the server SHOULD send a representation containing an explanation of the error situation, and whether it is a temporary or permanent condition. These status codes are applicable to any request method. User agents SHOULD display any included representation to the user.

My understanding is 410 is used when the client requests for some HTTP resource once available, but not available anymore. From 410 code specification:

The 410 response is primarily intended to assist the task of web maintenance by notifying the recipient that the resource is intentionally unavailable and that the server owners desire that remote links to that resource be removed.

This is a special case and not what we have here.

In our case, if you successfully requested to delete an item and the item was there, no error occurred. The operation was successful (2xx).

[PromoFaux]

Running the command I provided inside the container should "just work"

Not in v6, a complete custom image will need to be built, but it's pretty straight forward! See:

https://github.com/pi-hole/docker-pi-hole/tree/development-v6?tab=readme-ov-file#building-an-image-with-alternative-component-branches

TL;DR

• Clone Docker Repo
• Switch to branch development-v6
• Run docker buildx build src/. --tag pihole_custom --build-arg FTL_BRANCH=fix/delete_codes --no-cache to build the image with this branch
• replace image: pihole/pihole:development-v6 in your compose file with image: pihole_custom
• Stop/Destroy/Start container.

[akordowski]

Thank you for the detailed explanation. I understand your point, but here how I see it. The API docs for this action are stating clear the possible HTTP codes 200, 401, 410 in the response. As I was deleting the SID for the first time I was expecting a success response code (200) but recieved instead an error code (410). Therefore I was confused, and after looking in the UI I was sure the SID was deleted.

The current results are as follow:

• First call DELETE /auth > Returns 410
• Second call DELETE /auth > Returns 401

I would rather expect the following result:

• First call DELETE /auth > Returns 200 or 204
• Second call DELETE /auth > Returns 410

However you decide to implement it, it should be documented much clearer so there is no confusion.

[rdwebdesign]

EDITED:

I 'm not sure if I understood your explanation about first and second calls...

The current results are as follow:
* First call DELETE /auth > Returns 410
* Second call DELETE /auth > Returns 401

I think you correctly received a 401, because on the first try you deleted the current session and the authorization failed for the second one (there was no valid session).

I would rather expect the following result:
* First call DELETE /auth > Returns 200 or 204
* Second call DELETE /auth > Returns 410

In my opinion, if you try to delete the same item twice, the first time it will answer with 200 or 204.
In the second attempt, the item was already removed, so 404 Not Found would be a better return code.

[akordowski]

In my opinion, if you try to delete the same item twice, the first time it will answer with 200 or 204.
In the second attempt, the item was already removed, so 404 Not Found would be a better return code.

This is what I have expected.

But currently the API at the first time answer with 410 and at the second time with 401.

[DL6ER]

Wait, before we mix some things here, let me first clarify:

1. The documentation is currently out of sync with the code, we will fix both at the same time when we found out what to do and how to do it. I don't want to say that either the code nor the documentation are the-one-single-source-of-truth at the moment. Let's assume they are both wrong for the sake of open-mindedness.

2. Getting 401 Unauthorized after deleting your own session is - no doubt - absolutely correct. You are simply not authorized any longer. The API will reject your request with 401 Unauthorized before even looking if such a session exists as you cannot prove you are allowed to edit/see anything.

As deleting your own session is a very special case, let's instead discuss the behavior of something that does not have such a special behavior like deleting the session of some other user or, even simpler, say

DELETE /api/clients/127.0.0.1

We will then apply our conclusion to all DELETE endpoints in the API.

---

Currently, the API returns 204 No Content when you call DELETE /api/clients/127.0.0.1. It does so when it deletes the entry, and it does the same when such an entry never existed. This is an optimization as the code does not check if an item exists or not before trying to delete it.

We can remove this optimization (make the API a little bit slower) and instead first check if the record exists. If not, then we reply with ... what? 404 Not Found? 410 Gone? My preference is 404 Not found as the status may be only temporarily and 410 Gone is some rather absolute statement.

TL;DR: Suggestion:

1. Successful deletion: 204 No Content
2. Item not present: 404 Not Found

[akordowski]

• The documentation is currently out of sync with the code, we will fix both at the same time when we found out what to do and how to do it. I don't want to say that either the code nor the documentation are the-one-single-source-of-truth at the moment. Let's assume they are both wrong for the sake of open-mindedness.

Good to know 😃

• Getting 401 Unauthorized after deleting your own session is - no doubt - absolutely correct. You are simply not authorized any longer. The API will reject your request with 401 Unauthorized before even looking if such a session exists as you cannot prove you are allowed to edit/see anything.

Ok, my bad. Just read in the API docs that the DELETE /auth action is meant as a LOGOUT.

Delete session
A logout attempt without a valid session will result in a 401 Unauthorized error.

Than the 401 code at the second call makes absolutley sense. But at the first call I would still expect a 2xx code rather then 410 Gone code 😉

[DL6ER]

I pushed by proposal in branch fix/delete_codes
Also, the online documentation will come up at https://ftl.pi-hole.net/fix/delete_codes/docs/#overview when the automated build process has finished.

It should become ready for testing in about half an hour. It'd be great if you could check it out and see if the returned codes are now as expected (and as documented). As we are approaching midnight over here, so my own testing was a bit limited.

[akordowski]

@DL6ER I will test it when the PR is merged and new version is released.

[DL6ER]

It is unclear when the team will have time to review this. If you could check out this branch before using

pihole checkout ftl fix/delete_codes

and provide feedback, this would tremendously help the process. Furthermore, it'd be much more efficient if we could test and then immediately change the PR than creating a fix, waiting for it to be merged, then merge it and see afterwards that it wasn't sufficient, starting the whole process over again. It could take months to get it eventually fixed in such an iterative approach...

[akordowski]

I would love to do it, but currently I am testing it using a Docker container. I'm not that experienced to get the branch compiled and running. I will try to find a way. Thank you for your efforts!

[DL6ER]

Running the command I provided inside the container should "just work"

[yubiuser]

Fixing PR has been merged.