diff --git a/README.md b/README.md index 1a461e3..083f655 100644 --- a/README.md +++ b/README.md @@ -354,6 +354,7 @@ This is the current list: |   |:white_check_mark:| ST005 | Extract Variables Step with FormParam | A check for message content should be performed before policy execution. | |   |:white_check_mark:| ST006 | JSON Threat Protection Step | A check for message content should be performed before policy execution. | |   |:white_check_mark:| ST007 | XML Threat Protection Step | A check for message content should be performed before policy execution. | +|   |:white_check_mark:| ST008 | Unreachable policies | Policies should not be attached after RaiseFault policies. | | Policy |   |   |   |   | |   |:white_check_mark:| PO006 | Policy Name & filename agreement | Policy name attribute should coincide with the policy filename. | |   |:white_check_mark:| PO007 | Policy Naming Conventions - type indication | It is recommended that the policy name use a prefix or follow a pattern that indicates the policy type. | diff --git a/lib/package/Endpoint.js b/lib/package/Endpoint.js index 7afe25f..7ac13a6 100644 --- a/lib/package/Endpoint.js +++ b/lib/package/Endpoint.js @@ -250,7 +250,7 @@ Endpoint.prototype.getAllFlows = function () { } allFlows.push.apply(allFlows, this.getFlows()); - this.allFlows = allFlows; + this.allFlows = allFlows.filter(e => !!e); } debug("< getAllFlows()"); return this.allFlows; diff --git a/lib/package/Flow.js b/lib/package/Flow.js index fb7fd8f..1f27675 100644 --- a/lib/package/Flow.js +++ b/lib/package/Flow.js @@ -105,15 +105,17 @@ Flow.prototype.getFlowResponse = function () { const n = xpath.select("./Response", this.element); this.flowResponse = n && n[0] && new FlowPhase(n[0], this); } - debug(`< getFlowResponse`); + debug(`< getFlowResponse ${this.flowResponse}`); return this.flowResponse; }; Flow.prototype.getSharedFlow = function () { + debug(`> getSharedFlow: ${this.getFlowName()}`); if (this.sharedflow == null) { const n = xpath.select("/SharedFlow", this.element); this.sharedflow = n && n[0] && new FlowPhase(n[0], this); } + debug(`< getSharedFlow (${this.sharedFlow})`); return this.sharedflow; }; diff --git a/lib/package/plugins/ST008-unreachable-policies-after-raisefault.js b/lib/package/plugins/ST008-unreachable-policies-after-raisefault.js new file mode 100644 index 0000000..7800150 --- /dev/null +++ b/lib/package/plugins/ST008-unreachable-policies-after-raisefault.js @@ -0,0 +1,175 @@ +/* + 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. +*/ + +const ruleId = require("../lintUtil.js").getRuleId(), + debug = require("debug")("apigeelint:" + ruleId), + util = require("util"), + xpath = require("xpath"); + +const plugin = { + ruleId, + name: "Unreachable Steps after RaiseFault", + message: "Steps in the same flow after a RaiseFault cannot be reached.", + fatal: false, + severity: 1, //1=warning, 2=error + nodeType: "Step", + enabled: true, +}; + +let bundle = null; + +class PolicyFlowChecker { + constructor(plugin, debug) { + debug(`PolicyFlowChecker ctor`); + this.plugin = plugin; + this.debug = debug; + } + + checkSequence(stepParent, entity) { + let flagged = false; + const allSteps = stepParent.getSteps(); + // get the array of indexes + const isUnconditionalRaiseFaults = allSteps.map((step) => { + if (step.getCondition()) return false; + let referredPolicy = bundle + .getPolicies() + .find((p) => p.getSteps().find((s) => s == step)); + if (!referredPolicy || referredPolicy.getType() !== "RaiseFault") + return false; + let enabled = referredPolicy.select("@enabled"); + enabled = enabled && enabled[0]; + if (!enabled || enabled == "true") return true; + return false; + }); + + // just look at the FIRST one, skip all subsequent unconditional RF + isUnconditionalRaiseFaults.find((found, ix) => { + if (found) { + const step = allSteps[ix]; + debug( + `found an attached, unconditional RaiseFault policy, line ${step.getElement().lineNumber}`, + ); + if (ix < allSteps.length - 1) { + debug(`not last in flow`); + allSteps.slice(ix + 1).forEach((unreachableStep) => { + flagged = true; + entity.addMessage({ + plugin: checker.plugin, + source: unreachableStep.getElement().toString(), + line: unreachableStep.getElement().lineNumber, + column: unreachableStep.getElement().columnNumber, + message: `Step ${unreachableStep.getName()} is attached after a RaiseFault, cannot be reached.`, + }); + }); + } + return true; + } + return false; + }); + return flagged; + } + + check(sequence) { + let flagged = false; + try { + let debug = this.debug; + let bundlePolicies = bundle.getPolicies(); + let checker = this; + let sequenceType = sequence.constructor.name; + debug(`thingType: ${sequenceType}`); + if (sequenceType == "Flow") { + const flow = sequence; + debug(`flow.name: ${flow.name}`); + + ["FlowRequest", "FlowResponse", "SharedFlow"].forEach((m) => { + const method = `get${m}`; + debug(`calling flow.${method}()`); + const flowPhase = flow[method](); + if (flowPhase) { + if (this.checkSequence(flowPhase, flow)) { + flagged = true; + } + } + }); + } else { + if (this.checkSequence(sequence, sequence.getParent())) { + flagged = true; + } + } + } catch (exc1) { + flagged = true; + sequence.addMessage({ + plugin: checker.plugin, + message: exc1.message + exc1.stack, + }); + } + + return flagged; + } +} + +const checker = new PolicyFlowChecker(plugin, debug); + +const checkEndpoint = function (endpoint, cb) { + let flagged = false; + try { + endpoint.getAllFlows().forEach((sequence, ix) => { + if (checker.check(sequence)) { + flagged = true; + } + }); + endpoint.getFaultRules().forEach((sequence) => { + if (checker.check(sequence)) { + flagged = true; + } + }); + if (endpoint.getDefaultFaultRule()) { + if (checker.check(endpoint.getDefaultFaultRule())) { + flagged = true; + } + } + } catch (exc1) { + endpoint.addMessage({ + plugin: checker.plugin, + message: exc1.message + " " + exc1.stack, + }); + } + + if (typeof cb == "function") { + cb(null, flagged); + } +}; + +const onProxyEndpoint = function (endpoint, cb) { + debug("onProxyEndpoint"); + checkEndpoint(endpoint, cb); +}; + +const onTargetEndpoint = function (endpoint, cb) { + debug("onTargetEndpoint"); + checkEndpoint(endpoint, cb); +}; + +const onBundle = function (b, cb) { + bundle = b; +}; + +module.exports = { + plugin, + onBundle, + onProxyEndpoint, + onTargetEndpoint, +}; diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-CleanResponseHeaders.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-CleanResponseHeaders.xml new file mode 100644 index 0000000..dafd4ff --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-CleanResponseHeaders.xml @@ -0,0 +1,11 @@ + + + + + + false + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Extra-Header.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Extra-Header.xml new file mode 100644 index 0000000..3c6b827 --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Extra-Header.xml @@ -0,0 +1,7 @@ + + + +
{messageid}
+ + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-InjectProxyVersionHeader.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-InjectProxyVersionHeader.xml new file mode 100644 index 0000000..b87c5ed --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-InjectProxyVersionHeader.xml @@ -0,0 +1,7 @@ + + + +
{apiproxy.name} r{apiproxy.revision}
+ + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Response.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Response.xml new file mode 100644 index 0000000..8f54f54 --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Response.xml @@ -0,0 +1,9 @@ + + + +request verb: {request.verb} + + 200 + OK + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/JS-RemoveCopiedHeaders.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/JS-RemoveCopiedHeaders.xml new file mode 100644 index 0000000..736162d --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/JS-RemoveCopiedHeaders.xml @@ -0,0 +1,13 @@ + + + +var names = String(context.getVariable('request.headers.names')); +names = names.substring(1, names.length-1); +names.split(',').forEach(function(name) { + context.removeVariable('response.header.' + name.trim()); +}); + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error-Not-Enabled.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error-Not-Enabled.xml new file mode 100644 index 0000000..ee720b2 --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error-Not-Enabled.xml @@ -0,0 +1,9 @@ + + + + Error + 500 + + + true + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error.xml new file mode 100644 index 0000000..5996f80 --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error.xml @@ -0,0 +1,9 @@ + + + + Error + 500 + + + true + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Invalid-Request.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Invalid-Request.xml new file mode 100644 index 0000000..49a864a --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Invalid-Request.xml @@ -0,0 +1,10 @@ + + + + Bad Request + 400 + Bad Request + + + true + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/proxies/endpoint1.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/proxies/endpoint1.xml new file mode 100644 index 0000000..83641e2 --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/proxies/endpoint1.xml @@ -0,0 +1,98 @@ + + + /unreached-policies + + + + + AM-InjectProxyVersionHeader + + true + + + + + + RF-Error + + + + AM-CleanResponseHeaders + + + AM-Extra-Header + + conditional_statement + + + + + + + + + + AM-CleanResponseHeaders + + + JS-RemoveCopiedHeaders + + + AM-InjectProxyVersionHeader + + + + + + + a contrived response + + + + + proxy.pathsuffix MatchesPath "/f1" + + + + possibly a contrived response, possibly a fault + + + request.header.accept = "*/*" + RF-Invalid-Request + + + + AM-Extra-Header + + + + proxy.pathsuffix MatchesPath "/f2" + + + + unknown request - raise a fault + + + RF-Invalid-Request + + + + AM-Extra-Header + + + + + + + + + + + AM-Response + request.verb != "OPTIONS" + + + + + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/proxies/endpoint2.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/proxies/endpoint2.xml new file mode 100644 index 0000000..7912b5b --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/proxies/endpoint2.xml @@ -0,0 +1,98 @@ + + + /unreached-policies + + + + + AM-InjectProxyVersionHeader + + true + + + + + + RF-Error-Not-Enabled + + + + AM-CleanResponseHeaders + + + AM-Extra-Header + + conditional_statement + + + + + + + + + + AM-CleanResponseHeaders + + + JS-RemoveCopiedHeaders + + + AM-InjectProxyVersionHeader + + + + + + + a contrived response + + + + + proxy.pathsuffix MatchesPath "/f1" + + + + possibly a contrived response, possibly a fault + + + request.header.accept = "*/*" + RF-Invalid-Request + + + + AM-Extra-Header + + + + proxy.pathsuffix MatchesPath "/f2" + + + + unknown request - raise a fault + + + RF-Invalid-Request + + + + AM-Extra-Header + + + + + + + + + + + AM-Response + request.verb != "OPTIONS" + + + + + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/unreached-policies.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/unreached-policies.xml new file mode 100644 index 0000000..3e10127 --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/unreached-policies.xml @@ -0,0 +1,5 @@ + + 1621009025657 + 1621009262854 + /unreached-policies + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/Shared-Flow-1.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/Shared-Flow-1.xml new file mode 100644 index 0000000..577126f --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/Shared-Flow-1.xml @@ -0,0 +1,22 @@ + + + + 1486431487439 + dchiesa@google.com + + Shared-Flow-1 + 1486431673737 + dchiesa@google.com + + JavaScript-1 + + + jsc://JavaScript-1.js + + + SharedFlow + false + + default + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/AM-Header-1.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/AM-Header-1.xml new file mode 100644 index 0000000..3c36aba --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/AM-Header-1.xml @@ -0,0 +1,7 @@ + + + +
{messageid}
+ + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/AM-Header-2.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/AM-Header-2.xml new file mode 100644 index 0000000..e0f90fc --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/AM-Header-2.xml @@ -0,0 +1,7 @@ + + + +
{messageid}
+ + + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/RF-Unsupported.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/RF-Unsupported.xml new file mode 100644 index 0000000..f64a140 --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/policies/RF-Unsupported.xml @@ -0,0 +1,10 @@ + + + + Bad Request + 400 + Bad Request + + + true + diff --git a/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/sharedflows/sf-default.xml b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/sharedflows/sf-default.xml new file mode 100644 index 0000000..a3d3f31 --- /dev/null +++ b/test/fixtures/resources/ST008-unreached-policies-after-raisefault/sharedflowbundle/sharedflows/sf-default.xml @@ -0,0 +1,13 @@ + + + + AM-Header-1 + + + RF-Unsupported + + + AM-Header-2 + + + diff --git a/test/specs/ST008-test.js b/test/specs/ST008-test.js new file mode 100644 index 0000000..88d1295 --- /dev/null +++ b/test/specs/ST008-test.js @@ -0,0 +1,104 @@ +/* + 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 */ + +const assert = require("assert"), + ruleId = 'ST008', + path = require("path"), + debug = require("debug")(`apigeelint:${ruleId}-test`), + util = require("util"), + bl = require("../../lib/package/bundleLinter.js"); + +const expectedMessageRe = + new RegExp("^Step [-A-Za-z0-9_]{2,} is attached after a RaiseFault,.+$"); + + +describe(`${ruleId} - unreachable policies`, () => { + function check(fileBasename, bundleType, expected) { + let configuration = { + debug: true, + source: { + type: "filesystem", + path: path.resolve(__dirname, `../fixtures/resources/ST008-unreached-policies-after-raisefault/${bundleType}`), + bundleType + }, + excluded: {}, + setExitCode: false, + output: () => {} // suppress output + }; + + bl.lint(configuration, (bundle) => { + let items = bundle.getReport(); + assert.ok(items); + assert.ok(items.length); + debug(util.format(items)); + items = items.filter( m => m.filePath.endsWith(fileBasename)); + debug(util.format(items)); + assert.equal(items.length, 1, `expected:1`); + items.forEach( item => + debug(util.format(item.messages))); + let st008Messages = items[0].messages.filter( m => m.ruleId == ruleId); + debug(util.format(st008Messages)); + assert.equal(st008Messages.length, expected.length); + + expected.forEach( (item, ix) => { + assert.equal(st008Messages[ix].line, item.line, `case(${ix}) line`); + assert.equal(st008Messages[ix].column, item.column, `case(${ix}) column`); + assert.equal(st008Messages[ix].severity, 1, `case(${ix}) severity`); + assert.ok(st008Messages[ix].message.match(expectedMessageRe), `case(${ix}) message ${st008Messages[ix].message}`); + }); + }); + } + + it('should find the expected warnings in an apiproxy', () => { + let expected = [ + { + line: 19, + column: 7 + }, + { + line: 22, + column: 7 + }, + { + line: 79, + column: 9 + } + ]; + + check('endpoint1.xml', 'apiproxy', expected); + }); + + it('should not find warnings when the RaiseFault policy is not enabled', () => { + let expected = [ + { + line: 79, + column: 9 + } + ]; + + check('endpoint2.xml', 'apiproxy', expected); + }); + + it('should find the expected warnings in a sharedflow', () => { + let expected = [ + { line: 9, column: 3 } + ]; + check('sf-default.xml', 'sharedflowbundle', expected); + }); + +});