fix(api): return 404 status codes on "not found" pages (#674)

contributes to #638.

app/controllers/postViewer.js

@@ -16,7 +16,6 @@ exports.controller = function (request, reqParams, response) {
 
   function render(p) {
     if (p && p.errorCode) {
-      console.log('postViewer error:', p.errorCode, 'on', request.url);
       errorTemplate.renderErrorResponse(
         p,
         response,

app/templates/error.js

@@ -1,3 +1,5 @@
+// @ts-check
+
 /**
  * template for error pages
  * @author adrienjoly, whyd
@@ -15,14 +17,33 @@ const renderPage404 = (params) =>
     content: page404Html,
   });
 
+/** @typedef {{ message: string, status: number }} ErrorWithStatus */
+
+/** @type {Record<number|string, ErrorWithStatus>} */
 exports.ERRORCODE = {
-  401: 'Unauthorized. Please login before accessing this page.',
-  404: '404 / not found. There might have been some music here, in the past... Please make sure that this URL is correct.',
-  REQ_LOGIN: 'Please login first',
-  USER_NOT_FOUND: 'User not found...',
-  POST_NOT_FOUND: "Sorry, we can't find this track... maybe was it deleted?",
+  401: {
+    status: 404,
+    message: 'Unauthorized. Please login before accessing this page.',
+  },
+  404: {
+    status: 404,
+    message:
+      '404 / not found. There might have been some music here, in the past... Please make sure that this URL is correct.',
+  },
+  REQ_LOGIN: { status: 401, message: 'Please login first' },
+  USER_NOT_FOUND: { status: 404, message: 'User not found...' },
+  POST_NOT_FOUND: {
+    status: 404,
+    message: "Sorry, we can't find this track... maybe was it deleted?",
+  },
 };
 
+/**
+ * Renders a HTML page for the provided error message.
+ * @param {string} errorMessage
+ * @param {unknown} loggedUser
+ * @returns {string}
+ */
 exports.renderErrorMessage = function (errorMessage, loggedUser) {
   const params = {
     loggedUser: loggedUser,
@@ -41,26 +62,31 @@ exports.renderErrorCode = function (errorCode, loggedUser) {
   if (!err) {
     console.error('invalid error code:', errorCode);
   }
-  return exports.renderErrorMessage(err, loggedUser);
+  return exports.renderErrorMessage(err.message, loggedUser);
 };
 
+/** @typedef {keyof typeof exports.ERRORCODE} ErrorCode */
+
+/**
+ *
+ * @param {{ errorCode?: ErrorCode, error?: string } | undefined} errorObj
+ * @param {*} response
+ * @param {*} format
+ * @param {*} loggedUser
+ */
 exports.renderErrorResponse = function (
   errorObj,
   response,
   format = 'html',
   loggedUser,
 ) {
-  const statusCode =
-    errorObj && typeof errorObj.errorCode === 'number' && errorObj.errorCode;
+  const errorCode = errorObj?.errorCode;
+  const statusCode = exports.ERRORCODE[errorCode]?.status;
   //var format = (querystring.parse(url.parse(request.url).query) || {}).format || "";
   if (format.toLowerCase() == 'json') {
-    errorObj.error = errorObj.error || exports.ERRORCODE[errorObj.errorCode];
+    errorObj.error = errorObj.error || exports.ERRORCODE[errorCode]?.message;
     response.renderJSON(errorObj, statusCode);
-  } else if (
-    errorObj.errorCode == 404 ||
-    errorObj.errorCode == 'USER_NOT_FOUND'
-  ) {
-    response.status(404);
+  } else if (errorCode == 404 || errorCode == 'USER_NOT_FOUND') {
     if (format === 'html') {
       //response.sendFile("public/html/404.html");
       response.renderHTML(
@@ -72,9 +98,9 @@ exports.renderErrorResponse = function (
       );
       // TODO: response.render('404', { url: req.url });
     } else if (format === 'json') {
-      response.send({ error: 'Not found' });
+      response.status(statusCode).send({ error: 'Not found' });
     } else {
-      response.type('txt').send('Not found');
+      response.status(statusCode).type('txt').send('Not found');
     }
   } else if (errorObj.errorCode)
     response.renderHTML(

test/api/base.api.tests.js

@@ -0,0 +1,30 @@
+const assert = require('assert');
+const util = require('util');
+
+const { OpenwhydTestEnv } = require('../approval-tests-helpers.js');
+const { cleanup } = require('../fixtures.js');
+const api = require('../api-client.js');
+
+describe('base api', function () {
+  const openwhyd = new OpenwhydTestEnv({
+    startWithEnv: process.env.START_WITH_ENV_FILE,
+  });
+
+  before(cleanup.bind(this, { silent: true })); // to prevent side effects between tests
+
+  before(async () => {
+    await openwhyd.setup();
+  });
+
+  after(async () => {
+    await openwhyd.release();
+  });
+
+  it("should return a 404 for URLs that don't exist", async () => {
+    const { response } = await util.promisify(api.getRaw)(
+      null,
+      '//wp-content/dropdown.php',
+    );
+    assert.equal(response.statusCode, 404);
+  });
+});

test/api/post.api.tests.js

@@ -8,6 +8,7 @@ const {
   cleanup,
   URL_PREFIX,
   DUMMY_USER,
+  FAKE_ID,
 } = require('../fixtures.js');
 const api = require('../api-client.js');
 const randomString = () => Math.random().toString(36).substring(2, 9);
@@ -43,6 +44,14 @@ describe(`post api`, function () {
      */
   });
 
+  it("should return a 404 for post that doesn't exist", async () => {
+    const { response } = await util.promisify(api.getRaw)(
+      null,
+      `/c/${FAKE_ID}`,
+    );
+    assert.equal(response.statusCode, 404);
+  });
+
   it("should edit a track's name", async function () {
     const { body } = await api.addPost(jar, post);
     const pId = body._id;

test/api/user.api.tests.js

@@ -3,7 +3,7 @@ const assert = require('assert');
 const util = require('util');
 
 const { OpenwhydTestEnv } = require('../approval-tests-helpers.js');
-const { DUMMY_USER, cleanup } = require('../fixtures.js');
+const { DUMMY_USER, cleanup, FAKE_ID } = require('../fixtures.js');
 const api = require('../api-client.js');
 
 describe('user api', function () {
@@ -21,6 +21,19 @@ describe('user api', function () {
     await openwhyd.release();
   });
 
+  it("should return a 404 for user handle that doesn't exist", async () => {
+    const { response } = await util.promisify(api.getRaw)(null, '/nobody');
+    assert.equal(response.statusCode, 404);
+  });
+
+  it("should return a 404 for user id that doesn't exist", async () => {
+    const { response } = await util.promisify(api.getRaw)(
+      null,
+      `/u/${FAKE_ID}`,
+    );
+    assert.equal(response.statusCode, 404);
+  });
+
   describe(`getting user data`, function () {
     it(`gets user profile data`, function (done) {
       const url =

test/fixtures.js

@@ -15,6 +15,8 @@ exports.loadEnvVars = async (file) => {
   return envVars;
 };
 
+exports.FAKE_ID = 'a0000000000000000000000a';
+
 exports.URL_PREFIX = 'http://localhost:8080';
 
 // inserted by config/initdb_testing.js