fix: enforce https in webhook urls

[AngeloDanducci]

Pull Request

Issue

Fixes # - Partial for #1246

Description

Security hardening to enforce https and block ssrf targets

Also updated the formatting in utils.py to match our contributing guidelines - I assume I will continue to find some RST for the foreseeable future.

Testing

• Tests added to the respective file if code was changed
• New code has 100% coverage if code was added
• Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

• AI coding assistants used

---

Adding a new component, requirement, sampling strategy, or tool?

If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.

• Component
• Requirement
• Sampling Strategy
• Tool

NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.

[psschwei]

we could also use something like this:

import ipaddress
addr = ipaddress.ip_address(hostname)
if addr.is_loopback or addr.is_link_local or addr.is_private or addr.is_unspecified or addr.is_reserved:
    # block

explicit list is nice in that it's easier to audit, but then we have to keep it up to date (for example, should we include 0.0.0.0 and/or ::). don't have a strong opinion on which way is better (that's why just a comment rather than an explicit suggestion), wdyt?

[AngeloDanducci]

Valid question and definitely easier to maintain - though I think is_private is a superset of the others.

I think this would be fine but this also brings up one other question which is do we think this is sufficient or do we also need to guard against dns rebinding @psschwei ?

[psschwei]

Fixing the docstrings is good and we should keep that.

Enforcing https for forwarding logs is probably fine to keep too.

But I don't think we need the denylist. The threat it's protecting against requires the attacker to already have control of the environment, which would mean we have much bigger problems. So I would drop that part completely.

[psschwei]

Actually now that I think about it, i'm a little torn. If we require https, and someone wants to test things out locally, then they have to hit an HTTPS endpoint on their machine. I don't know if that's the right behavior or not (might be good for someone who uses this functionality more to weigh in too)

[jakelorocco]

I agree. The security risk seems minor.

• I would suggest we edit the PR title to mention only https (so that it's not misleading when merged).
• Maybe we can add a flag or something for http development like (MELLEA_LOGGER_INSECURE_HTTP_ALLOWED?).

[AngeloDanducci]

Edited PR title, added flag to allow http when desired.

requested changes were made

[psschwei]

one nit and one question:

do the changes we introduce here need an update in docs/docs/observability/logging.md ?

otherwise LGTM

[psschwei]

lgtm