-
Notifications
You must be signed in to change notification settings - Fork 356
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
Deleting Push with Last View yields Error 500 #2522
Comments
Hey @Viajaz - I've been a bit underwater. I'll take a look at this soon. |
Let me know if I can provide more information. Sorry it's a bit light on details but, as far as I can tell, it seems to be just that simple to reproduce. |
Just reproduced the error now:
|
I switched to Confirmed in logs that
|
How are you deleting the push? Via API or UI? |
It was the UI, however, when I went to perform another reproduction, and collect some more HTTP request details for you, the error is now gone. I don't have an explanation, it was the same test instance that I had used 8 hours before this post and I checked to ensure the version was the latest when posting last. Regardless, I guess problem is fixed? |
No I think you are right. There is another code path that can give this error. I need a better fix. |
Hello, Testing here, If I delete a push sended to a Public gateway, the page dont redirect to the page "you link expired", its runs eternal. BUT the link is deleted, if I refresh manually the page (press F5), the link was gone. |
When a push is deleted/expired, if anonymous, you get redirected back to the "expired" page. If you are logged in and the push owner/creator, you get sent to the audit log. Audit log doesn't exist on the public gateway. My theory is that the problem is somewhere in this redirect logic. If you both test in an incognito window, does the problem still occur? I'll get back on this soon. |
There's an argument to be made about the need to even show a Delete button if this was the last view. |
This should be fixed now in v1.45.11.
If some of you could confirm, it would be very much appreciated. |
Hi, the issue is still very much active, after pushing files and accessing the link ( set for 1 view) . restarting the page will still show the blue screen error 500, but here is the thing I found out. If I upload 3 files ( set with 1 view) go to the push link. and after refreshing the page I get the blue screen error, but now I have to refresh 3 times to get to the expired page. and if you upload 5 files you got to refresh the page 5 times to get to the expired page.( tested with chrome, and iphone safari) on the side note, im not sure if your able to verify this , but with URLs ( set for 1 view) the link is expired before I even access it. tried on multiple devices and browser same issue. |
That sounds like an issue with the file storage. Could you get me the stack trace of the error from inside the public gateway container? The logs are in |
here are the logs from /opt/PasswordPusher/log/production.log 1343322be5ec:/opt/PasswordPusher/log$ tail -f production.log |
any update on this ? let me know if you need more logs. |
Apologies all - I am just flooded this week.
This definitely shouldn't be there and would definitely cause an error 500. The last view triggers an immediate expiration of the push - and deletes the payload and attached files. Can the pwpusher user in the container delete the files it lays down? |
Without the permission denied, the files are deleted. You can see the line of code in that stack trace that calls https://github.com/pglombardo/PasswordPusher/blob/master/app/models/file_push.rb#L45 When you are inside the container as user |
yes inside the pwpush container I was able to create and delete a text file. One thing id like to know, in the previous screen shot I shared , that is the storage for Files. are those directory's ( 0i , 73, b2, g4, t6) are they supposed to be auto removed by the container once the file link expires ? |
I would think so yes but if they are not, it's no big deal as they are just organizational directories based on blob ID. More important is that the files themselves are deleted. Occasionally if a file upload gets interrupted or something goes wrong (like the error 500) files can be orphaned as the process didn't complete. In that case you can run Between the pwpush and pwpush-public-gateway container, they use a shared filesystem mount? |
Filed #2646 so that I remember to do this soon. |
between the pwpush and the gateway container , they both use the same local storage thats on the host. When you mentioned permissions in your last comment, I added the (user: "0:0") to the gateway container as well, this actually seems to have resolved my error 500 issue. but those files I mentioned before just dont get cleaned up. inside pwpush console, how do I exactly run the active_storage:cleanup command ? an example would be great |
I'm away from the computer so let's see if I can type this. Log into pwpush container and run bin/pwpush active_storage:cleanup /opt/PasswordPusher directory Back in a bit |
its saying unrecognized command 15e41661190e:/opt/PasswordPusher# bin/pwpush active_storage:cleanup |
My bad - the command has changed in the beta release. The command for the latest pwpush container is:
|
well the command worked, but those directory are still there even tho my file pushes are expiring fine now and no error 500. |
Ok well that's good news then. Let me know if it happens again. Otherwise I'll close out this issue soon if nothing else remains. Have a good weekend! |
even tho the command you gave me ran successfully, the directory files still remained, is that standard ? or should it be removing those ? or do I need to manually remove these once in a while |
I would leave them. The blob IDs are randomly generated and the directories are used to organize files. This is really low level but if it helps, for every file uploaded a base64-encoded blob id is generated. It includes a signature for integrity and also contains other metadata:
Also this way you don't have uploaded files with sensitive filenames such as so tldr; leave the directories. |
alright sounds good , thanks Good to close the ticket |
🐛 Bug Report
Deleting a password push that is already on it's last view will yield HTTP error 500 on
pglombardo/pwpush-public-gateway:stable
🔬 How To Reproduce
Steps to reproduce the behavior:
pglombardo/pwpush-public-gateway:stable
Environment
Where are you running/using Password Pusher?
If applicable, what version of Password Pusher?: 1.45.4
📈 Expected behavior
Probably "We apologize but this secret link has expired."
The text was updated successfully, but these errors were encountered: