Update TypeORM package to v0.3.0 (CAMPD)

maxdiebold-erg commented 5 months ago

A SQL injection vulnerability has been found in the TypeORM package for versions prior to 0.3.0, so it must be updated. This version has a number of breaking changes detailed in the release notes, and it is necessary to make additional considerations for its interactions with NestJS. The steps required to update the package are as follows.

Steps to Update

Change to the `find` and `findOne` function signatures

In previous versions, the find* methods of the EntityManager and Repository classes were overloaded to accept an options object with query parameters or a single primitive that was interpreted as a WHERE condition against the corresponding table's primary key column. This single primitive is now longer accepted, so all calls must be converted to use the object syntax. There were also new versions of the functions added, findBy and findOneBy: these functions still require an object argument, but they assume the object details the WHERE conditions of the query (i.e. findBy({ id: 123 }) vs find({ where: { id: 123 } })). Changing to the *By versions may require updating unit tests.

Nulls must be made explicit in `where` conditions of `find*` methods

If a value of null is to be passed in to a query, it must be designated as such with the IsNull() operator (in the typeorm package). This means that all nullable variables must be checked when they are used in a query and replaced with IsNull() where appropriate. For example:

const foo = repository.findOneBy({ bar: baz.qux ?? IsNull() }); // Column `qux` of `baz` is null

In cases where the column being queried is not nullable, it may be better to conditionally execute the query instead.

`getManager` has been removed

The getManager() function has been deprecated, and while it hasn't been removed yet, it does not return a usable instance. Instead, it is recommended for NestJS projects to inject EntityManager or DataSource where needed, e.g.:

export class ExampleService {
  constructor(
    private readonly dataSource: DataSource,
    private readonly entityManager: EntityManager,
  ) {}
}

In the corresponding unit tests, EntityManager/DataSource (depending on which was used) should be added to the testing module's list of providers.

In cases where dependency injection is not possible (e.g. in custom interceptors), a custom service (ConnectionService) has been created in easey-common with static methods getEntityManager and getDataSource. ConnectionService.getEntityManager can be used as a drop-in replacement for getManager, but dependency injection is recommended as a best practice when possible.

Inline validators in validation pipes that use `getManager` should be moved to separate classes so that `EntityManager` can be injected

See the DbLookup pipe and validator for an example.

This change requires the validator to be listed as a provider in each app it is used. class-validator provides a helper for this: in the applications main.ts, wrap the app module as follows:

import { useContainer } from "class-validator";

export async function bootstrap() {
  const app = await NestFactory.create(AppModule);

  useContainer(app.select(AppModule), { fallbackOnErrors: true });

  // The rest...
}

Then in the application's app.module.ts, add the validator to the providers:

