From 7e736c82cbf7b289e48abcf2b30a5e0c6749d8dc Mon Sep 17 00:00:00 2001 From: ch <1938389+netravnen@users.noreply.github.com> Date: Wed, 19 Aug 2026 20:43:11 +0200 Subject: [PATCH] fix(fp): distinguish an unreadable IX-F export from a genuine absence The per-row "Verify IX-F" check treated an export it could not read as proof that the network is not in the feed. extractIxfMatchForAsnIp coerced a missing or non-array member_list to [], found nothing in it, and returned "none" -- which the panel renders as "IX-F export has no entry for AS at this exchange at all" directly above a "Remove netixlan entry" button. A 200-OK CDN error page, a renamed schema, a feed that moved, or an ixlan whose ixf_ixp_member_list_url points at a different exchange all take that path, so an admin acting on a confident negative could delete a netixlan row the feed never actually contradicted. "none" is a positive claim, not a default. It is now returned only from an export that could be read, and everything else reports as inconclusive with no destructive control offered. Changes: - extractIxfMatchForAsnIp returns matched: "unreadable" with a machine `reason` when member_list is missing, non-array or empty, and when there is no target ASN to look up. All returns carry `reason` so the shape stays uniform. - Add renderUnreadableIxfExportResult: names the check inconclusive, explains that this is not evidence for removal, and shows the source URL so the admin can inspect the feed. It deliberately offers no remove or resolve button, and says so in its docblock. - Branch to it before the "none" case in the verify flow; document on renderNoIxfEntryResult why it is now unreachable on a false negative. Security: - Closes a path where unvalidated third-party data (an exchange's own export URL, fetched cross-origin) could induce an admin to delete a live netixlan record. The feed is not attacker-controlled in the usual sense, but it is third-party, unauthenticated, and frequently broken. Testing: - node --test: 507 tests, 506 pass, 1 skipped (live tests are opt-in). - Replaced the test that asserted the old conflated behavior. New cases cover null body, empty object, non-IX-F JSON, non-array member_list, and present-but-empty member_list -- each must be "unreadable" and explicitly not "none". - Kept a case proving "none" still fires for a readable export that genuinely lacks the ASN, so the Remove path is not silently dead. - Removed the guard and confirmed 4 failures, then restored. Backwards Compatibility: - extractIxfMatchForAsnIp gains a fourth `matched` value and a `reason` field. Its only caller is updated in the same commit; it is exported on the test hooks but has no other consumer. buildIxfDiff already keys off matched === "ip" and returns [] for the new state unchanged. Assisted-by: Claude:claude-opus-5 --- .../peeringdb-fp-consolidated-tools.src.js | 71 +++++++++++++++++-- .../peeringdb-fp-consolidated-tools.user.js | 71 +++++++++++++++++-- user.js/tests/fp-netixlan-ixf-verify.test.js | 51 +++++++++++-- 3 files changed, 178 insertions(+), 15 deletions(-) diff --git a/user.js/peeringdb-fp-consolidated-tools.src.js b/user.js/peeringdb-fp-consolidated-tools.src.js index a2dbe63..f30bf8e 100644 --- a/user.js/peeringdb-fp-consolidated-tools.src.js +++ b/user.js/peeringdb-fp-consolidated-tools.src.js @@ -1169,7 +1169,11 @@ * @ai Keep behavior stable and prefer minimal, localized edits. * @param {object} ixfData - Parsed IX-F member-export JSON. * @param {{ asn: string|number, ipaddr4: string, ipaddr6: string }} target - * @returns {{ matched: "ip"|"asn-only"|"none", connection: object|null, vlan: object|null, asnEntries: Array<{v4: string, v6: string}> }} + * "none" is a positive claim of absence and the UI turns it into a + * "Remove netixlan entry" button, so it is only ever returned from an + * export we could actually read. Anything we could not interpret comes + * back as "unreadable" instead -- see the guard in the body. + * @returns {{ matched: "ip"|"asn-only"|"none"|"unreadable", reason: string, connection: object|null, vlan: object|null, asnEntries: Array<{v4: string, v6: string}> }} */ function extractIxfMatchForAsnIp(ixfData, { asn, ipaddr4, ipaddr6 } = {}) { const targetAsn = String(asn ?? "").trim(); @@ -1178,7 +1182,19 @@ const memberList = Array.isArray(ixfData?.member_list) ? ixfData.member_list : []; const asnEntries = []; - if (!targetAsn) return { matched: "none", connection: null, vlan: null, asnEntries }; + // A 200-OK body is not the same as a readable IX-F export. A CDN error + // page, a renamed schema, a feed that moved, or a URL pointing at another + // exchange all parse to an empty member_list -- that is silence, not + // evidence that the network is absent. Coercing it to [] and reporting + // "none" told the admin "IX-F has no entry for this ASN at all" and + // offered to delete the netixlan row on the strength of it. + if (!memberList.length) { + return { matched: "unreadable", reason: "no-member-list", connection: null, vlan: null, asnEntries }; + } + // Likewise, with no ASN to look for there is nothing to conclude. + if (!targetAsn) { + return { matched: "unreadable", reason: "no-target-asn", connection: null, vlan: null, asnEntries }; + } for (const member of memberList) { const memberAsn = String(member?.asnum ?? member?.asn ?? "").trim(); @@ -1196,13 +1212,13 @@ const v4Matches = !!targetV4 && v4 === targetV4; const v6Matches = !!targetV6Norm && !!v6 && normalizeIpv6ForCompareFp(v6) === targetV6Norm; if (v4Matches || v6Matches) { - return { matched: "ip", connection, vlan, asnEntries }; + return { matched: "ip", reason: "", connection, vlan, asnEntries }; } } } } - return { matched: asnEntries.length ? "asn-only" : "none", connection: null, vlan: null, asnEntries }; + return { matched: asnEntries.length ? "asn-only" : "none", reason: "", connection: null, vlan: null, asnEntries }; } /** @@ -2582,10 +2598,53 @@ } } + /** + * Renders the "we could not read this IX-F export" result. + * Purpose: Report that the check was inconclusive, distinctly from + * reporting that the network is genuinely absent from the feed. + * Necessity: Both used to render the same panel, so an unreadable export + * -- a CDN error page, a moved feed, a URL pointing at the wrong exchange + * -- presented as a confident negative alongside a "Remove netixlan + * entry" button. This state deliberately offers no destructive action; + * the admin's next step is to look at the feed, not to delete a row. + * @ai Preserve the absence of a remove/resolve control here. + * @param {HTMLElement} panel - Result panel for this netixlan row. + * @param {object} netixlanRow - Live netixlan record being checked. + * @param {string} reason - Machine reason from extractIxfMatchForAsnIp. + * @param {string} ixfUrl - Export URL that was fetched. + * @returns {void} + */ + function renderUnreadableIxfExportResult(panel, netixlanRow, reason, ixfUrl) { + panel.textContent = ""; + + const heading = document.createElement("div"); + heading.textContent = "IX-F check inconclusive"; + heading.style.fontWeight = "bold"; + panel.appendChild(heading); + + const detail = document.createElement("div"); + detail.textContent = reason === "no-target-asn" + ? `Netixlan #${netixlanRow.id ?? ""} has no ASN to look up, so the export could not be checked.` + : "The export was fetched but contains no readable member_list, so it cannot show whether " + + `AS${netixlanRow.asn} is present. This is not evidence that the entry should be removed.`; + panel.appendChild(detail); + + if (ixfUrl) { + const source = document.createElement("div"); + source.style.marginTop = "4px"; + source.textContent = `Source: ${ixfUrl}`; + panel.appendChild(source); + } + } + /** * Renders the "IX-F has no entry for this ASN at all" result, with a * "Remove netixlan entry" option (see removeNetixlanEntry for why this * is a native confirm() rather than a second custom button). + * Only reachable from a readable export -- extractIxfMatchForAsnIp + * returns "unreadable" rather than "none" when it could not interpret + * the feed, precisely so this destructive path cannot be reached on a + * false negative. * @ai Keep behavior stable and prefer minimal, localized edits. */ function renderNoIxfEntryResult(panel, netixlanId, netixlanRow) { @@ -2699,6 +2758,10 @@ ipaddr6: netixlanRow.ipaddr6, }); + if (matchResult.matched === "unreadable") { + renderUnreadableIxfExportResult(panel, netixlanRow, matchResult.reason, ixfUrl); + return; + } if (matchResult.matched === "none") { renderNoIxfEntryResult(panel, netixlanId, netixlanRow); return; diff --git a/user.js/peeringdb-fp-consolidated-tools.user.js b/user.js/peeringdb-fp-consolidated-tools.user.js index 532188f..eb3372d 100644 --- a/user.js/peeringdb-fp-consolidated-tools.user.js +++ b/user.js/peeringdb-fp-consolidated-tools.user.js @@ -1571,7 +1571,11 @@ * @ai Keep behavior stable and prefer minimal, localized edits. * @param {object} ixfData - Parsed IX-F member-export JSON. * @param {{ asn: string|number, ipaddr4: string, ipaddr6: string }} target - * @returns {{ matched: "ip"|"asn-only"|"none", connection: object|null, vlan: object|null, asnEntries: Array<{v4: string, v6: string}> }} + * "none" is a positive claim of absence and the UI turns it into a + * "Remove netixlan entry" button, so it is only ever returned from an + * export we could actually read. Anything we could not interpret comes + * back as "unreadable" instead -- see the guard in the body. + * @returns {{ matched: "ip"|"asn-only"|"none"|"unreadable", reason: string, connection: object|null, vlan: object|null, asnEntries: Array<{v4: string, v6: string}> }} */ function extractIxfMatchForAsnIp(ixfData, { asn, ipaddr4, ipaddr6 } = {}) { const targetAsn = String(asn ?? "").trim(); @@ -1580,7 +1584,19 @@ const memberList = Array.isArray(ixfData?.member_list) ? ixfData.member_list : []; const asnEntries = []; - if (!targetAsn) return { matched: "none", connection: null, vlan: null, asnEntries }; + // A 200-OK body is not the same as a readable IX-F export. A CDN error + // page, a renamed schema, a feed that moved, or a URL pointing at another + // exchange all parse to an empty member_list -- that is silence, not + // evidence that the network is absent. Coercing it to [] and reporting + // "none" told the admin "IX-F has no entry for this ASN at all" and + // offered to delete the netixlan row on the strength of it. + if (!memberList.length) { + return { matched: "unreadable", reason: "no-member-list", connection: null, vlan: null, asnEntries }; + } + // Likewise, with no ASN to look for there is nothing to conclude. + if (!targetAsn) { + return { matched: "unreadable", reason: "no-target-asn", connection: null, vlan: null, asnEntries }; + } for (const member of memberList) { const memberAsn = String(member?.asnum ?? member?.asn ?? "").trim(); @@ -1598,13 +1614,13 @@ const v4Matches = !!targetV4 && v4 === targetV4; const v6Matches = !!targetV6Norm && !!v6 && normalizeIpv6ForCompareFp(v6) === targetV6Norm; if (v4Matches || v6Matches) { - return { matched: "ip", connection, vlan, asnEntries }; + return { matched: "ip", reason: "", connection, vlan, asnEntries }; } } } } - return { matched: asnEntries.length ? "asn-only" : "none", connection: null, vlan: null, asnEntries }; + return { matched: asnEntries.length ? "asn-only" : "none", reason: "", connection: null, vlan: null, asnEntries }; } /** @@ -2984,10 +3000,53 @@ } } + /** + * Renders the "we could not read this IX-F export" result. + * Purpose: Report that the check was inconclusive, distinctly from + * reporting that the network is genuinely absent from the feed. + * Necessity: Both used to render the same panel, so an unreadable export + * -- a CDN error page, a moved feed, a URL pointing at the wrong exchange + * -- presented as a confident negative alongside a "Remove netixlan + * entry" button. This state deliberately offers no destructive action; + * the admin's next step is to look at the feed, not to delete a row. + * @ai Preserve the absence of a remove/resolve control here. + * @param {HTMLElement} panel - Result panel for this netixlan row. + * @param {object} netixlanRow - Live netixlan record being checked. + * @param {string} reason - Machine reason from extractIxfMatchForAsnIp. + * @param {string} ixfUrl - Export URL that was fetched. + * @returns {void} + */ + function renderUnreadableIxfExportResult(panel, netixlanRow, reason, ixfUrl) { + panel.textContent = ""; + + const heading = document.createElement("div"); + heading.textContent = "IX-F check inconclusive"; + heading.style.fontWeight = "bold"; + panel.appendChild(heading); + + const detail = document.createElement("div"); + detail.textContent = reason === "no-target-asn" + ? `Netixlan #${netixlanRow.id ?? ""} has no ASN to look up, so the export could not be checked.` + : "The export was fetched but contains no readable member_list, so it cannot show whether " + + `AS${netixlanRow.asn} is present. This is not evidence that the entry should be removed.`; + panel.appendChild(detail); + + if (ixfUrl) { + const source = document.createElement("div"); + source.style.marginTop = "4px"; + source.textContent = `Source: ${ixfUrl}`; + panel.appendChild(source); + } + } + /** * Renders the "IX-F has no entry for this ASN at all" result, with a * "Remove netixlan entry" option (see removeNetixlanEntry for why this * is a native confirm() rather than a second custom button). + * Only reachable from a readable export -- extractIxfMatchForAsnIp + * returns "unreadable" rather than "none" when it could not interpret + * the feed, precisely so this destructive path cannot be reached on a + * false negative. * @ai Keep behavior stable and prefer minimal, localized edits. */ function renderNoIxfEntryResult(panel, netixlanId, netixlanRow) { @@ -3101,6 +3160,10 @@ ipaddr6: netixlanRow.ipaddr6, }); + if (matchResult.matched === "unreadable") { + renderUnreadableIxfExportResult(panel, netixlanRow, matchResult.reason, ixfUrl); + return; + } if (matchResult.matched === "none") { renderNoIxfEntryResult(panel, netixlanId, netixlanRow); return; diff --git a/user.js/tests/fp-netixlan-ixf-verify.test.js b/user.js/tests/fp-netixlan-ixf-verify.test.js index 04c1462..bbdab01 100644 --- a/user.js/tests/fp-netixlan-ixf-verify.test.js +++ b/user.js/tests/fp-netixlan-ixf-verify.test.js @@ -121,15 +121,42 @@ test('extractIxfMatchForAsnIp', async (t) => { assert.equal(result.asnEntries.length, 0); }); - await t.test('malformed/empty ixfData -> "none" without throwing', () => { - assert.equal(hooks.extractIxfMatchForAsnIp(null, { asn: 64500 }).matched, 'none'); - assert.equal(hooks.extractIxfMatchForAsnIp({}, { asn: 64500 }).matched, 'none'); - assert.equal(hooks.extractIxfMatchForAsnIp({ member_list: 'nope' }, { asn: 64500 }).matched, 'none'); + await t.test('an unreadable export is "unreadable", never "none"', () => { + // Regression: all of these used to coerce to [] and report "none", which + // the panel renders as "IX-F has no entry for this ASN at all" next to a + // "Remove netixlan entry" button. A 200-OK CDN error page, a renamed + // schema, or a URL pointing at another exchange is silence, not absence, + // and must never reach the destructive path. + for (const [label, body] of [ + ['null body', null], + ['empty object', {}], + ['non-IX-F JSON', { detail: 'Not Found' }], + ['member_list is not an array', { member_list: 'nope' }], + ['member_list present but empty', { member_list: [] }], + ]) { + const result = hooks.extractIxfMatchForAsnIp(body, { asn: 64500, ipaddr4: '1.2.3.4' }); + assert.equal(result.matched, 'unreadable', label); + assert.equal(result.reason, 'no-member-list', label); + assert.notEqual(result.matched, 'none', label); + } }); - await t.test('missing asn -> "none"', () => { + await t.test('missing asn -> "unreadable", not a claim of absence', () => { const ixfData = { member_list: [ixfMember({ asnum: 64500, vlans: [{ ipv4: { address: '1.2.3.4' }, ipv6: {} }] })] }; - assert.equal(hooks.extractIxfMatchForAsnIp(ixfData, { asn: '', ipaddr4: '1.2.3.4' }).matched, 'none'); + const result = hooks.extractIxfMatchForAsnIp(ixfData, { asn: '', ipaddr4: '1.2.3.4' }); + assert.equal(result.matched, 'unreadable'); + assert.equal(result.reason, 'no-target-asn'); + }); + + await t.test('a readable export that genuinely lacks the ASN is still "none"', () => { + // The other half of the fix: "none" must keep working where it is true, + // or the Remove path becomes unreachable and the guard is vacuous. + const ixfData = { + member_list: [ixfMember({ asnum: 64999, vlans: [{ ipv4: { address: '9.9.9.9' }, ipv6: {} }] })], + }; + const result = hooks.extractIxfMatchForAsnIp(ixfData, { asn: 64500, ipaddr4: '1.2.3.4' }); + assert.equal(result.matched, 'none'); + assert.equal(result.asnEntries.length, 0); }); }); @@ -205,9 +232,19 @@ test('buildIxfDiff', async (t) => { }); await t.test('returns [] when there is no IP match', () => { - const noMatch = hooks.extractIxfMatchForAsnIp({}, { asn: 64500 }); + const noMatch = hooks.extractIxfMatchForAsnIp( + { member_list: [ixfMember({ asnum: 64999, vlans: [{ ipv4: { address: '9.9.9.9' }, ipv6: {} }] })] }, + { asn: 64500 }, + ); + assert.equal(noMatch.matched, 'none'); assert.equal(hooks.buildIxfDiff({}, noMatch).length, 0); }); + + await t.test('returns [] for an unreadable export too', () => { + const unreadable = hooks.extractIxfMatchForAsnIp({}, { asn: 64500 }); + assert.equal(unreadable.matched, 'unreadable'); + assert.equal(hooks.buildIxfDiff({}, unreadable).length, 0); + }); }); test('buildNetixlanResolvePayload', async (t) => {