feat: check for policies attached after RaiseFault

apigee · Dec 6, 2024 · 8bd4ce8 · 8bd4ce8
1 parent 12e2bdb
commit 8bd4ce8
Show file tree

Hide file tree

Showing 21 changed files with 619 additions and 2 deletions.
diff --git a/README.md b/README.md
@@ -354,6 +354,7 @@ This is the current list:
 | &nbsp; |:white_check_mark:| ST005 | Extract Variables Step with FormParam | A check for message content should be performed before policy execution. |
 | &nbsp; |:white_check_mark:| ST006 | JSON Threat Protection Step | A check for message content should be performed before policy execution. |
 | &nbsp; |:white_check_mark:| ST007 | XML Threat Protection Step | A check for message content should be performed before policy execution. |
+| &nbsp; |:white_check_mark:| ST008 | Unreachable policies | Policies should not be attached after RaiseFault policies. |
 | Policy | &nbsp; | &nbsp; | &nbsp; | &nbsp; |
 | &nbsp; |:white_check_mark:| PO006 | Policy Name &amp; filename agreement |  Policy name attribute should coincide with the policy filename. |
 | &nbsp; |: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
@@ -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
@@ -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
@@ -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/...s/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-CleanResponseHeaders.xml b/...s/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-CleanResponseHeaders.xml
@@ -0,0 +1,11 @@
+<AssignMessage name='AM-CleanResponseHeaders' enabled='false'>
+  <!--
+       This policy is not enabled.  Using it with CORS on a loopback
+       policy, even as the first policy in the response preflow, will
+       erase the response headers that the cORS policy sets.
+  -->
+  <Remove>
+    <Headers/>
+  </Remove>
+  <IgnoreUnresolvedVariables>false</IgnoreUnresolvedVariables>
+</AssignMessage>
diff --git a/...resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Extra-Header.xml b/...resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Extra-Header.xml
@@ -0,0 +1,7 @@
+<AssignMessage name="AM-Extra-Header">
+  <Set>
+    <Headers>
+      <Header name='extra'>{messageid}</Header>
+    </Headers>
+  </Set>
+</AssignMessage>
diff --git a/...008-unreached-policies-after-raisefault/apiproxy/policies/AM-InjectProxyVersionHeader.xml b/...008-unreached-policies-after-raisefault/apiproxy/policies/AM-InjectProxyVersionHeader.xml
@@ -0,0 +1,7 @@
+<AssignMessage name="AM-InjectProxyVersionHeader">
+  <Set>
+    <Headers>
+      <Header name="APIProxy">{apiproxy.name} r{apiproxy.revision}</Header>
+    </Headers>
+  </Set>
+</AssignMessage>
diff --git a/...res/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Response.xml b/...res/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/AM-Response.xml
@@ -0,0 +1,9 @@
+<AssignMessage name="AM-Response">
+  <Set>
+    <Payload contentType="text/plain">
+request verb: {request.verb}
+</Payload>
+    <StatusCode>200</StatusCode>
+    <ReasonPhrase>OK</ReasonPhrase>
+  </Set>
+</AssignMessage>
diff --git a/...es/ST008-unreached-policies-after-raisefault/apiproxy/policies/JS-RemoveCopiedHeaders.xml b/...es/ST008-unreached-policies-after-raisefault/apiproxy/policies/JS-RemoveCopiedHeaders.xml
@@ -0,0 +1,13 @@
+<Javascript name="JS-RemoveCopiedHeaders" enabled='true'>
+  <!--
+       This policy removes the request headers that Apigee
+       implicitly copies to the response, on a oopback proxy.
+  -->
+  <Source>
+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());
+});
+  </Source>
+</Javascript>
diff --git a/...rces/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error-Not-Enabled.xml b/...rces/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error-Not-Enabled.xml
@@ -0,0 +1,9 @@
+<RaiseFault enabled="false" name="RF-Error-Not-Enabled">
+  <FaultResponse>
+    <Set>
+      <Payload contentType="text/plain">Error</Payload>
+      <StatusCode>500</StatusCode>
+    </Set>
+  </FaultResponse>
+  <IgnoreUnresolvedVariables>true</IgnoreUnresolvedVariables>
+</RaiseFault>
diff --git a/...xtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error.xml b/...xtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Error.xml
@@ -0,0 +1,9 @@
+<RaiseFault name="RF-Error">
+  <FaultResponse>
+    <Set>
+      <Payload contentType="text/plain">Error</Payload>
+      <StatusCode>500</StatusCode>
+    </Set>
+  </FaultResponse>
+  <IgnoreUnresolvedVariables>true</IgnoreUnresolvedVariables>
+</RaiseFault>
diff --git a/...ources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Invalid-Request.xml b/...ources/ST008-unreached-policies-after-raisefault/apiproxy/policies/RF-Invalid-Request.xml
@@ -0,0 +1,10 @@
+<RaiseFault name="RF-Invalid-Request">
+  <FaultResponse>
+    <Set>
+      <Payload contentType="text/plain">Bad Request</Payload>
+      <StatusCode>400</StatusCode>
+      <ReasonPhrase>Bad Request</ReasonPhrase>
+    </Set>
+  </FaultResponse>
+  <IgnoreUnresolvedVariables>true</IgnoreUnresolvedVariables>
+</RaiseFault>
diff --git a/...xtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/proxies/endpoint1.xml b/...xtures/resources/ST008-unreached-policies-after-raisefault/apiproxy/proxies/endpoint1.xml
@@ -0,0 +1,98 @@
+<ProxyEndpoint name="endpoint1">
+  <HTTPProxyConnection>
+    <BasePath>/unreached-policies</BasePath>
+  </HTTPProxyConnection>
+
+  <DefaultFaultRule name="default-fault-rule">
+    <Step>
+      <Name>AM-InjectProxyVersionHeader</Name>
+    </Step>
+    <AlwaysEnforce>true</AlwaysEnforce>
+  </DefaultFaultRule>
+
+  <FaultRules>
+    <FaultRule name='rule1'>
+      <Step>
+        <Name>RF-Error</Name>
+      </Step>
+      <!-- error -->
+      <Step>
+        <Name>AM-CleanResponseHeaders</Name>
+      </Step>
+      <Step>
+        <Name>AM-Extra-Header</Name>
+      </Step>
+      <Condition>conditional_statement</Condition>
+    </FaultRule>
+  </FaultRules>
+
+
+  <PreFlow name="PreFlow">
+    <Request>
+    </Request>
+    <Response>
+      <Step>
+        <Name>AM-CleanResponseHeaders</Name>
+      </Step>
+      <Step>
+        <Name>JS-RemoveCopiedHeaders</Name>
+      </Step>
+      <Step>
+        <Name>AM-InjectProxyVersionHeader</Name>
+      </Step>
+    </Response>
+  </PreFlow>
+
+  <Flows>
+    <Flow name="f1">
+      <Description>a contrived response</Description>
+      <Request>
+      </Request>
+      <Response>
+      </Response>
+      <Condition>proxy.pathsuffix MatchesPath "/f1"</Condition>
+    </Flow>
+
+    <Flow name="f2">
+      <Description>possibly a contrived response, possibly a fault</Description>
+      <Request>
+        <Step>
+          <Condition>request.header.accept = "*/*"</Condition>
+          <Name>RF-Invalid-Request</Name>
+        </Step>
+        <!-- not an error -->
+        <Step>
+          <Name>AM-Extra-Header</Name>
+        </Step>
+      </Request>
+      <Response/>
+      <Condition>proxy.pathsuffix MatchesPath "/f2"</Condition>
+    </Flow>
+
+    <Flow name="f3">
+      <Description>unknown request - raise a fault</Description>
+      <Request>
+        <Step>
+          <Name>RF-Invalid-Request</Name>
+        </Step>
+        <!-- error: unreachable -->
+        <Step>
+          <Name>AM-Extra-Header</Name>
+        </Step>
+      </Request>
+      <Response/>
+    </Flow>
+  </Flows>
+
+  <PostFlow name="PostFlow">
+    <Request/>
+    <Response>
+      <Step>
+        <Name>AM-Response</Name>
+        <Condition>request.verb != "OPTIONS"</Condition>
+      </Step>
+    </Response>
+  </PostFlow>
+
+  <RouteRule name="noroute"/>
+</ProxyEndpoint>