Add support for comma-separated lists to filter and ensure comparison…

…s are valid (#356) (#1634) * add support for lists to filters * update changelog * add check that provided comparison is a valid comparison * fix issue with test * fix comment
CityOfDetroit · Aug 11, 2021 · 72cfb3d · 72cfb3d
1 parent dc1ecd5
commit 72cfb3d
Show file tree

Hide file tree

Showing 4 changed files with 80 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -31,6 +31,7 @@ All notable changes to this project will be documented in this file. The format
 
   - Filtering, pagination, and tests for listings endpoint (Parts of Detroit Team [#18](https://github.com/CityOfDetroit/bloom/pull/18), [#133](https://github.com/CityOfDetroit/bloom/pull/133), [#180](https://github.com/CityOfDetroit/bloom/pull/180), [#257](https://github.com/CityOfDetroit/bloom/pull/257), [#264](https://github.com/CityOfDetroit/bloom/pull/264), [#271](https://github.com/CityOfDetroit/bloom/pull/271)) [#1578](https://github.com/CityOfDetroit/bloom/pull/1578)
   - Units summary table ([#1607](https://github.com/bloom-housing/bloom/pull/1607))
+  - Add support for comma-separated lists to filters, ensure comparison is valid ([Detroit Team #356](https://github.com/CityOfDetroit/bloom/pull/356), [#1634](https://github.com/bloom-housing/bloom/pull/1634))
 
 - Changed:
 

diff --git a/backend/core/src/listings/listings.service.spec.ts b/backend/core/src/listings/listings.service.spec.ts
@@ -108,6 +108,32 @@ describe("ListingsService", () => {
       )
     })
 
+    it("should support filters with comma-separated arrays", async () => {
+      mockListingsRepo.createQueryBuilder
+        .mockReturnValueOnce(mockInnerQueryBuilder)
+        .mockReturnValueOnce(mockQueryBuilder)
+      const expectedNeighborhoodString = "Fox Creek, , Coliseum," // intentional extra and trailing commas for test
+      // lowercased, trimmed spaces, filtered empty
+      const expectedNeighborhoodArray = ["fox creek", "coliseum"]
+
+      const queryParams: ListingsQueryParams = {
+        filter: {
+          $comparison: Compare["IN"],
+          neighborhood: expectedNeighborhoodString,
+        },
+      }
+
+      const listings = await service.list(origin, queryParams)
+
+      expect(listings.items).toEqual(mockListings)
+      expect(mockInnerQueryBuilder.andWhere).toHaveBeenCalledWith(
+        "LOWER(CAST(property.neighborhood as text)) IN (:...neighborhood_0)",
+        {
+          neighborhood_0: expectedNeighborhoodArray,
+        }
+      )
+    })
+
     it("should throw an exception if an unsupported filter is used", async () => {
       mockListingsRepo.createQueryBuilder.mockReturnValueOnce(mockInnerQueryBuilder)
 
@@ -116,7 +142,7 @@ describe("ListingsService", () => {
           $comparison: Compare["="],
           otherField: "otherField",
           // The querystring can contain unknown fields that aren't on the
-          // ListingFilterParams type, so we force it to the type for testing
+          // ListingFilterParams type, so we force it to the type for testing.
         } as ListingFilterParams,
       }
 
@@ -125,6 +151,25 @@ describe("ListingsService", () => {
       )
     })
 
+    //TODO(avaleske): A lot of these tests should be moved to a spec file specific to the filters code.
+    it("should throw an exception if an unsupported comparison is used", async () => {
+      mockListingsRepo.createQueryBuilder.mockReturnValueOnce(mockInnerQueryBuilder)
+
+      const queryParams: ListingsQueryParams = {
+        filter: {
+          // The value of the filter[$comparison] query param is not validated,
+          // and the type system trusts that whatever is provided is correct,
+          // so we force it to an invalid type for testing.
+          $comparison: "); DROP TABLE Students;" as Compare,
+          name: "test name",
+        } as ListingFilterParams,
+      }
+
+      await expect(service.list(origin, queryParams)).rejects.toThrow(
+        new HttpException("Comparison Not Implemented", HttpStatus.NOT_IMPLEMENTED)
+      )
+    })
+
     it("should not call limit() and offset() if pagination params are not specified", async () => {
       mockListingsRepo.createQueryBuilder
         .mockReturnValueOnce(mockInnerQueryBuilder)

diff --git a/backend/core/src/shared/dto/filter.dto.ts b/backend/core/src/shared/dto/filter.dto.ts
@@ -5,6 +5,7 @@ import { Expose } from "class-transformer"
 export enum Compare {
   "=" = "=",
   "<>" = "<>",
+  "IN" = "IN",
 }
 
 export class BaseFilter {

diff --git a/backend/core/src/shared/filter/index.ts b/backend/core/src/shared/filter/index.ts
@@ -1,5 +1,6 @@
 import { HttpException, HttpStatus } from "@nestjs/common"
 import { WhereExpression } from "typeorm"
+import { Compare } from "../dto/filter.dto"
 
 /**
  *
@@ -57,14 +58,37 @@ export function addFilters<FilterParams, FilterFieldMap>(
         values.forEach((val: string, i: number) => {
           // Each WHERE param must be unique across the entire QueryBuilder
           const whereParameterName = `${filterType}_${i}`
-          qb.andWhere(
-            `LOWER(CAST(${filterTypeToFieldMap[filterType.toLowerCase()]} as text)) ${
-              comparisonsForCurrentFilter[i]
-            } LOWER(:${whereParameterName})`,
-            {
-              [whereParameterName]: val,
-            }
-          )
+
+          const comparison = comparisonsForCurrentFilter[i]
+          // Explicitly check for allowed comparisons, to prevent SQL injections
+          switch (comparison) {
+            case Compare.IN:
+              qb.andWhere(
+                `LOWER(CAST(${
+                  filterTypeToFieldMap[filterType.toLowerCase()]
+                } as text)) IN (:...${whereParameterName})`,
+                {
+                  [whereParameterName]: val
+                    .split(",")
+                    .map((s) => s.trim().toLowerCase())
+                    .filter((s) => s.length !== 0),
+                }
+              )
+              break
+            case Compare["<>"]:
+            case Compare["="]:
+              qb.andWhere(
+                `LOWER(CAST(${
+                  filterTypeToFieldMap[filterType.toLowerCase()]
+                } as text)) ${comparison} LOWER(:${whereParameterName})`,
+                {
+                  [whereParameterName]: val,
+                }
+              )
+              break
+            default:
+              throw new HttpException("Comparison Not Implemented", HttpStatus.NOT_IMPLEMENTED)
+          }
         })
       }
     }