Fix creating a justification based on an existing proposition (#387)

* Remove .strict from Entity schemas (instead rely on the default .strip behavior so that it's convenient to pass Out models as Input models.
* Fix a XSS vulnerability where we allowed javascript: URLs.
* Add check:yarn-install to check:everything

---------

Signed-off-by: Carl Gieringer <78054+carlgieringer@users.noreply.github.com>

howdju-client-common/lib/models.ts

@@ -111,7 +111,11 @@ export const makeCreateContentReportInput = (
     fields
   );
 
-/** Remove the fields that are circular, unserializable, or not part of the API update model. */
+/**
+ * Remove the fields that are circular.
+ *
+ * TODO(386) add a generic circularity check in our action creators.
+ */
 export function toUpdatePropositionInput(
   proposition: PropositionOut
 ): UpdatePropositionInput {

howdju-common/lib/zodRefinements.ts

@@ -4,18 +4,21 @@ import isUrl from "validator/lib/isURL";
 
 import { extractDomain } from "./urls";
 
-type UrlOptions = { domain: RegExp };
+type UrlOptions = {
+  domain?: RegExp;
+};
+
 /** Zod refinement for whether a string is a valid URL.
  *
- * If domain is present, the URL must m
+ * If domain is present, the URL's domain must match it.
  */
 const urlRefinement =
   (options: UrlOptions) => (val: string, ctx: z.RefinementCtx) => {
-    const { domain: domainPattern } = options;
-    if (!isUrl(val)) {
+    const { domain: domainPattern } = options ?? {};
+    if (!isUrl(val, { protocols: ["http", "https"], require_protocol: true })) {
       ctx.addIssue({
         code: z.ZodIssueCode.custom,
-        message: "Must be a URL",
+        message: "Must be a valid URL",
       });
     }
 
@@ -30,8 +33,8 @@ const urlRefinement =
     }
   };
 
-export const urlString = (options: UrlOptions) =>
-  z.string().superRefine(urlRefinement(options));
+export const urlString = (options?: UrlOptions) =>
+  z.string().superRefine(urlRefinement(options ?? {}));
 
 // @types/moment doesn't provide this constructor, but it works.
 type MomentConstructor = {

howdju-common/lib/zodSchemas.test.ts

@@ -1,4 +1,5 @@
 import moment from "moment";
+import { urlString } from "./zodRefinements";
 import { Justification } from "./zodSchemas";
 
 describe("Justification schema", () => {
@@ -44,3 +45,18 @@ describe("Justification schema", () => {
     expect(result).toEqual(justification);
   });
 });
+
+describe("urlString", () => {
+  it("should match a domain", () => {
+    const result = urlString({
+      domain: /some-websites.com$/,
+    }).safeParse("https://www.some-websites.com/the-path");
+    expect(result.success).toBe(true);
+  });
+  it("should not match a different domain", () => {
+    const result = urlString({
+      domain: /the-other-site.com$/,
+    }).safeParse("https://www.some-websites.com/the-path");
+    expect(result.success).toBe(false);
+  });
+});

howdju-common/lib/zodSchemas.ts

@@ -37,7 +37,7 @@ export const Proposition = Entity.extend({
    */
   text: z.string().min(1),
   created: momentObject,
-}).strict();
+});
 export type Proposition = z.infer<typeof Proposition>;
 
 export const UpdatePropositionInput = Proposition.merge(PersistedEntity).omit({
@@ -128,12 +128,12 @@ export const Persorg = Entity.extend({
    */
   knownFor: z.string().optional(),
   /** The official or primary website representing the persorg. */
-  websiteUrl: z.string().url().optional(),
+  websiteUrl: urlString().optional(),
   /** The persorg's Twitter. */
   twitterUrl: urlString({ domain: /twitter.com$/ }).optional(),
   /** The persorg's Wikipedia URL. */
   wikipediaUrl: urlString({ domain: /wikipedia.org$/ }).optional(),
-}).strict();
+});
 export type Persorg = z.infer<typeof Persorg>;
 
 export const CreatePersorg = Persorg;
@@ -173,12 +173,12 @@ export const Statement: z.ZodType<Statement> = z.lazy(() =>
       sentenceType: z.literal(sentenceTypes.Enum.PROPOSITION),
       sentence: Proposition,
       ...baseStatement,
-    }).strict(),
+    }),
     Entity.extend({
       sentenceType: z.literal(sentenceTypes.Enum.STATEMENT),
       sentence: Statement,
       ...baseStatement,
-    }).strict(),
+    }),
   ])
 );
 export type Sentence = Statement["sentence"];
@@ -204,12 +204,12 @@ export const CreateStatementInput: z.ZodType<CreateStatementInput> = z.lazy(
         sentenceType: z.literal(sentenceTypes.Enum.PROPOSITION),
         sentence: CreatePropositionInput,
         speaker: Persorg,
-      }).strict(),
+      }),
       Entity.extend({
         sentenceType: z.literal(sentenceTypes.Enum.STATEMENT),
         sentence: CreateStatementInput,
         speaker: Persorg,
-      }).strict(),
+      }),
     ])
 );
 
