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

Expired passwords aren't flagged as expired upon hitting the view thresholds #481

Closed
Heathland opened this issue Nov 22, 2022 · 4 comments
Closed
Labels

Comments

@Heathland
Copy link

When creating a password with a max of 1 view, and opening this secret, the secret will still show as 'active' on the dashboard.
So it looks like hitting the 1/1 view, does not expire the password.
However, opening it again, making it 2/1 will expire the password.

This isn't that much of an issue, apart from the fact that the password (payload_ciphertext) only gets removed from the database upon being flagged as expired. So security-wise, it's better to flag the password as expired upon hitting 1/1.

Example:
In the dashboard
image
In the DB
image

After opening it again, triggering the expire
In the dashboard
image
In the DB
image

@pglombardo
Copy link
Owner

Hi @Heathland - Thanks for pointing this out. I agree - here is some background, reasoning and what the current solution is.

To expire pushes while the data is still in-flight (being delivered to the user) brings unnecessary complexity in the code between controllers and views. This can be avoided by scheduling an async expire job to run after the view has been delivered.

This is a good solution but now docker containers also have to also run a background job server (such as Sidekiq etc..) which also then requires a background Redis instance. A lot of extra baggage to expire passwords immediately.

Since I'm the only maintainer, I also aim to keep things as simple as possible so maintenance doesn't get out of control and this ending up being an abandoned project.

So as you pointed out, passwords are validated/expired before rendering them to users which can leave a middle state of technically expired but not deleted yet. These pushes can't ever be accessed but still the encrypted payload is on disk.

So to keep things simple, there are background jobs that can be run periodically to pre-emptively expired and delete payloads.

I run these tasks daily on pwpush.com and recommend those who self-host to do similar.

I also created #468 to have this happen automatically in the containers - run those background jobs hourly or daily (via cron) automatically for users.

This is a lot - I hope I didn't overload you. Did I explain this well enough?

@pglombardo
Copy link
Owner

Despite what I said, I don't like that dashboard experience. Pushes should be moved to expired as soon as possible.

I'll give this some thought.

@Heathland
Copy link
Author

Hi @pglombardo,

You've explained it perfectly. I suspected it as much that it had to do with sync and async processes.
I'm using the docker container with MySQL as hosting. So I'll have a look if I can find a pretty way to get it work in Docker.
If I find a nice solution to get the background jobs to work I'll share it in #468 as well.
If we can run it, say, hourly (depending on the load). It would minimize the risk even further

Thank you for your quick answer, and most of all, for all the work you're doing on this project!

@pglombardo
Copy link
Owner

Hi @Heathland - just an update on this that I was wrong. What I said above was historical but now seems easy enough to add.

I created #482 and will look to release it over the weekend. Assuming no issues come up, it will be live by Monday with new containers built.

Thanks for pointing this out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants