Skip to content

firewall add-rule does not enforce required source-ips/destination-ips for the chosen direction #1471

Description

@krishna3554

Summary

hcloud firewall add-rule documents --source-ips as "required when direction is in" and --destination-ips as "required when direction is out", but the command never enforces this. A rule can be submitted to the API with an empty IP list for its direction, and IPs passed for the opposite direction are silently ignored.

Location

  • File: internal/cmd/firewall/add_rule.go
  • Function: parseRuleFromArgs (lines 76–142) and the AddRuleCmd run function
  • Related docs: docs/reference/manual/hcloud_firewall_add-rule.md, flag help text in BaseCobraCommand

Problem

parseRuleFromArgs validates:

  • direction must be in/out (line 89–94)
  • protocol must be one of the five supported values (line 96–101)
  • --port is required for tcp/udp and rejected for icmp/esp/gre (lines 111–120)

However, there is no check that the IP list matching the chosen direction is non-empty, even though both the flag help text ("(required when direction is in)") and the generated manual page promise exactly that.

Additionally, the switch over rule.Direction (lines 122–139) only ever reads the list matching the direction: if a user passes --source-ips with --direction out (or vice versa), those values are silently dropped without warning.

Trigger / Reproduction

Static analysis finding — not confirmed by execution. From the code path on current main (1255e956):

# no --source-ids provided although --direction in is used:
hcloud firewall add-rule my-firewall --direction in --protocol tcp --port 80

parseRuleFromArgs returns a rule with SourceIPs = []net.IPNet{} (initialized empty at line 84–87, never appended), which is then sent to the API via SetRules together with the firewall's existing rules.

Similarly:

hcloud firewall add-rule my-firewall --direction out --protocol tcp --port 80 --source-ips 10.0.0.0/8

exits successfully while silently ignoring --source-ips.

Expected Behavior

Per the documented contract, the command should fail locally with a clear message when the IP list for the selected direction is missing (mirroring how --port is validated for tcp/udp), and ideally warn/error when IPs for the opposite direction are supplied but unused.

Actual Behavior

The rule with an empty source/destination list is submitted to the API; whether it is accepted or produces a remote error depends entirely on API-side validation. Opposite-direction flags are ignored without any indication.

Impact

  1. Users get inconsistent behavior between what the help text promises ("required") and what the CLI enforces — a confusing remote error instead of local validation at best.
  2. If the API accepts rules with empty IP lists, this can result in unintentionally permissive rules being added without the user noticing.
  3. Silently dropping opposite-direction IPs can hide typos like swapping --source-ips/--destination-ips.

Suggested Direction

In parseRuleFromArgs, after resolving the direction, require the corresponding slice to be non-empty (returning a usage-style error consistent with the existing port is required (--port) message). Optionally validate that the non-matching list is empty or emit a warning before proceeding.

Happy to follow up with more detail if useful.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions