Apply suggestions from code review

Co-authored-by: Chris Smowton <smowton@github.com>

ql/src/Security/CWE-020/ExternalAPISinkExample.go

@@ -13,7 +13,7 @@ func serve() {
 			// BAD: a request parameter is incorporated without validation into the response
 			fmt.Fprintf(w, "%q is an unknown user", username)
 		} else {
-			// TODO: do something exciting
+			// TODO: Handle successful login
 		}
 	})
 	http.ListenAndServe(":80", nil)

ql/src/Security/CWE-020/ExternalAPIsUsedWithUntrustedData.qhelp

@@ -9,9 +9,9 @@ unique sources of untrusted data flow to this API. This query is designed primar
 may be relevant for security analysis of this application.</p>
 
 <p>An external API is defined as a call to a function that is not defined in the source code and is not
-modeled as a taint step in the default taint library. Calls made in test files are also excluded.
+modeled as a taint step in the default taint library. Calls made in test files are excluded.
 External APIs may be from the  Go standard library, third party dependencies or from internal dependencies.
-The query will report the fully qualified method name, along with either <code>[param x]</code>,
+The query will report the fully-qualified method name, along with either <code>[param x]</code>,
 where <code>x</code> indicates the position of the parameter receiving the untrusted data or <code>[receiver]</code>
 indicating the untrusted data is used as the receiver of the method call.</p>
 
@@ -23,7 +23,7 @@ indicating the untrusted data is used as the receiver of the method call.</p>
 <ul>
   <li>If the result highlights a known sink, no action is required.</li>
   <li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query.</li>
-  <li>If the result represents a call to an external API which transfers taint, add the appropriate modeling, and 
+  <li>If the result represents a call to an external API which transfers taint (for example, from a parameter to its return value), add the appropriate modeling, and 
   re-run the query to determine what new results have appeared due to this additional modeling.</li>
 </ul>
 
@@ -33,10 +33,14 @@ class to exclude known safe external APIs from future analysis.</p>
 </recommendation>
 <example>
 
+<sample src="ExternalAPISinkExample.go" />
+
 <p>If the query were to return the API <code>fmt.Fprintf [param 2]</code> then we should first consider
 whether this a security relevant sink. In this case, this is writing to a HTTP response, so we should
 consider whether this is an XSS sink. If it is, we should confirm that it is handled by the XSS query.</p>
 
+<sample src="ExternalAPITaintStepExample.go" />
+
 <p>If the query were to return the API <code>fmt.Sprintf [param 1]</code>, then this should be
 reviewed as a possible taint step, because tainted data would flow from the 1st argument to the return value
 of the call.</p>

ql/src/Security/CWE-020/UntrustedDataToExternalAPI.qhelp

@@ -5,11 +5,11 @@
 <overview>
 <p>Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports 
 external APIs that use untrusted data. The results have very little filtering so that you can audit almost all
-examples. The query provides data for security reviews of the application and you can also use it to identify
+examples. The query provides data for security reviews of the application. It can also be used to identify
 external APIs that should be modeled as either taint steps, or sinks for specific problems.</p>
 
 <p>An external API is defined as a call to a function that is not defined in the source code and is not
-modeled as a taint step in the default taint library. Calls made in test files are also excluded.
+modeled as a taint step in the default taint library. Calls made in test files are excluded.
 External APIs may be from the Go standard library, third-party dependencies or from internal dependencies.
 The query reports uses of untrusted data in either the receiver or as one of the arguments of external APIs.</p>
 
@@ -23,7 +23,7 @@ The query reports uses of untrusted data in either the receiver or as one of the
   that the result is a false positive because this data is sanitized.</li>
   <li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query, 
   and confirm that the result is either found, or is safe due to appropriate sanitization.</li>
-  <li>If the result represents a call to an external API that transfers taint, add the appropriate modeling, and 
+  <li>If the result represents a call to an external API that transfers taint (for example, from a parameter to its return value), add the appropriate modeling, and 
   re-run the query to determine what new results have appeared due to this additional modeling.</li>
 </ul>
 

ql/src/Security/CWE-020/UntrustedDataToUnknownExternalAPI.qhelp

@@ -5,11 +5,11 @@
 <overview>
 <p>Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports
 external APIs that use untrusted data. The results have been filtered. The query provides data for security
-reviews of the application and you can also use it to identify external APIs that should be modeled as either
+reviews of the application. It can also be used to identify external APIs that should be modeled as either
 taint steps, or sinks for specific problems.</p>
 
 <p>An external API is defined as a call to a function that is not defined in the source code and is not
-modeled as a taint step in the default taint library. Calls made in test files are also excluded.
+modeled as a taint step in the default taint library. Calls made in test files are excluded.
 External APIs may be from the Go standard library, third-party dependencies or from internal dependencies.
 The query reports uses of untrusted data in either the receiver or as one of the arguments of external APIs.</p>
 
@@ -24,7 +24,7 @@ to not be a possible source of security vulnerabilities.</p>
 <ul>
   <li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query,
   and confirm that the result is either found, or is safe due to appropriate sanitization.</li>
-  <li>If the result represents a call to an external API that transfers taint, add the appropriate modeling, and
+  <li>If the result represents a call to an external API that transfers taint (for example, from a parameter to its return value), add the appropriate modeling, and
   re-run the query to determine what new results have appeared due to this additional modeling.</li>
 </ul>
 

ql/src/Security/CWE-079/ReflectedXss.go

@@ -13,7 +13,7 @@ func serve() {
 			// BAD: a request parameter is incorporated without validation into the response
 			fmt.Fprintf(w, "%q is an unknown user", username)
 		} else {
-			// TODO: do something exciting
+			// TODO: Handle successful login
 		}
 	})
 	http.ListenAndServe(":80", nil)

ql/src/semmle/go/security/ExternalAPIs.qll

@@ -74,7 +74,7 @@ class ExternalAPIDataNode extends DataFlow::Node {
     ) and
     // Not defined in the code that is being analysed
     not exists(call.getACallee().getBody()) and
-    // Not a function pointer, unless it's declared in a package
+    // Not a function pointer, unless it's declared at package scope
     not isProbableLocalFunctionPointer(call) and
     // Not defined in a test file
     not call.getFile() instanceof TestFile and
@@ -113,6 +113,7 @@ class ExternalAPIDataNode extends DataFlow::Node {
 TaintTracking::FunctionModel getAMethodModelInPackage(Package p) {
   p = result.getPackage() and
   result instanceof Method and
+  // We model any method of the form "String() string"
   result.getName() != "String" and
   not exists(TaintTracking::FunctionModel baseMethod |
     baseMethod != result and result.(Method).implements(baseMethod)
@@ -144,7 +145,7 @@ predicate isACommonSink(DataFlow::Node n) {
   n instanceof CleartextLogging::Sink
 }
 
-/** A node representing data being passed to an external API. */
+/** A node representing data being passed to an unknown external API. */
 class UnknownExternalAPIDataNode extends ExternalAPIDataNode {
   UnknownExternalAPIDataNode() {
     // Not a sink for a commonly-used query