Merge pull request #13755 from github/max-schaefer/js-server-crash-help

JavaScript: Improve qhelp for js/server-crash.

Author: asgerf

javascript/ql/src/Security/CWE-730/ServerCrash.qhelp

@@ -42,15 +42,13 @@
 
 	<p>
 
-		The following server implementation checks if a client-provided
-		file path is valid and throws an exception if the check fails. It can
-		be seen that the exception is uncaught, and it is therefore reasonable to
-		expect the server to respond with an error response to client requests
-		that cause the check to fail.
-
-		But since the exception is uncaught in the context of an
-		asynchronous callback invocation (<code>fs.access(...)</code>), the
-		entire server will terminate instead.
+		The following server code checks if a client-provided file path is valid
+		before saving data to that path. It would be reasonable to expect that the
+		server responds with an error in case the request contains an invalid
+		file path. However, the server instead throws an exception, which is
+		uncaught in the context of the asynchronous callback invocation
+		(<code>fs.access(...)</code>). This causes the entire server to
+		terminate abruptly.
 
 	</p>
 
@@ -67,11 +65,9 @@
 
 	<p>
 
-		An alternative is to use an <code>async</code> and
-		<code>await</code> for the asynchronous behavior, since the server
-		will then print warning messages about uncaught exceptions instead of
-		terminating, unless the server was started with the commandline option
-		<code>--unhandled-rejections=strict</code>:
+		To simplify exception handling, it may be advisable to switch to
+		async/await syntax instead of using callbacks, which allows wrapping the
+		entire request handler in a <code>try/catch</code> block:
 
 	</p>
 

javascript/ql/src/Security/CWE-730/examples/server-crash.BAD.js

@@ -7,10 +7,13 @@ function save(rootDir, path, content) {
   }
   // write content to disk
 }
+
 express().post("/save", (req, res) => {
-  fs.exists(rootDir, (exists) => {
-    if (!exists) {
-      console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
+  fs.access(rootDir, (err) => {
+    if (err) {
+      console.error(
+        `Server setup is corrupted, ${rootDir} cannot be accessed!`
+      );
       res.status(500);
       res.end();
       return;

javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-A.js

@@ -1,9 +1,9 @@
 // ...
 express().post("/save", (req, res) => {
-  fs.exists(rootDir, (exists) => {
+  fs.access(rootDir, (err) => {
     // ...
     try {
-      save(rootDir, req.query.path, req.body); // GOOD no uncaught exception
+      save(rootDir, req.query.path, req.body); // GOOD exception is caught below
       res.status(200);
       res.end();
     } catch (e) {

javascript/ql/src/Security/CWE-730/examples/server-crash.GOOD-B.js

@@ -1,12 +1,12 @@
 // ...
 express().post("/save", async (req, res) => {
-  if (!(await fs.promises.exists(rootDir))) {
-    console.error(`Server setup is corrupted, ${rootDir} does not exist!`);
+  try {
+    await fs.promises.access(rootDir);
+    save(rootDir, req.query.path, req.body); // GOOD exception is caught below
+    res.status(200);
+    res.end();
+  } catch (e) {
     res.status(500);
     res.end();
-    return;
   }
-  save(rootDir, req.query.path, req.body); // MAYBE BAD, depends on the commandline options
-  res.status(200);
-  res.end();
 });