fix: Remove ability to use GET for RPC API requests by default (#4042)

* fix: Remove ability to use GET for RPC API requests by default

* tsc

server/middlewares/methodOverride.ts

@@ -3,11 +3,10 @@ import queryString from "query-string";
 
 export default function methodOverride() {
   return async function methodOverrideMiddleware(ctx: Context, next: Next) {
+    // TODO: Need to remove this use of ctx.body to enable proper typing of requests
     if (ctx.method === "POST") {
       ctx.body = ctx.request.body;
     } else if (ctx.method === "GET") {
-      ctx.method = 'POST'; // eslint-disable-line
-
       ctx.body = queryString.parse(ctx.querystring);
     }
 

server/routes/api/attachments.ts

@@ -7,6 +7,7 @@ import { AuthorizationError, ValidationError } from "@server/errors";
 import auth from "@server/middlewares/authentication";
 import { Attachment, Document, Event } from "@server/models";
 import { authorize } from "@server/policies";
+import { ContextWithState } from "@server/types";
 import {
   getPresignedPost,
   publicS3Endpoint,
@@ -145,9 +146,10 @@ router.post("attachments.delete", auth(), async (ctx) => {
   };
 });
 
-router.post("attachments.redirect", auth(), async (ctx) => {
-  const { id } = ctx.body;
+const handleAttachmentsRedirect = async (ctx: ContextWithState) => {
+  const { id } = ctx.body as { id?: string };
   assertUuid(id, "id is required");
+
   const { user } = ctx.state;
   const attachment = await Attachment.findByPk(id, {
     rejectOnEmpty: true,
@@ -163,6 +165,9 @@ router.post("attachments.redirect", auth(), async (ctx) => {
   } else {
     ctx.redirect(attachment.canonicalUrl);
   }
-});
+};
+
+router.get("attachments.redirect", auth(), handleAttachmentsRedirect);
+router.post("attachments.redirect", auth(), handleAttachmentsRedirect);
 
 export default router;

server/routes/api/cron.ts

@@ -40,9 +40,11 @@ const cronHandler = async (ctx: Context) => {
   };
 };
 
+router.get("cron.:period", cronHandler);
 router.post("cron.:period", cronHandler);
 
 // For backwards compatibility
+router.get("utils.gc", cronHandler);
 router.post("utils.gc", cronHandler);
 
 export default router;

server/routes/api/documents.ts

@@ -14,6 +14,7 @@ import {
   NotFoundError,
   InvalidRequestError,
   AuthenticationError,
+  ValidationError,
 } from "@server/errors";
 import auth from "@server/middlewares/authentication";
 import {
@@ -478,8 +479,10 @@ router.post("documents.restore", auth({ member: true }), async (ctx) => {
   // be caught as a 403 on the authorize call below. Otherwise we're checking here
   // that the original collection still exists and advising to pass collectionId
   // if not.
-  if (!collectionId) {
-    assertPresent(collection, "collectionId is required");
+  if (!collectionId && !collection) {
+    throw ValidationError(
+      "Unable to restore to original collection, it may have been deleted"
+    );
   }
 
   authorize(user, "update", collection);

server/routes/api/index.test.ts

@@ -17,3 +17,10 @@ describe("GET unknown endpoint", () => {
     expect(res.status).toEqual(404);
   });
 });
+
+describe("PATCH unknown endpoint", () => {
+  it("should be method not allowed", async () => {
+    const res = await server.patch("/api/blah");
+    expect(res.status).toEqual(405);
+  });
+});

server/routes/api/index.ts

@@ -1,4 +1,4 @@
-import Koa from "koa";
+import Koa, { DefaultContext, DefaultState } from "koa";
 import bodyParser from "koa-body";
 import Router from "koa-router";
 import env from "@server/env";
@@ -6,6 +6,7 @@ import { NotFoundError } from "@server/errors";
 import errorHandling from "@server/middlewares/errorHandling";
 import methodOverride from "@server/middlewares/methodOverride";
 import { defaultRateLimiter } from "@server/middlewares/rateLimiter";
+import { AuthenticatedState } from "@server/types";
 import apiKeys from "./apiKeys";
 import attachments from "./attachments";
 import auth from "./auth";
@@ -33,7 +34,10 @@ import users from "./users";
 import views from "./views";
 import webhookSubscriptions from "./webhookSubscriptions";
 
-const api = new Koa();
+const api = new Koa<
+  DefaultState & AuthenticatedState,
+  DefaultContext & { body: Record<string, any> }
+>();
 const router = new Router();
 
 // middlewares
@@ -83,10 +87,15 @@ router.post("*", (ctx) => {
   ctx.throw(NotFoundError("Endpoint not found"));
 });
 
+router.get("*", (ctx) => {
+  ctx.throw(NotFoundError("Endpoint not found"));
+});
+
 api.use(defaultRateLimiter());
 
 // Router is embedded in a Koa application wrapper, because koa-router does not
 // allow middleware to catch any routes which were not explicitly defined.
 api.use(router.routes());
+api.use(router.allowedMethods());
 
 export default api;

server/routes/api/notificationSettings.ts

@@ -4,6 +4,7 @@ import auth from "@server/middlewares/authentication";
 import { Team, NotificationSetting } from "@server/models";
 import { authorize } from "@server/policies";
 import { presentNotificationSetting } from "@server/presenters";
+import { ContextWithState } from "@server/types";
 import { assertPresent, assertUuid } from "@server/validation";
 
 const router = new Router();
@@ -55,8 +56,8 @@ router.post("notificationSettings.delete", auth(), async (ctx) => {
   };
 });
 
-router.post("notificationSettings.unsubscribe", async (ctx) => {
-  const { id, token } = ctx.body;
+const handleUnsubscribe = async (ctx: ContextWithState) => {
+  const { id, token } = ctx.body as { id?: string; token?: string };
   assertUuid(id, "id is required");
   assertPresent(token, "token is required");
 
@@ -77,6 +78,9 @@ router.post("notificationSettings.unsubscribe", async (ctx) => {
   }
 
   ctx.redirect(`${env.URL}?notice=invalid-auth`);
-});
+};
+
+router.get("notificationSettings.unsubscribe", handleUnsubscribe);
+router.post("notificationSettings.unsubscribe", handleUnsubscribe);
 
 export default router;

server/test/env.ts

@@ -7,6 +7,7 @@ env.GOOGLE_CLIENT_ID = "123";
 env.GOOGLE_CLIENT_SECRET = "123";
 env.SLACK_CLIENT_ID = "123";
 env.SLACK_CLIENT_SECRET = "123";
+env.RATE_LIMITER_ENABLED = false;
 env.DEPLOYMENT = undefined;
 
 if (process.env.DATABASE_URL_TEST) {

server/types.ts

@@ -6,12 +6,14 @@ export enum AuthenticationType {
   APP = "app",
 }
 
+export type AuthenticatedState = {
+  user: User;
+  token: string;
+  authType: AuthenticationType;
+};
+
 export type ContextWithState = Context & {
-  state: {
-    user: User;
-    token: string;
-    authType: AuthenticationType;
-  };
+  state: AuthenticatedState;
 };
 
 type BaseEvent = {

server/validation.ts

@@ -1,21 +1,27 @@
 import { isArrayLike } from "lodash";
+import { Primitive } from "utility-types";
 import validator from "validator";
 import { CollectionPermission } from "../shared/types";
 import { validateColorHex } from "../shared/utils/color";
 import { validateIndexCharacters } from "../shared/utils/indexCharacters";
 import { ParamRequiredError, ValidationError } from "./errors";
 
-export const assertPresent = (value: unknown, message: string) => {
+type IncomingValue = Primitive | string[];
+
+export const assertPresent = (value: IncomingValue, message: string) => {
   if (value === undefined || value === null || value === "") {
     throw ParamRequiredError(message);
   }
 };
 
-export const assertArray = (value: unknown, message?: string) => {
+export function assertArray(
+  value: IncomingValue,
+  message?: string
+): asserts value {
   if (!isArrayLike(value)) {
     throw ValidationError(message);
   }
-};
+}
 
 export const assertIn = (
   value: string,
@@ -37,41 +43,57 @@ export const assertSort = (
   }
 };
 
-export const assertNotEmpty = (value: unknown, message: string) => {
+export function assertNotEmpty(
+  value: IncomingValue,
+  message: string
+): asserts value {
   assertPresent(value, message);
 
   if (typeof value === "string" && value.trim() === "") {
     throw ValidationError(message);
   }
-};
+}
 
-export const assertEmail = (value = "", message?: string) => {
-  if (!validator.isEmail(value)) {
+export function assertEmail(
+  value: IncomingValue = "",
+  message?: string
+): asserts value {
+  if (typeof value !== "string" || !validator.isEmail(value)) {
     throw ValidationError(message);
   }
-};
+}
 
-export const assertUrl = (value = "", message?: string) => {
+export function assertUrl(
+  value: IncomingValue = "",
+  message?: string
+): asserts value {
   if (
+    typeof value !== "string" ||
     !validator.isURL(value, {
       protocols: ["http", "https"],
       require_valid_protocol: true,
     })
   ) {
-    throw ValidationError(message ?? `${value} is an invalid url!`);
+    throw ValidationError(message ?? `${String(value)} is an invalid url!`);
   }
-};
+}
 
-export const assertUuid = (value: unknown, message?: string) => {
+export function assertUuid(
+  value: IncomingValue,
+  message?: string
+): asserts value {
   if (typeof value !== "string") {
     throw ValidationError(message);
   }
   if (!validator.isUUID(value)) {
     throw ValidationError(message);
   }
-};
+}
 
-export const assertPositiveInteger = (value: unknown, message?: string) => {
+export const assertPositiveInteger = (
+  value: IncomingValue,
+  message?: string
+) => {
   if (
     !validator.isInt(String(value), {
       min: 0,