@@ -231,12 +231,12 @@ export const CreateStatement: z.ZodType<CreateStatement> = z.lazy(() =>
       sentenceType: z.literal(sentenceTypes.Enum.PROPOSITION),
       sentence: CreateProposition,
       speaker: CreatePersorg,
-    }).strict(),
+    }),
     Entity.extend({
       sentenceType: z.literal(sentenceTypes.Enum.STATEMENT),
       sentence: CreateStatement,
       speaker: CreatePersorg,
-    }).strict(),
+    }),
   ])
 );
 // Statement has no Update models; users can edit the proposition/statement or the speaker.
@@ -278,7 +278,7 @@ export const UrlTarget = Entity.extend({
 });
 
 export const Url = Entity.extend({
-  url: z.string().url(),
+  url: urlString(),
   // TODO(38) I don't think target should be part of URL. Targets should be related to URLs.
   target: UrlTarget.optional(),
 });
@@ -988,7 +988,7 @@ export const ContentReport = Entity.extend({
   // When creating or reading a content report, we only need to keep the unique types.
   types: z.array(ContentReportType),
   description: z.string(),
-  url: z.string().url(),
+  url: urlString(),
   reporterUserId: z.string(),
   created: momentObject,
 });

howdju-service-common/lib/daos/JustificationsDao.test.ts

@@ -3,13 +3,8 @@ import moment from "moment";
 import { expect } from "@jest/globals";
 
 import { JustificationsDao } from "./JustificationsDao";
-import { mockLogger } from "howdju-test-common";
-import {
-  endPoolAndDropDb,
-  expectToBeSameMomentDeep,
-  initDb,
-  makeTestDbConfig,
-} from "@/util/testUtil";
+import { mockLogger, expectToBeSameMomentDeep } from "howdju-test-common";
+import { endPoolAndDropDb, initDb, makeTestDbConfig } from "@/util/testUtil";
 import { Pool } from "pg";
 import {
   AuthService,

howdju-service-common/lib/index.ts

@@ -1,3 +1,5 @@
+/// <reference path="../../howdju-test-common/lib/globals.d.ts" />
+
 import anyPromiseRegister from "any-promise/register";
 import * as moment from "moment";
 import momentDurationFormatSetup from "moment-duration-format";

howdju-service-common/lib/services/ContextTrailsService.test.ts

@@ -1,14 +1,9 @@
 import { Pool } from "pg";
 
 import { ContextTrailItemInfo } from "howdju-common";
-import { mockLogger } from "howdju-test-common";
+import { mockLogger, expectToBeSameMomentDeep } from "howdju-test-common";
 
-import {
-  endPoolAndDropDb,
-  expectToBeSameMomentDeep,
-  initDb,
-  makeTestDbConfig,
-} from "@/util/testUtil";
+import { endPoolAndDropDb, initDb, makeTestDbConfig } from "@/util/testUtil";
 import {
   AuthService,
   ConflictError,

howdju-service-common/lib/services/JustificationsService.test.ts

@@ -191,6 +191,47 @@ describe("JustificationsService", () => {
         },
       });
     });
