zqr10159 commented on PR #4386: URL: https://github.com/apache/hertzbeat/pull/4386#issuecomment-5908352474
Thanks for working on this hardening. I think we need to address compatibility and handler-level coverage before merging: 1. The unconditional private-address rejection also blocks legitimate internal Gotify, Ntfy and generic webhook deployments, including Gotify URLs configured by the operator in AlerterProperties. Under the documented trusted-user model, please discuss an administrator-controlled allowlist or configurable policy so operators can explicitly retain intended internal notification endpoints. 2. The existing GotifyAlertNotifyHandlerImplTest sets http://localhost:8080/gotify/%s and testNotifyAlertSuccess calls send(). The newly added validation rejects that address before the mocked HTTP call. Ntfy success tests similarly use ntfy.example.com, but now perform real DNS resolution before reaching the mock. Please make the handler tests deterministic and cover permitted endpoints, rejected endpoints and the chosen internal-endpoint policy, then run the full alerter suite. 3. Regarding the acknowledged redirect limitation: RestTemplateConfig currently enables HttpClient.Redirect.NORMAL, while this validation runs only before the initial request. Please either disable redirects for these outbound notification requests or validate each redirect destination under the same policy, with regression coverage. These are source-review findings, not locally executed test results or a demonstrated exploit. Backend and license CI are still awaiting maintainer approval. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
