fix: "hstore injection" & properly handle NULL, empty string, backslashes & quotes in hstore key/value pairs (typeorm#4720)

tobyhinloopen · pleerock · commit 3abe5b9508c9 · 2019-09-13T19:04:12.000+05:00
* Improve HStore object support

* Add hstore-injection test
diff --git a/src/driver/postgres/PostgresDriver.ts b/src/driver/postgres/PostgresDriver.ts
@@ -424,9 +424,18 @@ export class PostgresDriver implements Driver {
             if (typeof value === "string") {
                 return value;
             } else {
-                return Object.keys(value).map(key => {
-                    return `"${key}"=>"${value[key]}"`;
-                }).join(", ");
+                // https://www.postgresql.org/docs/9.0/hstore.html
+                const quoteString = (value: unknown) => {
+                    // If a string to be quoted is `null` or `undefined`, we return a literal unquoted NULL.
+                    // This way, NULL values can be stored in the hstore object.
+                    if (value === null || typeof value === "undefined") {
+                        return "NULL";
+                    }
+                    // Convert non-null values to string since HStore only stores strings anyway.
+                    // To include a double quote or a backslash in a key or value, escape it with a backslash.
+                    return `"${`${value}`.replace(/(?=["\\])/g, "\\")}"`;
+                };
+                return Object.keys(value).map(key => quoteString(key) + "=>" + quoteString(value[key])).join(",");
             }
 
         } else if (columnMetadata.type === "simple-array") {
@@ -476,13 +485,13 @@ export class PostgresDriver implements Driver {
 
         } else if (columnMetadata.type === "hstore") {
             if (columnMetadata.hstoreType === "object") {
-                const regexp = /"(.*?)"=>"(.*?[^\\"])"/gi;
-                const matchValue = value.match(regexp);
+                const unescapeString = (str: string) => str.replace(/\\./g, (m) => m[1]);
+                const regexp = /"([^"\\]*(?:\\.[^"\\]*)*)"=>(?:(NULL)|"([^"\\]*(?:\\.[^"\\]*)*)")(?:,|$)/g;
                 const object: ObjectLiteral = {};
-                let match;
-                while (match = regexp.exec(matchValue)) {
-                    object[match[1].replace(`\\"`, `"`)] = match[2].replace(`\\"`, `"`);
-                }
+                `${value}`.replace(regexp, (_, key, nullValue, stringValue) => {
+                    object[unescapeString(key)] = nullValue ? null : unescapeString(stringValue);
+                    return "";
+                });
                 return object;
 
             } else {
diff --git a/test/github-issues/4719/entity/Post.ts b/test/github-issues/4719/entity/Post.ts
@@ -0,0 +1,12 @@
+import {Column, Entity, PrimaryGeneratedColumn, ObjectLiteral} from "../../../../src/index";
+
+@Entity()
+export class Post {
+
+    @PrimaryGeneratedColumn()
+    id: number;
+
+    @Column("hstore", { hstoreType: "object" })
+    hstoreObj: ObjectLiteral;
+
+}
diff --git a/test/github-issues/4719/issue-4719.ts b/test/github-issues/4719/issue-4719.ts
@@ -0,0 +1,41 @@
+import "reflect-metadata";
+import {closeTestingConnections, createTestingConnections, reloadTestingDatabases} from "../../utils/test-utils";
+import {Connection} from "../../../src/connection/Connection";
+import {Post} from "./entity/Post";
+
+describe("github issues > #4719 HStore with empty string values", () => {
+    let connections: Connection[];
+    before(async () => connections = await createTestingConnections({
+        entities: [__dirname + "/entity/*{.js,.ts}"],
+        enabledDrivers: ["postgres"]
+    }));
+    beforeEach(() => reloadTestingDatabases(connections));
+    after(() => closeTestingConnections(connections));
+
+    it("should handle HStore with empty string keys or values", () => Promise.all(connections.map(async connection => {
+      const queryRunner = connection.createQueryRunner();
+      const postRepository = connection.getRepository(Post);
+
+      const post = new Post();
+      post.hstoreObj = {name: "Alice", surname: "A", age: 25, blank: "", "": "blank-key", "\"": "\"", foo: null};
+      const {id} = await postRepository.save(post);
+
+      const loadedPost = await postRepository.findOneOrFail(id);
+      loadedPost.hstoreObj.should.be.deep.equal(
+        { name: "Alice", surname: "A", age: "25", blank: "", "": "blank-key", "\"": "\"", foo: null });
+      await queryRunner.release();
+    })));
+
+    it("should not allow 'hstore injection'", () => Promise.all(connections.map(async connection => {
+      const queryRunner = connection.createQueryRunner();
+      const postRepository = connection.getRepository(Post);
+
+      const post = new Post();
+      post.hstoreObj = { username: `", admin=>"1`, admin: "0" };
+      const {id} = await postRepository.save(post);
+
+      const loadedPost = await postRepository.findOneOrFail(id);
+      loadedPost.hstoreObj.should.be.deep.equal({ username: `", admin=>"1`, admin: "0" });
+      await queryRunner.release();
+    })));
+});