+
+    test("fails to create a writ quote based justification with a javascript URL", async () => {
+      // Arrange
+      const { authToken } = await makeUser();
+
+      const createJustification = makeWritQuoteBasedJustification(
+        "javascript:alert('gotcha')"
+      );
+
+      // Act
+      let err;
+      try {
+        // Act
+        await service.readOrCreate(createJustification, authToken);
+        throw "Should have failed validation.";
+      } catch (e) {
+        err = e;
+      }
+
+      // Assert
+      expect(err).toBeInstanceOf(EntityValidationError);
+      expect(err).toMatchObject({
+        errors: {
+          basis: {
+            entity: {
+              urls: {
+                0: {
+                  url: {
+                    _errors: [
+                      {
+                        message: expect.stringContaining("Must be a valid URL"),
+                      },
+                    ],
+                  },
+                },
+              },
+            },
+          },
+        },
+      });
+    });
     test("can create a counter justification", async () => {
       // Arrange
       const { authToken, user } = await makeUser();
@@ -736,7 +777,9 @@ function makePropositionCompoundBasedJustification(): CreateJustification {
   };
 }
 
-function makeWritQuoteBasedJustification(): CreateJustification {
+function makeWritQuoteBasedJustification(
+  url: string = "https://www.trustworthy.news"
+): CreateJustification {
   return {
     target: {
       type: "PROPOSITION",
@@ -752,7 +795,7 @@ function makeWritQuoteBasedJustification(): CreateJustification {
         writ: {
           title: "Trustworthy online news source",
         },
-        urls: [{ url: "https://www.trustworthy.news" }],
+        urls: [{ url: url }],
       },
     },
     polarity: "POSITIVE",

howdju-service-common/lib/util/testUtil.ts

@@ -1,14 +1,12 @@
 import { randomBytes } from "crypto";
 import { readFileSync } from "fs";
 import { Pool, PoolConfig } from "pg";
-import { expect } from "@jest/globals";
 
 import { mockLogger } from "howdju-test-common";
 
 import { ApiConfig, baseConfig, makePool } from "..";
-import { logger, mapValuesDeep, toJson } from "howdju-common";
+import { logger, toJson } from "howdju-common";
 import { cloneDeep, toNumber } from "lodash";
-import { isMoment } from "moment";
 
 export function makeTestDbConfig() {
   return {
@@ -104,14 +102,3 @@ export async function endPoolAndDropDb(
 function randomDbName() {
   return "howdju_" + randomBytes(20).toString("hex");
 }
-
-/**
- * Deeply replaces all Moments with expect.toBeSameMoment.
- *
- * Use on a Jest expected value containing Moments to get reasonable match behavior.
- */
-export function expectToBeSameMomentDeep(value: any) {
-  return mapValuesDeep(value, (v) =>
-    isMoment(v) ? expect.toBeSameMoment(v) : v
-  );
-}

howdju-service-routes/lib/index.ts

@@ -1,3 +1,5 @@
+/// <reference path="../../howdju-test-common/lib/globals.d.ts" />
+
 export * from "./routes";
 export * from "./routeTypeHelpers";
 export type {

howdju-service-routes/package.json

@@ -20,6 +20,7 @@
   "devDependencies": {
     "eslint": "^8.27.0",
     "eslint-config-howdju": "workspace:eslint-config-howdju",
+    "howdju-test-common": "workspace:howdju-test-common",
     "prettier": "2.7.1",
     "typescript": "^4.9.4"
   },

howdju-test-common/lib/index.ts

@@ -1,2 +1,3 @@
 export * from "./FakeTopicMessageSender";
 export * from "./mockLogger";
+export * from "./testUtil";

howdju-test-common/lib/testUtil.ts

@@ -0,0 +1,17 @@
+import { isMoment } from "moment";
+
+import { mapValuesDeep } from "howdju-common";
+
+/**
+ * Deeply replaces all Moments with expect.toBeSameMoment.
+ *
+ * Use on a Jest expected value containing Moments to get reasonable match behavior.
+ */
+export function expectToBeSameMomentDeep(value: any) {
+  return mapValuesDeep(value, (v) =>
+    // Can't add `import { expect } from "@jest/globals";` above or else the esbuild fails.
+    // So just typecast expect.
+    // TODO(388) remove the typecast.
+    isMoment(v) ? (expect as any).toBeSameMoment(v) : v
+  );
+}

package.json

@@ -8,12 +8,13 @@
   "license": "AGPL-3.0-or-later",
   "private": true,
   "scripts": {
-    "check:everything": "echo check:everything && yarn run lint:all && yarn run check-format:all && yarn run typecheck:all && yarn run test:all && yarn run custom-check:all && yarn run check-build:all && yarn run check:todo-format && yarn run check:do-not-merge && echo check:everything succeeded || {echo check:everything failed; exit 1}",
+    "check:everything": "echo check:everything && yarn run check:yarn-install && yarn run lint:all && yarn run check-format:all && yarn run typecheck:all && yarn run test:all && yarn run custom-check:all && yarn run check-build:all && yarn run check:todo-format && yarn run check:do-not-merge && echo check:everything succeeded || {echo check:everything failed; exit 1}",
     "check:pushed": "bin/check-pushed.sh",
     "check:committed": "node bin/check-committed.mjs",
     "check:do-not-merge": "echo check:do-not-merge && bin/check-do-not-merge.sh && echo check:do-not-merge succeeded || {echo check:do-not-merge failed; exit 1}",
     "check:todo-format": "echo check:todo-format && bin/check-todo-format.sh && echo check:todo-format succeeded || {echo check:todo-format failed; exit 1}",
     "check:pre-deploy": "bin/pre-deploy-check.sh",
+    "check:yarn-install": "yarn install --immutable",
     "check-build:all": "echo check-build:all && yarn workspaces foreach -Apv run check-build && echo check-build:all succeeded || {echo check-build:all faied; exit 1}",
     "check-format": "yarn run prettier --check --ignore-path .gitignore jest lambdas/bin",
     "check-format:all": "echo check-format:all && yarn workspaces foreach -Apv run check-format && echo check-format:all succeeded || {echo check-format:all failed; exit 1}",

premiser-api/src/index.ts

@@ -1,3 +1,5 @@
+/// <reference path="../../howdju-test-common/lib/globals.d.ts" />
+
 import bluebird from "bluebird";
 
 // See https://github.com/DefinitelyTyped/DefinitelyTyped/issues/42084#issuecomment-581790307