Explicit header whitelist in configuration #336
Conversation
jeffkreeftmeijer
force-pushed
the
explicit-header-whitelist
branch
from
a416644 to
9d3dd4d
Compare
|
Related Ruby PR: appsignal/appsignal-ruby#406 |
Too much personal data being sent that's not GDPR compliant.
tombruijn
force-pushed
the
explicit-header-whitelist
branch
from
057af38 to
54ebebb
Compare
|
@jeffkreeftmeijer updated the header list. Now ready for review? |
|
Yes! PLease do. :) |
|
|
||
| Please check https://github.com/appsignal/appsignal-elixir/pull/336 for more information on this change. | ||
| """) | ||
| end |
There was a problem hiding this comment.
This warning is also being printed during the installation now, which is kind of weird. I think it's missing the config for the installation task when it sends a demo sample.
|
The warning will pop up when AppSignal is already installed without a |
|
@jeffkreeftmeijer I can confirm this still happens when removing the old config. The fix for this is setting the For example: config = %{active: true, push_api_key: push_api_key, request_headers: []} |
|
Ah I can confirm it also still happens with the proposed fix if you run the installer again in a project with a config already, but without the request_headers option. Less of a problem and I would suggest it's a non-fix because you wouldn't run the installer again if you've run it already. |
This suppresses the warning message about the missing `request_headers` config option. It's set to an empty list as the config option doesn't apply to the demo samples that are send later in the installer. This creates the most minimal config needed in the installer. No need to set the suggested request_headers here.
| end | ||
|
|
||
| defp test? do | ||
| Mix.env |
There was a problem hiding this comment.
@jeffkreeftmeijer It seems you've added a runtime dependency to Mix here, this breaks our distillery based releases which don't have Mix at runtime... Just upgraded from 1.5.0 to 1.6.0 and our app no longer boots.
There was a problem hiding this comment.
Great job on fixing this so fast 🎉
We have got the same issue as @doughsay with our distillery based deploys, so subscribe me to the next release notification :)
There was a problem hiding this comment.
@lucasmazza @doughsay We've released the 1.6.1 release yesterday that reverses this change. Please upgrade and let us know if you're running into any other issues.
Adds an explicit header whitelist to the configuration file or environment variables created/suggested by the installer. Closes #317.
TODO
Note: This is a work in progress until we've decided on the final list of default headers.