Refactor/laravel quickstart#25
Conversation
devenney
left a comment
There was a problem hiding this comment.
Thanks for this, Kamil.
It looks great, just one observation from my side: the logs are not visible to the user. I think this stems from two decisions:
artisanis invoked with--quiet.- The log driver writes the logs to
storage/logs/laravel.logbut this is not exposed to the user (unless theydocker execand read the logs manually).
Perhaps this is intentional, but I think we should discuss whether we wish to expose such logging to the user while the container is running. I feel it's quite useful to see how the Approov logic is behaving in realtime.
| ], | ||
| 'null' => [ | ||
| 'driver' => 'null', | ||
| ], |
There was a problem hiding this comment.
I see the following error in laravel.log:
[2026-02-02 12:24:10] laravel.EMERGENCY: Unable to create configured logger. Using emergency logger. {"exception":"[object] (InvalidArgumentException(code: 0): Driver [null] is not supported. at /app/vendor/laravel/framework/src/Illuminat
e/Log/LogManager.php:225)
It seems that the solution is this:
| ], | |
| ], | |
| 'deprecations' => [ | |
| 'driver' => 'daily', | |
| 'path' => storage_path('logs/php-deprecation-warnings.log'), | |
| 'level' => 'debug', | |
| 'days' => 7, | |
| ], |
There was a problem hiding this comment.
I made a few changes to improve the logging. If the secret isn’t configured, the server logs now include a clear message. I also enhanced the overall logging to be more professional, for example, capturing what happened, where it happened, and the status code, among other details.
- trim whitespace and wrapping quotes, before placeholder check and base64url decode - validate APPROOV_BASE64URL_SECRET in scripts/build.sh before docker run (missing, placeholder, empty, invalid base64url) and require base64 CLI - show container logs when startup container exits early or readiness timeout happens to make docker startup failures easier to diagnose - map TypeError/ValueError in ApproovTokenVerifier to Approov error codes instead of falling back to generic internal verification errors - set APP_DEBUG default to false and update README line links plus token-binding responsibility descriptions
jexh
left a comment
There was a problem hiding this comment.
Hi Kamil, I've had a look through and I think the content is really good. What you have put together here is very impressive. There are a few changes that I would like you to make. I hope these are not things that contradict what Brendan has already asked.
- I think the README content is good but I think we need to reorder the sections. I think it should have the following structure:
- A short intro. Including a very short description of what Approov is: a sentence with a link to the website (in case someone lands in the repo before knowing what Approov is).
- A short contents list that links to the remaining sections in the file
- A walk through of how to get the example up and running (you currently have too much explanation up front. Most people will want to get the example up and running and then try to understand it.
- Then the descriptions of the different components and what they do
- We need to group the Approov functionality into a single package if we can. At present most of it is in ApproovApplication.php, then a bit in ApproovTokenVerifier and the rest in the Approov package. Ideally, all Approov files are in a single directory and then they are included from the places they are needed. Approov configuration, functionality, and any dynamic state should be controlled by code in the package. Then the app should call into that code to perform Approov related functionality. This is important so that it is as easy as possible for a developer to integrate the token check into their application. Additionally, when we eventually come to release an Approov package for php, it will be natural for people to switch to importing it from elsewhere.
- There need to be comments on each of the php functions that you have defined. Getting AI to create them is easy enough but you need to make sure they correctly describe what the function does, its inputs and its outputs. Comments should be provided in the normal format used for php packages.
|
Fixed all 3 points.
I didn’t add a contents section because both GitHub and IDEs already provide an outline view, so it felt unnecessary, but I can add it if needed. |
Summary
/token-check,/token-binding,/token-double-binding) and unprotected endpoints..envconfiguration, and a CLI test harness.Composer install.How to run
cp .env.example .env.APPROOV_BASE64URL_SECRET.bash run_server.sh.bash test.sh.