Skip to content

refactor(security): make URL admission policy single-source across control plane and CLI #1162

Description

@seonghobae

Finding

appguardrail_core/controlplane.py and scanner/cli/appguardrail.py each define their own _is_safe_url() implementation. Fresh protected-branch inspection shows the same scheme/IP/DNS policy duplicated in both places, including the compatibility rule that tolerates socket.gaierror for syntactically valid dummy hostnames.

PR #1159 repairs the control-plane copy's missing-host fail-open boundary at exact head 58b48be2722bf60bb88c68c9f291a5fba608b12f, but the CLI copy remains a mutable sibling implementation. This is a security-policy ownership/DDD/SOLID gap: future changes can fix one consumer while leaving the other with different admission semantics.

RED acceptance

Against the then-current protected develop, add focused parity/admission contracts proving both consumers reject before DNS:

  • http://, https://;
  • http://user@, https://user@;
  • http:///path and equivalent missing-authority HTTP(S) inputs.

Also preserve hostile/public-network boundaries already owned by the policy: loopback/private/link-local/unspecified/multicast/reserved/non-global IPs remain rejected, mapped IPv6 is normalized, and malformed resolved IP data fails closed.

Preserve the current explicit compatibility contract for syntactically valid non-empty test hostnames that raise socket.gaierror unless an ADR deliberately replaces that behavior with a deterministic test resolver seam.

GREEN acceptance

Move URL admission into one canonical AppGuardrail-owned module/domain service and have control-plane redirects/webhooks and CLI consumers call that implementation. Do not copy source between consumers and do not add scanner suppression.

Acceptance requires:

  • one production implementation of the URL admission invariant;
  • consumer-level regressions for control plane and CLI;
  • 100% owned production docstring/test/edge-case coverage for the touched URL-policy surface;
  • exact-head repository tests plus Security/SAST/CodeQL terminal GREEN;
  • CodeGraph/call-site evidence showing the duplicate policy is removed or no longer authoritative;
  • code-current ADR/security/test-strategy or docs/product-technical-gap-baseline.md projection through its existing single-writer documentation lane;
  • normal protected integration only, with no force push, destructive rebase, self-approval, gate weakening, stale predecessor evidence, or mutable sibling-head dependency.

This issue does not relabel the malformed empty-host case as a demonstrated CRITICAL SSRF exploit. The verified defect is fail-open URL admission; severity should follow demonstrated reachability and network effect.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    • Status
      Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions