Fix normalizePort and consolidate delete handlers

bin/tinyNode.js

@@ -16,7 +16,9 @@ import http from "http"
  * Get port from environment and store in Express.
  */
 
-const port = normalizePort(process.env.PORT || '3002')
+const envPort = normalizePort(process.env.PORT)
+if (process.env.PORT && !envPort) console.warn(`Invalid PORT "${process.env.PORT}", using 3002`)
+const port = envPort || 3002
 app.set('port', port)
 
 /**
@@ -34,19 +36,13 @@ server.on('error', onError)
 server.on('listening', onListening)
 
 /**
- * Normalize a port into a number, string, or false.
+ * Normalize a port into a valid TCP port number or false.
  */
 
 function normalizePort(val) {
-  const port = parseInt(val, 10)
+  const port = Number(val)
 
-  if (isNaN(port)) {
-    // named pipe
-    return val
-  }
-
-  if (port >= 0) {
-    // port number
+  if (Number.isInteger(port) && port >= 1 && port <= 65535) {
     return port
   }
 
@@ -62,9 +58,7 @@ function onError(error) {
     throw error
   }
 
-  const bind = typeof port === 'string'
-    ? 'Pipe ' + port
-    : 'Port ' + port
+  const bind = 'Port ' + port
 
   // handle specific listen errors with friendly messages
   switch (error.code) {
@@ -86,9 +80,5 @@ function onError(error) {
  */
 
 function onListening() {
-  const addr = server.address()
-  const bind = typeof addr === 'string'
-    ? 'pipe ' + addr
-    : 'port ' + addr.port
-  console.log('Listening on ' + bind)
+  console.log('Listening on port ' + server.address().port)
 }

routes/delete.js

@@ -5,27 +5,24 @@ import { fetchRerum } from "../rerum.js"
 import { isPassthroughRequest, requirePassthroughAllowed, resolveAuthorization } from "../passthrough.js"
 const router = express.Router()
 
-/* Legacy delete pattern w/body. */
-router.delete('/', verifyJsonContentType, requirePassthroughAllowed, checkAccessToken, async (req, res, next) => {
+async function deleteRecord(req, res) {
   try {
-    if (!req?.body || !(req.body['@id'] ?? req.body.id)) {
+    const uri = req.params.id ?? req.body?.['@id'] ?? req.body?.id
+    const id = typeof uri === "string" ? uri.split("/").pop() : undefined
+    if (!id || id === "." || id === "..") {
       const err = new Error("No record id to delete! (https://store.rerum.io/v1/API.html#delete)")
       err.status = 400
       throw err
     }
-
-    const deleteBody = JSON.stringify(req.body)
+    const deleteURL = `${process.env.RERUM_API_ADDR}delete/${encodeURIComponent(id)}`
     const deleteOptions = {
-      body: deleteBody,
       method: 'DELETE',
       headers: {
         'user-agent': 'Tiny-Things/1.0',
         'Origin': process.env.ORIGIN,
-        'Authorization': resolveAuthorization(req),
-        'Content-Type' : "application/json; charset=utf-8"
+        'Authorization': resolveAuthorization(req)
       }
     }
-    const deleteURL = `${process.env.RERUM_API_ADDR}delete`
     await fetchRerum(deleteURL, deleteOptions)
     .then(async (resp) => {
       if (resp.ok) return
@@ -54,51 +51,13 @@ router.delete('/', verifyJsonContentType, requirePassthroughAllowed, checkAccess
     console.error(err)
     res.status(err.status ?? 500).type('text/plain').send(err.message ?? 'An error occurred')
   }
-})
+}
+
+/* Legacy delete pattern w/body. */
+router.delete('/', verifyJsonContentType, requirePassthroughAllowed, checkAccessToken, deleteRecord)
 
 /* DELETE an object by ID via the RERUM API. */
-router.delete('/:id', requirePassthroughAllowed, checkAccessToken, async (req, res, next) => {
-  try {
-
-    const deleteURL = `${process.env.RERUM_API_ADDR}delete/${req.params.id}`
-    const deleteOptions = {
-      method: "DELETE",
-      headers: {
-        'user-agent': 'Tiny-Things/1.0',
-        'Origin': process.env.ORIGIN,
-        'Authorization': resolveAuthorization(req),
-      }
-    }
-    await fetchRerum(deleteURL, deleteOptions)
-    .then(async (resp) => {
-      if (resp.ok) return
-      if (isPassthroughRequest(req) && (resp.status === 401 || resp.status === 403)) {
-        let rerumAuthMessage
-        try {
-          rerumAuthMessage = `${resp.status}: ${deleteURL} - ${await resp.text()}`
-        } catch (e) {
-          rerumAuthMessage = `${resp.status}: ${deleteURL} - A RERUM error occurred`
-        }
-        throw httpError(rerumAuthMessage, resp.status)
-      }
-      // The response from RERUM indicates a failure, likely with a specific code and textual body
-      let rerumErrorMessage
-      try {
-        rerumErrorMessage = `${resp.status ?? 500}: ${deleteURL} - ${await resp.text()}`
-      } catch (e) {
-        rerumErrorMessage = `500: ${deleteURL} - A RERUM error occurred`
-      }
-      const err = new Error(rerumErrorMessage)
-      err.status = 502
-      throw err
-    })
-    res.status(204).end()
-  }
-  catch (err) {
-    console.error(err)
-    res.status(err.status ?? 500).type('text/plain').send(err.message ?? 'An error occurred')
-  }
-})
+router.delete('/:id', requirePassthroughAllowed, checkAccessToken, deleteRecord)
 
 router.all('/', (req, res, next) => {
   res.status(405).send("Method Not Allowed")

test/routes/delete.test.js

@@ -43,11 +43,12 @@ describe("Check that the request/response behavior of the TinyNode delete route
 
     assert.equal(response.statusCode, 204)
 
-    // Verify upstream contract for body-based delete
-    assert.match(lastFetchUrl, /\/delete$/, "URL should end with /delete")
+    // Verify body-based delete is forwarded upstream as a path-based delete
+    assert.match(lastFetchUrl, /\/delete\/_not_$/, "URL should end with /delete/_not_")
     assert.equal(lastFetchOptions.method, "DELETE", "Method should be DELETE")
     assert.match(lastFetchOptions.headers["Authorization"], /^Bearer /, "Authorization header missing or invalid")
-    assert.equal(lastFetchOptions.headers["Content-Type"], "application/json; charset=utf-8", "Content-Type header mismatch")
+    assert.equal(lastFetchOptions.body, undefined, "Body should not be forwarded upstream")
+    assert.equal(lastFetchOptions.headers["Content-Type"], undefined, "Content-Type should not be sent")
 
     response = await request(routeTester)
       .delete("/delete/00000")
@@ -58,6 +59,30 @@ describe("Check that the request/response behavior of the TinyNode delete route
     assert.match(lastFetchUrl, /\/delete\/00000$/, "URL should end with /delete/00000")
     assert.equal(lastFetchOptions.method, "DELETE", "Method should be DELETE")
   })
+
+  it("Extracts the record id from the body 'id' key and from an encoded URI in the path.", async () => {
+    let response = await request(routeTester)
+      .delete("/delete")
+      .send({ id: rerumUri })
+      .set("Content-Type", "application/json")
+
+    assert.equal(response.statusCode, 204)
+    assert.match(lastFetchUrl, /\/delete\/_not_$/, "URL should end with /delete/_not_")
+
+    response = await request(routeTester)
+      .delete(`/delete/${encodeURIComponent(rerumUri)}`)
+
+    assert.equal(response.statusCode, 204)
+    assert.match(lastFetchUrl, /\/delete\/_not_$/, "URL should end with /delete/_not_")
+
+    response = await request(routeTester)
+      .delete("/delete")
+      .send({ "@id": `${rerumUri}?x=1#f` })
+      .set("Content-Type", "application/json")
+
+    assert.equal(response.statusCode, 204)
+    assert.match(lastFetchUrl, /\/delete\/_not_%3Fx%3D1%23f$/, "id should be URI encoded upstream")
+  })
 })
 
 describe("Check that incorrect TinyNode delete route usage results in expected RESTful responses from RERUM.  __rest __core", () => {