firefly-iii-fints-importer
firefly-iii-fints-importer copied to clipboard
bnw
apcu and session destruction
- Cleanup session on proper exit
- Added a warning about local file session
- If APCu is enabled, password is memory stored.
- Closes #118
The session cleanup should also be added to the error handler, but i was not able to get this running.
After investigating some more, it seems the "preferred" way to resolve this issue woud be using a shared memory session handler.
- mm - requires recompiling php with custom config
- memcache/memcached - requires setting a dedicated memcache server
The solutions would be cleaner, but are much more involved and probably a bit of overkill, at least in my opinion.
In case you are not satisfied with this solution, alternatives i could provide are:
- Createing an apcu-based session replacement. Much cleaner code, but not advised as this would not support proper locking: https://stackoverflow.com/questions/1724587/how-to-store-php-sessions-in-apc-cache. However, probably not an issue with this tool.
- Completely remove apcu and only check for memcached setup with a warning. However, installing apcu is simpler than memcached and therefore more friendly users.
Hi @TyrionWarMage, thanks a lot for your work. Sorry again for the late reply. Unfortunately, I know very little about the safety implications of this change. If we write the pin into some globaly accessible apcu, don't we make it available to every php script on the system? 😅
According to https://stackoverflow.com/questions/34533695/is-the-new-apcu-apc-user-cache-shared-between-processes, it will be shared between all Apache/Nginx sessions, but not CLI. Surely not optimal, but probably beats a plain text file, readable by every script/process running with the same user or group. Furthermore, the cache is cleared once the import is done and it is required to manually enable APCu.
I'm also no security expert but i usually try to secure my file system, but think if an intruder is in your memory, everything is too late anyhow...
Of course,I'm for merging, but it is your call ... Just let me know.
Ok, I guess you are right and this improves the situation. I'm willing to merge this, but have two requests:
- Could you consolidate all the apuc code into one class? Maybe some PasswordStorage class or similar?
- Could you double-check that apcu is working in the docker container? This will ensure that the pin is stored in memory at least for this common use-case. Maybe you could add a simple unit test for this? The github action will run the test inside the built docker container.
Thanks again for your work and your input 😊 🙏
Ok, I guess you are right and this improves the situation. I'm willing to merge this, but have two requests:
1. Could you consolidate all the apuc code into one class? Maybe some PasswordStorage class or similar? 2. Could you double-check that apcu is working in the docker container? This will ensure that the pin is stored in memory at least for this common use-case. Maybe you could add a simple unit test for this? The [github action will run the test inside the built docker container](https://github.com/bnw/firefly-iii-fints-importer/blob/8c8063ca71943814a7a92c45e40cee6250ffe02f/.github/workflows/publish-docker-image.yml#L42).Thanks again for your work and your input 😊 🙏
Done. I'm reusing the global session object to not require instantiating the new class. Alternatively, i could make the session an argument, but i preferred this way because session is only used in the not-apcu path