Regression: cannot post images anymore

JayVii commented 1 week ago

Looks like 2.4.0 introduced a little regression, as images attached to posts are not shown anymore after submission.

I tried different images with various formats either with and without caption.

toddsundsted commented 6 days ago

thanks! this is fixed in https://github.com/toddsundsted/ktistec/commit/e6ad89660e634d503d032ad3e249d0ec95e8566f

i found a few regressions in 2.4.0. i am going to release 2.4.1 very shortly.

toddsundsted commented 6 days ago

and released

JayVii commented 5 days ago

For some reason I can still not post images on 2.4.1 (via docker). I will investigate further whether this is an issue on my end some time this week.

Perhaps my /uploads folder has wrong permissions, if i interprete your fixes correctly

toddsundsted commented 5 days ago

can you check to see if 1) the file is actually uploaded, 2) what the permissions are?

JayVii commented 4 days ago

The image was not uploaded, however ktistec did create a folder-structure for it inside /uploads with permissions 755/rwxr-xr-x (the same as all other folders), however owned by root:root instead of www-data:www-data

JayVii commented 4 days ago

So, potentially (without knowing crystal at all...), this could have been caused in:

https://github.com/toddsundsted/ktistec/compare/v2.3.0...v2.4.1#diff-db5856ebf33ad088d67cd065519dccd593194202fe65b91dc60390f9967af1c5R59 (albeit this seems like a check/read action, not setting permissions?)
https://github.com/toddsundsted/ktistec/compare/v2.3.0...v2.4.1#diff-45f09ad8f46bf2b988a83018cac9aa996b67a5c8aef22b7dc9e5c8b7056fe3b7R97 & https://github.com/toddsundsted/ktistec/compare/v2.3.0...v2.4.1#diff-a248dd6a77ab4447934c5af5d08c30ecc5d62d7d876050dedc90236f3793ac14R20 (that is the 2.4.1 patch IIRC, however the folder-permissions do not appear to be 644, but instead 755. However, this line targets the uploaded file if I am not mistaken, which is not present, though. The directory permissions are already out of line, so this is never applied)

Lastly, of course this could be a docker-related issue. I could try to rebuild the container and creating the www-data user (or a user with the same UID) and run the container with the according USER argument? This should generally not be necessary, though

toddsundsted commented 4 days ago

ktistec did create a folder-structure for it inside /uploads

is the /uploads folder empty? every time a file is uploaded, Ktistec creates a randomly named directory hierarchy for the uploaded file. i would expect the directories to exist, but to be empty?

JayVii commented 3 days ago

ktistec did create a folder-structure for it inside /uploads

is the /uploads folder empty? every time a file is uploaded, Ktistec creates a randomly named directory hierarchy for the uploaded file. i would expect the directories to exist, but to be empty?

Yes, this is exactly what is happening. Th randomly named folder hierarchy is created (owner is root with 755-perms), but the image file is not there

toddsundsted commented 3 days ago

thanks! yeah, i think this is related to the change i made to move the temporary file into place vs. copying the temporary file. i'm wondering if, in some environments, the file is being unlinked after the move despite it having been moved.

JayVii commented 3 days ago

i'm wondering if, in some environments, the file is being unlinked after the move despite it having been moved.

Maybe this can be gathered from the logs? Is there anything i could set to debug from the /system view? If not, i could try to set up a filesystem notify on my server and upload an image

toddsundsted commented 3 days ago

the only thing i can think of are any obvious stack traces or other indicators in the logs. when you do an upload you should see something like the following if it is successful. if it's anything other than 201 then something went wrong:

2024-11-08 14:45:56 UTC 201 POST /uploads 7.24ms

if you see a 201 and there is no stack trace, then i suspect my theory about the file being unlinked is correct, but it would be great to have confirmation.

toddsundsted commented 3 days ago

after looking through the library code for tempfiles and handling uploads, i'm less convinced my theory is correct. if you want to try to revert the change locally, it's four lines of diff:

https://github.com/toddsundsted/ktistec/commit/ae321b7f01f22b2320ac486b3274b63b4c5112b8#diff-a248dd6a77ab4447934c5af5d08c30ecc5d62d7d876050dedc90236f3793ac14L20

JayVii commented 2 days ago

With current 2.4.1 without any changes, trying to post an image:

2024-11-14 21:15:46 UTC 200 GET /actors/jayvii 12.08ms
Exception: Error renaming file: '/tmp/cd1xxd1z' -> './public/uploads/e0db20d6/8fbf/428d/1.png': Cross-device link (File::Error)
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???
  from ???

2024-11-14 21:16:02 UTC 500 POST /uploads 605.5ms
2024-11-14 21:16:09 UTC 302 POST /actors/jayvii/outbox 11.71ms
2024-11-14 21:16:09 UTC 200 GET /remote/objects/842421 2.55ms
2024-11-14 21:16:09 UTC 200 GET /uploads/673de797/7b0e/4bd3/1.png 1.23ms
2024-11-14 21:16:09 UTC 200 GET /objects/PHFacvFXTXI/replies 2.62ms
2024-11-14 21:16:10 UTC 200 GET /objects/PHFacvFXTXI/replies 2.53ms
2024-11-14 21:16:11 UTC 200 GET /objects/PHFacvFXTXI/replies 2.75ms
2024-11-14 21:16:11 UTC 200 GET /objects/PHFacvFXTXI/replies 2.11ms
2024-11-14 21:16:12 UTC 200 GET /objects/PHFacvFXTXI/replies 2.15ms
2024-11-14 21:16:12 UTC 200 GET /objects/PHFacvFXTXI/replies 2.04ms
2024-11-14 21:16:13 UTC 200 GET /objects/PHFacvFXTXI/replies 2.13ms
2024-11-14 21:16:13 UTC 302 POST /actors/jayvii/outbox 5.26ms
2024-11-14 21:16:13 UTC 200 GET /actors/jayvii 13.49ms

Note: the image /uploads/673de797/7b0e/4bd3/1.png visible in the logs is my profile picture, NOT the posted image. This line in the logs is unrelated.

Note 2: the path referenced in the error output public/uploads/e0db20d6/8fbf/428d/1.png does not exist. Neither does the folder structure public/uploads/e0db20d6/8fbf/428d/, public/uploads/e0db20d6/8fbf/, public/uploads/e0db20d6/

JayVii commented 2 days ago

Now, running with following patch (from the diff you posted above):

diff --git a/src/controllers/uploads.cr b/src/controllers/uploads.cr
index 0e524496..0b0e9e4c 100644
--- a/src/controllers/uploads.cr
+++ b/src/controllers/uploads.cr
@@ -8,7 +8,7 @@ class UploadsController
   post "/uploads" do |env|
     filename = nil
     filepath = nil
-    env.params.files.each do |name, part|
+    HTTP::FormData.parse(env.request) do |part|
       if part.filename.presence
         filename = env.account.actor.id.to_s
         filepath = File.join("uploads", *Tuple(String, String, String).from(UUID.random.to_s.split("-")[0..2]))
@@ -17,8 +17,9 @@ class UploadsController
           filename = "#{filename}#{extension}"
         end
         Dir.mkdir_p(File.join(Kemal.config.public_folder, filepath))
-        part.tempfile.chmod(0o644) # fix permissions
-        part.tempfile.rename(File.join(Kemal.config.public_folder, filepath, filename))
+        File.open(File.join(Kemal.config.public_folder, filepath, filename), "w") do |file|
+          IO.copy(part.body, file)
+        end
       end
     end
     if filename && filepath
diff --git a/src/framework/method.cr b/src/framework/method.cr
index 39216df9..8531c548 100644
--- a/src/framework/method.cr
+++ b/src/framework/method.cr
@@ -18,7 +18,7 @@ module Ktistec
       # "application/x-www-form-urlencoded" on activities. see:
       # https://github.com/pixelfed/pixelfed/issues/3049
       return call_next env unless env.request.method == "POST"
-      return call_next env if env.request.path.ends_with?("/inbox")
+      return call_next env if env.request.path == "/uploads" || env.request.path.ends_with?("/inbox")
       return call_next env unless env.params.body["_method"]? == "delete"

       # switch method and fix URL params

Posting an image does work indeed and produces the following output in the logs:

2024-11-14 21:40:57 UTC 200 GET /actors/jayvii 12.94ms
2024-11-14 21:40:57 UTC 200 GET /actors/jayvii/posts 11.85ms
2024-11-14 21:41:17 UTC 200 GET /actors/jayvii/followers 42.77ms
2024-11-14 21:41:22 UTC 201 POST /uploads 1.23ms
2024-11-14 21:41:22 UTC 200 GET /uploads/193d7bbd/8aca/40f1/1.png 572.27µs
2024-11-14 21:41:26 UTC 302 POST /actors/jayvii/outbox 37.18ms
2024-11-14 21:41:26 UTC 200 GET /remote/objects/842443 6.34ms
2024-11-14 21:41:26 UTC 200 GET /3rd/themes/default/assets/fonts/Lato-Italic.woff2 396.92µs
2024-11-14 21:41:27 UTC 200 GET /uploads/193d7bbd/8aca/40f1/1.png 115.16µs
2024-11-14 21:41:27 UTC 200 GET /objects/CQO1D3QsonI/replies 1.97ms
2024-11-14 21:41:28 UTC 200 GET /uploads/193d7bbd/8aca/40f1/1.png 126.69µs
2024-11-14 21:41:28 UTC 200 GET /uploads/193d7bbd/8aca/40f1/1.png 87.01µs
2024-11-14 21:41:28 UTC 200 GET /uploads/193d7bbd/8aca/40f1/1.png 135.07µs
2024-11-14 21:41:28 UTC 200 GET /objects/CQO1D3QsonI/replies 1.9ms
2024-11-14 21:41:28 UTC 200 HEAD /uploads/193d7bbd/8aca/40f1/1.png 66.82µs
2024-11-14 21:41:28 UTC 200 GET /objects/CQO1D3QsonI/replies 2.17ms
2024-11-14 21:41:28 UTC 200 GET /uploads/193d7bbd/8aca/40f1/1.png 90.17µs
2024-11-14 21:41:29 UTC 200 GET /objects/CQO1D3QsonI/replies 1.52ms
2024-11-14 21:41:29 UTC 200 GET /uploads/193d7bbd/8aca/40f1/1.png 101.88µs

toddsundsted commented 2 days ago

perfect, thanks!

Exception: Error renaming file: '/tmp/cd1xxd1z' -> './public/uploads/e0db20d6/8fbf/428d/1.png': Cross-device link (File::Error)

from that exception and stack trace i can tell that the temporary directory and image directory are on different mounts, and it's not possible to rename across them—the file has to be copied. i'm going to look at one alternative to this, but i may just revert that rename change and go back to copying which is known to work.

i appreciate your help with this!

toddsundsted commented 2 days ago

@JayVii i just pushed 0cc6e635 which should fix this.

JayVii commented 2 days ago

from that exception and stack trace i can tell that the temporary directory and image directory are on different mounts, and it's not possible to rename across them

So this is in fact a docker-related issue then. The uploads directory is a mounted directory from the docker host, while the /tmp directory is container internal.

Thank you a lot for the effort! I am going to test the patch some time today and report back here.

toddsundsted commented 2 days ago

not docker-related—it could have happened on any linux host with a separate mount of temporary files. probably pretty common except in my hosting configuration.

JayVii commented 2 days ago

@JayVii i just pushed 0cc6e63 which should fix this.

This fixed the issue. Thanks a lot!

toddsundsted commented 2 days ago

no problem! thanks for your help debugging!

toddsundsted / ktistec

Regression: cannot post images anymore #117