@Module({
  imports: [
    // Imports...
  ],
  providers: [DbLookupValidator],

Another result of this change is that the load static method of the CheckCatalogService was removed in favor of making the CheckCatalogModule dynamic. So instead of loading the entries in main.ts:

-await CheckCatalogService.load(
-  `camdecmpsmd.vw_example_catalog_results'
-);

Specify the table as an argument when dynamically loading the module:

@Module({
  imports: [
    CheckCatalogModule.register(
      'camdecmpsmd.vw_monitor_plan_api_check_catalog_results',
    ),
    // Other imports...
  ],
});

The `@EntityRepository` decorator has been removed, and the `Repository` class now has a constructor

The @EntityRepository decorator was previously used to create custom repositories from entities. They should now be created with the @Injectable decorator and a constructor designating the underlying entity and entity manager instance, for example:

@Injectable
export class ExampleRepository extends Repository<ExampleEntity> {
  constructor(entityManager: EntityManager) {
    super(ExampleEntity, entityManager);
  }

  // Repository methods...
}

In addition, the repository should be added to the corresponding modules list of providers, and to the list of exports if it will be consumed elsewhere via its containing module.

@Module({
  imports: [TypeOrmModule.forFeature([ExampleRepository])],
  providers: [ExampleService, ExampleRepository],
  exports: [TypeOrmModule, ExampleService, ExampleRepository],
})
export class ExampleModule {}

Note that the @InjectRepository decorator is no longer needed when adding the repository to a service's constructor, since it is already marked as an injectable.

The repository class methods can no longer be called as static methods on entity classes

Whereas it was previously possible to query an entity's repository through the entity, as in

-  const result = await new Example.findOneBy({ id: "foo" });

They should all now be queried through an instance of the repository, either via injecting the repository into the constructor or through the entity manager

const result = await this.exampleRepository.findOneBy({ id: "foo" });

// OR

const result = await this.entityManager.findOneBy(Example, { id: "foo" });

`findOne` and `QueryBuilder.getOne` now return `null` instead of `undefined` when a row is not found

                                                                                                                                                                                                                                                                      If the result row is explicitly compared against `undefined`, it should now be compared against `null` instead.

Other Notes

For more examples, see the pull requests linked to issue #6185.
To ease the eventual merge into the develop branch, the commits from the pull requests mentioned above should first be applied as patches, resolving any conflicts that arise, then lingering issues should be addressed.
The repositories in use by CAMPD that were affected by this vulnerability and that require an update include:
- easey-account-api
- easey-auth-api
- easey-camd-services
- easey-common
- easey-emissions-api
- easey-facilities-api
- easey-mdm-api
- easey-streaming-services

mark-hayward-erg commented 2 months ago

Updates have been deployed to staging (https://campd-stg.app.cloud.gov/). A full regression test of all CAMPD functionality should be done to confirm that the TypeORM upgrade has not introduced any issues.

@lgiannini1 or @mxtomoto1 The priority for this testing is after completing any remaining Sprint 8 testing and before any ECMPS 2.0 testing for Sprint 9.

maxdiebold-erg commented 2 months ago

@ergjustin brought it to my attention that the 11.x version of easey-common is being maintained for CAMPD-specific changes. In the first pull requests for this issue, I had updated the version of easey-common to 17.4.1, which was the version with all TypeORM updates applied, but I've instead applied those updates to the 11.x major version and updated the CAMPD APIs to use that version. The related pull requests are listed below.

lgiannini1 commented 2 months ago

Not sure if this is a TypeORM issue, but for Emissions Based Compliance searches some rows are returned with null Facility Name, Facility ID, Unit ID, and State columns for years past 2020/2021. This issue appears in the UI and on the account-mgmt and streaming-services swagger pages.

Rows with null values returned searching on Wheeling Power Company:

Rows with null values returned searching on Appalachian Power Company in CSV download:

maxdiebold-erg commented 1 month ago

@lgiannini1 I'm not able to reproduce the above issue, could you include the request URL?

lgiannini1 commented 1 month ago

@maxdiebold-erg https://api.epa.gov/easey/staging/account-mgmt/emissions-compliance?ownerOperator=Wheeling%20Power%20Company&page=1&perPage=100

maxdiebold-erg commented 1 month ago

@lgiannini1 That's not looking like a TypeORM issue, although it may still be unintended behavior. When querying emissions compliance, rows from _camddmw.unitfact are left joined to rows in _camddmw.unit_compliancedim using the columns unit_id and op_year. The columns with data for Facility Name, Facility ID, Unit ID, and State all come from the joined _unitfact table, so if a row is not found when joining that table then those columns will be null.

You can see this by executing the following query against the database:

SELECT
    *
FROM
    camddmw.unit_compliance_dim
    LEFT JOIN camddmw.unit_fact USING (unit_id, op_year)
WHERE
    unit_id = 2564
    AND op_year = 2022

mxtomoto1 commented 1 month ago

Regression tested all of CAMPD-stg modules. Findings viewable in attachment. CAMPD Regression.docx

(Any links to pages outside of "campd-stg" were omitted from testing.)

szintgraff commented 1 month ago

I repeated some of the regression testing outline in the document above and found no problems. From what I can tell, this will not have make any visible changes to CAMPD and we just need to make sure the functionality is still working after the changes were made.

In the regressing testing document, there is a note that the contact us page returns an error. Will that be a problem on deployment?

@maheese, @ibarra-michelle, and @j-tafoya: is there anything you want to look at here before I move to ready for deployment?

ibarra-michelle commented 1 month ago

I repeated some of the regression testing outline in the document above and found no problems. From what I can tell, this will not have make any visible changes to CAMPD and we just need to make sure the functionality is still working after the changes were made.

In the regressing testing document, there is a note that the contact us page returns an error. Will that be a problem on deployment?

@maheese, @ibarra-michelle, and @j-tafoya: is there anything you want to look at here before I move to ready for deployment?

@szintgraff, can you confirm what environment you are testing in? e.g., campd-tst, campd-stg, etc. thx.

szintgraff commented 1 month ago

@ibarra-michelle staging: https://campd-stg.app.cloud.gov/

US-EPA-CAMD / easey-ui