Skip to content

fix(xo-server-netbox): use numeric comparison to find the most specific prefix - #10297

Open
matlantin wants to merge 4 commits into
vatesfr:masterfrom
matlantin:fix/netbox-prefix-string-comparison
Open

fix(xo-server-netbox): use numeric comparison to find the most specific prefix#10297
matlantin wants to merge 4 commits into
vatesfr:masterfrom
matlantin:fix/netbox-prefix-string-comparison

Conversation

@matlantin

Copy link
Copy Markdown

Fixes #10240.

Root cause

In packages/xo-server-netbox/src/index.js, the "find the smallest (most specific) prefix" logic destructures bits from prefix.split('/'), which is a string, then compares it with >/=== against highestBits:

let smallestPrefix
let highestBits = 0
nbPrefixes.forEach(({ prefix }) => {
  const [range, bits] = prefix.split('/')
  ...
  if (... && bits > highestBits) {
    smallestPrefix = prefix
    highestBits = bits
  }
})

The first comparison (bits > 0) works correctly because JS coerces the string to a number when compared against a number. But once highestBits itself becomes a string (assigned from bits), every subsequent comparison is lexicographic, not numeric: "8" > "24" evaluates to true ('8' > '2' character-by-character), even though 8 < 24 numerically.

Effect: a prefix with a single-digit mask (typically a /8 container/supernet prefix) wins over any two-digit, more specific prefix (/10-/79) that also matches the IP, regardless of the order nbPrefixes is iterated in. The same bug affects the later idempotency check (bits === highestBits), which never matches once bits and highestBits are of different types.

Real-world repro: a NetBox instance with the common container-prefix hierarchy 10.0.0.0/8 (container) → 10.10.0.0/16 (container) → 10.10.10.0/24 (active) — syncing a VM with IP 10.10.10.7 creates 10.10.10.7/8 in NetBox instead of the expected 10.10.10.7/24. Subnets without a competing single-digit-mask container prefix (e.g. a bare 192.168.1.0/24 with no /8 registered) are unaffected, which is why this went unnoticed for a while.

Fix

Force numeric comparison in both places, matching the change validated on a production instance (fixed real IPs that were previously synced as /8):

- if (parsedRange.kind() === ipKind && parsedIp.match(parsedRange, bits) && bits > highestBits) {
+ if (parsedRange.kind() === ipKind && parsedIp.match(parsedRange, bits) && Number(bits) > highestBits) {
    smallestPrefix = prefix
-   highestBits = bits
+   highestBits = Number(bits)
  }
- return nbCompactIp === xoCompactIp && bits === highestBits
+ return nbCompactIp === xoCompactIp && Number(bits) === highestBits

Testing

  • Verified against a live NetBox instance: before the fix, VMs on a 10.x.x.x subnet with a 10.0.0.0/8 container prefix registered were synced with a /8 mask; after the fix (deployed to the running xo-server, followed by xo-cli netbox.synchronize), the same VMs are synced with their correct, more specific /24 mask, and re-running the sync is idempotent (no repeated create/delete).
  • node --check passes on the modified file. This package has no existing unit tests to run.

matlantin added a commit to matlantin/xen-orchestra that referenced this pull request Aug 25, 2026
@b-Nollet
b-Nollet requested a review from pdonias August 27, 2026 09:47
Comment thread CHANGELOG.unreleased.md Outdated
Comment thread CHANGELOG.unreleased.md Outdated
Comment thread packages/xo-server-netbox/src/index.js Outdated
@matlantin
matlantin force-pushed the fix/netbox-prefix-string-comparison branch from ff93cbb to ca39e52 Compare August 31, 2026 09:15
matlantin added a commit to matlantin/xen-orchestra that referenced this pull request Aug 31, 2026
@matlantin

Copy link
Copy Markdown
Author

Rebased onto current `master` (the branch had fallen behind after a release cleared `CHANGELOG.unreleased.md`'s package list) and re-added `xo-server-netbox patch` there. Also applied the `bits`-as-number suggestion in a follow-up commit — replied on that thread.

@pdonias your directly-pushed changelog wording (issue link) is preserved, it matched what I'd already written when resolving the rebase conflict on that file.

All 3 review comments should be addressed now.

Comment thread packages/xo-server-netbox/src/index.js
…ic prefix

`prefix.split('/')` returns bits as a string. Comparing it directly
with `>`/`===` works numerically only on the first iteration (against
the initial `highestBits = 0`), then degrades to a lexicographic
string comparison once `highestBits` itself becomes a string. E.g.
`"8" > "24"` is `true` as strings, even though 8 < 24 numerically.

Effect: any prefix with a single-digit mask (typically a /8 container
prefix) wins over a more specific two-digit prefix that also matches,
regardless of iteration order. Reported in vatesfr#10240.

Fixes vatesfr#10240
Per review feedback: destructure bits as a number at the point each
prefix string is parsed, instead of casting with Number(bits) at each
comparison site. Also renamed the raw split fragment in the second
match block to bitsStr for consistency, since it's compared against
the now-numeric highestBits.
…tency check too

Per review feedback: mirror the prefix loop above so both blocks
convert bits the same way, at the point of destructuring rather than
inline at the comparison site.
@matlantin
matlantin force-pushed the fix/netbox-prefix-string-comparison branch from ca39e52 to a2a5b35 Compare August 31, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xo-server-netbox: prefix selection uses string comparison, /8 wins over more specific prefixes

2 participants