From 1901d3e659baac46fc045e9064193e5e4a002f86 Mon Sep 17 00:00:00 2001 From: Dino Chiesa Date: Wed, 18 Sep 2024 22:51:24 -0700 Subject: [PATCH] feat: BN014 duplicate policy checker --- .../plugins/BN014-duplicate-policies.js | 254 ++++++++++++++++++ .../resources/BN014/cors-test/README.md | 7 + .../BN014/cors-test/apiproxy/cors-test.xml | 10 + ...AM-Clean-Request-Headers-From-Response.xml | 21 ++ .../AM-Inject-Proxy-Revision-Header.xml | 7 + .../apiproxy/policies/AM-Response-2.xml | 21 ++ .../apiproxy/policies/AM-Response-3.xml | 19 ++ .../apiproxy/policies/AM-Response.xml | 12 + .../cors-test/apiproxy/policies/CORS-1.xml | 10 + .../cors-test/apiproxy/policies/CORS-A.xml | 21 ++ .../apiproxy/policies/RF-Unknown-Request.xml | 16 ++ .../apiproxy/policies/VerifyAPIKey-1.xml | 3 + .../cors-test/apiproxy/proxies/endpoint1.xml | 74 +++++ test/specs/BN014-duplicate-policies.js | 92 +++++++ test/specs/PO037-DataCapture-hygiene.js | 2 +- 15 files changed, 568 insertions(+), 1 deletion(-) create mode 100644 lib/package/plugins/BN014-duplicate-policies.js create mode 100644 test/fixtures/resources/BN014/cors-test/README.md create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/cors-test.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Clean-Request-Headers-From-Response.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Inject-Proxy-Revision-Header.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response-2.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response-3.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/CORS-1.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/CORS-A.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/RF-Unknown-Request.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/policies/VerifyAPIKey-1.xml create mode 100644 test/fixtures/resources/BN014/cors-test/apiproxy/proxies/endpoint1.xml create mode 100644 test/specs/BN014-duplicate-policies.js diff --git a/lib/package/plugins/BN014-duplicate-policies.js b/lib/package/plugins/BN014-duplicate-policies.js new file mode 100644 index 00000000..bb02967a --- /dev/null +++ b/lib/package/plugins/BN014-duplicate-policies.js @@ -0,0 +1,254 @@ +/* + Copyright 2019-2020,2024 Google LLC + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +const ruleId = require("../myUtil.js").getRuleId(), + debug = require("debug")("apigeelint:" + ruleId), + xpath = require("xpath"); + +const ELEMENT_NODE = 1; +//const ATTRIBUTE_NODE = 2; +const TEXT_NODE = 3; +const CDATA_SECTION_NODE = 4; + +const plugin = { + ruleId, + name: "Check for duplicate policies", + message: "Multiple identically configured policies.", + fatal: false, + severity: 1, // 1=warning, 2=error + nodeType: "Endpoint", + enabled: true +}; + +const onBundle = function (bundle, cb) { + debug("onBundle"); + const flagged = check(bundle); + if (typeof cb == "function") { + cb(null, flagged); + } +}; + +const _markPolicy = (policy, msg) => + policy.addMessage({ + ruleId: plugin.ruleId, + severity: plugin.severity, + source: policy.getSource(), + line: policy.getElement().lineNumber, + column: policy.getElement().columnNumber, + nodeType: "Policy", + message: msg + }); + +/* + * Recursive fn that returns true if XML elements are different. Ignores any intervening whitespace or indents. + * + **/ +const _diffXmlNode = (isTopLevel) => (indent, elt1, elt2) => { + debug(`${indent}elt1: TYPE(${elt1.nodeType}) tag(${elt1.tagName})`); + debug(`${indent}elt2: TYPE(${elt2.nodeType}) tag(${elt2.tagName})`); + // verify same node type + if (elt1.nodeType != elt2.nodeType) { + debug(`${indent}different nodeType`); + return true; + } + + if (elt1.nodeType == TEXT_NODE) { + debug( + `${indent}test '${elt1.nodeValue.trim()}' ==? '${elt2.nodeValue.trim()}'` + ); + return elt1.nodeValue.trim() != elt2.nodeValue.trim(); + } + + if (elt1.nodeType == CDATA_SECTION_NODE) { + debug(`${indent}test '${elt1.data}' ==? '${elt2.data}'`); + return elt1.data != elt2.data; + } + + if (elt1.nodeType == ELEMENT_NODE) { + // compare tagname on element nodes + debug(`${indent}test '${elt1.tagName}' ==? '${elt2.tagName}'`); + if (elt1.tagName != elt2.tagName) { + return true; + } + + // compare attrs, maybe excepting name + const attrsToCompare = (elt) => { + let attrs = xpath.select("@*", elt); + if (isTopLevel) { + attrs = attrs.filter((attr) => attr.name != "name"); + } + return attrs; + }; + + // compare attrs without respect to ordering + const attrs1 = attrsToCompare(elt1); + const attrs2 = attrsToCompare(elt2); + let diff = + attrs1.length != attrs2.length || + !!attrs1.find( + (attr1, _i) => + !attrs2.find( + (attr2) => attr2.name == attr1.name && attr2.value == attr1.value + ) + ); + + // compare child nodes, respecting ordering + if (!diff) { + const childrenToCompare = (elt) => + xpath + .select("node()|text()", elt) + .filter( + (node) => + node.nodeType == ELEMENT_NODE || + node.nodeType == TEXT_NODE || + node.nodeType == CDATA_SECTION_NODE + ); + const children1 = childrenToCompare(elt1); + const children2 = childrenToCompare(elt2); + + if ( + children1.length == children2.length && + children2.length == 1 && + children2[0].nodeType == TEXT_NODE + ) { + diff = children1[0].nodeValue.trim() != children2[0].nodeValue.trim(); + } else { + debug(`${indent}recurse`); + /* + * It is too simplistic to use the lengths of the node sets as a basis of + * difference. If there is a comment in one of the documents, and not in the other, + * it results in multiple text nodes in one of the nodesets and just one in the + * other. Instead, we effectively ignore toplevel text nodes in either + * document. This is ok, as there are no cases in which Apigee uses complex XML in + * which a child nodeset includes both TEXT nodes and elements. In other words, in + * all valid Apigee configuration, if the node is an element, it either has a single + * TEXT child, or it has one or more element children. Never both. + **/ + const reducer = (a) => (accumulator, elt, _index) => { + // if they are already different, skip all further checks + if (!accumulator.different) { + // skip all TEXT nodes in children1 + if (elt.nodeType != TEXT_NODE) { + // likewise, skip all successive TEXT nodes in children2 + let ix2 = accumulator.ix2; + while (a[ix2] && a[ix2].nodeType == TEXT_NODE) { + ix2++; + } + const elt2 = a[ix2]; + debug(`${indent} elt2: ${elt2}`); + accumulator.different = + !elt2 || _diffXmlNode(false)(indent + " ", elt, elt2); + accumulator.ix2 = ++ix2; + } + } + return accumulator; + }; + + const result = children1.reduce(reducer(children2), { + ix2: 0, + different: false + }); + diff = result.different; + } + } + return diff; + } + throw new Error("unhandled node type"); +}; + +/* + * Returns true if XML elements are different. This ignores the toplevel name attribute, + * and any intervening whitespace or indents. + * + **/ +const diffXml = (elt1, elt2) => _diffXmlNode(true)("", elt1, elt2); + +const _checkForDuplicatePolicies = (policies) => { + const xpath = "/*/*"; + let flagged = false; + /** + * Check each policy for duplicates. + * + * This sort of works, but it's naive in that it compares the XML + * configuration directly, including whitespace. A better comparison would + * ignore whitespace and just compare the digest of the XML. That is an + * unrealizable dream as the digest is determined by how Apigee interprets + * the XML. We might be able to get close by comparing the XML infoset. + * + **/ + const previouslyDetected = []; + + policies.slice(0, -1).forEach((policy1, i, a) => { + if (!previouslyDetected.includes(i)) { + try { + //const p1 = policy1.select(xpath).toString().trim(); + debug( + `looking at index ${i}: type(${policy1.getType()}) name(${policy1.getName()})` + ); + const dupesForI = []; + a.slice(i + 1).forEach((c, j) => { + // debug(` comparing to ${c}...`); + if (!diffXml(policy1.getElement(), c.getElement())) { + const actualIndex = j + i + 1; + debug(` duplicate found at index ${actualIndex}`); + dupesForI.push(actualIndex); + } + }); + + if (dupesForI.length) { + debug(` dupes found for ${i}: ${dupesForI}`); + dupesForI.forEach((ix) => + _markPolicy( + policies[ix], + `Policy ${policies[ix].getName()} is a duplicate of Policy ${policies[i].getName()}. Eliminate duplicates and attach a single policy in multiple places.` + ) + ); + flagged = true; + previouslyDetected.push(...dupesForI); + debug(`\n dupes found so far: ${previouslyDetected}`); + } + } catch (e) { + console.log(e.stack); + _markPolicy( + policy1, + `Error processing Policy ${policy1.getName()}: ${e.message}.` + ); + flagged = true; + } + } + }); + return flagged; +}; + +const check = (bundle) => { + let flagged = false; + if (bundle.policies) { + debug("number of policies: " + bundle.policies.length); + if (bundle.policies.length > 1) { + flagged = _checkForDuplicatePolicies( + bundle.policies.toSorted((a, b) => + a.getName().localeCompare(b.getName()) + ) + ); + } + } + return flagged; +}; + +module.exports = { + plugin, + onBundle +}; diff --git a/test/fixtures/resources/BN014/cors-test/README.md b/test/fixtures/resources/BN014/cors-test/README.md new file mode 100644 index 00000000..328df9b1 --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/README.md @@ -0,0 +1,7 @@ +20240424-1159 + +A loopback proxy to test CORS policy and DefaultFaultRule + +https://www.googlecloudcommunity.com/gc/Apigee/Default-Fault-Rule-not-adding-CORS-headers-in-Raise-Fault/m-p/742799#M79285 + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/cors-test.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/cors-test.xml new file mode 100644 index 00000000..6dd4a574 --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/cors-test.xml @@ -0,0 +1,10 @@ + + + 1489339923158 + dino + + cors-test + 1489340418969 + orgAdmin + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Clean-Request-Headers-From-Response.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Clean-Request-Headers-From-Response.xml new file mode 100644 index 00000000..0705cbde --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Clean-Request-Headers-From-Response.xml @@ -0,0 +1,21 @@ + + + +
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+ + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Inject-Proxy-Revision-Header.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Inject-Proxy-Revision-Header.xml new file mode 100644 index 00000000..fcf6c0ae --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Inject-Proxy-Revision-Header.xml @@ -0,0 +1,7 @@ + + + +
{apiproxy.name} r{apiproxy.revision}
+ + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response-2.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response-2.xml new file mode 100644 index 00000000..072d4f27 --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response-2.xml @@ -0,0 +1,21 @@ + + true + + + { + "status" : "ok" +} + + OK + 200 + + + response + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response-3.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response-3.xml new file mode 100644 index 00000000..9fa0aae3 --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response-3.xml @@ -0,0 +1,19 @@ + + true + + + { + "status" : "ok" +} + + + NOTOK + 200 + + + response + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response.xml new file mode 100644 index 00000000..f976727b --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/AM-Response.xml @@ -0,0 +1,12 @@ + + true + + { + "status" : "ok" +} + + OK + 200 + + response + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/CORS-1.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/CORS-1.xml new file mode 100644 index 00000000..ca18cfa0 --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/CORS-1.xml @@ -0,0 +1,10 @@ + + {request.header.origin:*} + GET, PUT, POST, DELETE, OPTIONS + origin, x-requested-with + * + 1800 + true + true + true + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/CORS-A.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/CORS-A.xml new file mode 100644 index 00000000..59ba682e --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/CORS-A.xml @@ -0,0 +1,21 @@ + + + + + {request.header.origin:*} + + GET, PUT, POST, DELETE, OPTIONS + + origin, x-requested-with + + * + + 1800 + + true + + true + + true + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/RF-Unknown-Request.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/RF-Unknown-Request.xml new file mode 100644 index 00000000..9f5f7fe1 --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/RF-Unknown-Request.xml @@ -0,0 +1,16 @@ + + true + + + { + "error" : { + "code" : 404.01, + "message" : "that request was unknown; try a different request." + } +} + + 404 + Not Found + + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/policies/VerifyAPIKey-1.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/VerifyAPIKey-1.xml new file mode 100644 index 00000000..f48908ec --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/policies/VerifyAPIKey-1.xml @@ -0,0 +1,3 @@ + + + diff --git a/test/fixtures/resources/BN014/cors-test/apiproxy/proxies/endpoint1.xml b/test/fixtures/resources/BN014/cors-test/apiproxy/proxies/endpoint1.xml new file mode 100644 index 00000000..1f8f44b8 --- /dev/null +++ b/test/fixtures/resources/BN014/cors-test/apiproxy/proxies/endpoint1.xml @@ -0,0 +1,74 @@ + + Proxy Endpoint 1 + + /cors-loopback + + + + + + AM-Inject-Proxy-Revision-Header + + true + + + + + + CORS-1 + + + CORS-A + + + + + AM-Clean-Request-Headers-From-Response + + + + + + + + + + AM-Inject-Proxy-Revision-Header + + + AM-Response + + + + + + + + + + + + + + + + VerifyAPIKey-1 + + + + (proxy.pathsuffix MatchesPath "/t1") and (request.verb = "GET") + + + + + + RF-Unknown-Request + + + + + + + + + diff --git a/test/specs/BN014-duplicate-policies.js b/test/specs/BN014-duplicate-policies.js new file mode 100644 index 00000000..85c24c0c --- /dev/null +++ b/test/specs/BN014-duplicate-policies.js @@ -0,0 +1,92 @@ +/* + Copyright 2019-2024 Google LLC + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +/* global describe, it, __dirname */ + +const assert = require("assert"), + path = require("path"), + util = require("util"), + ruleId = "BN014", + debug = require("debug")("apigeelint:" + ruleId), + bl = require("../../lib/package/bundleLinter.js"); + +const expectedErrors = { + "CORS-A.xml": [ + "Policy CORS-A is a duplicate of Policy CORS-1. Eliminate duplicates and attach a single policy in multiple places." + ], + "AM-Response-2.xml": [ + "Policy AM-Response-2 is a duplicate of Policy AM-Response. Eliminate duplicates and attach a single policy in multiple places." + ] +}; + +describe(`BN014 - Duplicate policies`, () => { + const configuration = { + debug: true, + source: { + type: "filesystem", + path: path.resolve( + __dirname, + "../fixtures/resources/BN014/cors-test", + "apiproxy" + ), + bundleType: "apiproxy", + profile: "apigeex" + }, + excluded: {}, + setExitCode: false, + output: () => {} // suppress output + }; + + debug(`BN014 configuration: ${util.format(configuration)}`); + bl.lint(configuration, (bundle) => { + const items = bundle.getReport(); + assert.ok(items); + assert.ok(items.length); + const bn014Items = items.filter((item) => + item.messages.some((m) => m.ruleId == "BN014") + ); + it(`should generate the expected number of errors`, () => { + debug(`bn014Items: ${util.format(bn014Items.map((i) => i.filePath))}`); + + assert.equal(bn014Items.length, Object.keys(expectedErrors).length); + }); + + Object.keys(expectedErrors).forEach((policyName, caseNum) => { + it(`should generate the expected errors for ${policyName}`, () => { + debug(`policyName: ${policyName}`); + const expected = expectedErrors[policyName]; + const policyItems = bn014Items.filter((item) => + item.filePath.endsWith(policyName) + ); + debug(`policyItems: ${util.format(policyItems)}`); + + assert.equal(policyItems.length, 1); + const bn014Messages = policyItems[0].messages.filter( + (m) => m.ruleId == "BN014" + ); + debug(`po035Messages: ${util.format(bn014Messages)}`); + assert.equal(bn014Messages.length, expected.length); + assert.equal(bn014Messages.length, 1); + + assert.equal( + bn014Messages[0].message, + expected[0], + `${policyName} case(${caseNum})` + ); + }); + }); + }); +}); diff --git a/test/specs/PO037-DataCapture-hygiene.js b/test/specs/PO037-DataCapture-hygiene.js index ee8bb6f2..f8b7149f 100644 --- a/test/specs/PO037-DataCapture-hygiene.js +++ b/test/specs/PO037-DataCapture-hygiene.js @@ -146,7 +146,7 @@ describe(`PO037 - DataCapture Source usage`, () => { it(`should generate no errors other than PO037`, () => { const nonPO37Items = items.filter((item) => - item.messages.some((m) => m.ruleId != "PO037") + item.messages.some((m) => m.ruleId != "PO037" && m.ruleId != "BN014") ); debug( `nonPO37Items: ${util.format(nonPO37Items.map((i) => i.filePath))}`