Skip to content

feat: building selection criteria file transfer script - #1220

Open
YazeedLoonat wants to merge 2 commits into
mainfrom
1217/building-selection-file-transfer-script
Open

feat: building selection criteria file transfer script#1220
YazeedLoonat wants to merge 2 commits into
mainfrom
1217/building-selection-file-transfer-script

Conversation

@YazeedLoonat

Copy link
Copy Markdown
Collaborator

This PR addresses #1217

  • Addresses the issue in full

Description

This adds a new transfer script that transfer building selection criteria

How Can This Be Tested/Reviewed?

You'll need to run the jurisdiction transfer scripts locally including the new script
you should have the building selection criteria files uploaded to AWS and attached the listing

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

Comment on lines +1184 to +1181
const image = await axios.get(
`https://res.cloudinary.com/${dataTransferDTO.cloudinaryName}/image/upload/${oldAssetInfo[j].file_id}.pdf`,
{
responseType: 'arraybuffer',
},
);

Check failure

Code scanning / CodeQL

Server-side request forgery

The [URL](1) of this request depends on a [user-provided value](2).

Copilot Autofix

AI over 1 year ago

To fix the SSRF vulnerability, we need to validate and restrict the cloudinaryName value to a predefined set of allowed values (allow-list). This ensures that only trusted subdomains are used in the URL construction. The best way to implement this is to:

  1. Define an allow-list of valid cloudinaryName values.
  2. Validate the cloudinaryName against this allow-list before using it in the URL.
  3. Reject or handle invalid values appropriately.

The validation should be implemented in the transferListingBuildingSelectionCriteriaAssetsData method in api/src/services/script-runner.service.ts. If the cloudinaryName is invalid, an exception should be thrown to prevent further processing.


Suggested changeset 1
api/src/services/script-runner.service.ts

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/api/src/services/script-runner.service.ts b/api/src/services/script-runner.service.ts
--- a/api/src/services/script-runner.service.ts
+++ b/api/src/services/script-runner.service.ts
@@ -1175,2 +1175,6 @@
         // pull down image from cloudinary
+        const allowedCloudinaryNames = ['trusted-cloudinary-name1', 'trusted-cloudinary-name2'];
+        if (!allowedCloudinaryNames.includes(dataTransferDTO.cloudinaryName)) {
+          throw new BadRequestException('Invalid cloudinaryName provided.');
+        }
         const image = await axios.get(
EOF
@@ -1175,2 +1175,6 @@
// pull down image from cloudinary
const allowedCloudinaryNames = ['trusted-cloudinary-name1', 'trusted-cloudinary-name2'];
if (!allowedCloudinaryNames.includes(dataTransferDTO.cloudinaryName)) {
throw new BadRequestException('Invalid cloudinaryName provided.');
}
const image = await axios.get(
Copilot is powered by AI and may make mistakes. Always verify output.
@YazeedLoonat
YazeedLoonat force-pushed the 1217/building-selection-file-transfer-script branch from cfa0d15 to d584f19 Compare April 29, 2025 17:44
@ludtkemorgan ludtkemorgan removed the high priority High priority issues label Jun 5, 2025
@mcgarrye

Copy link
Copy Markdown
Collaborator

Is this still needed?

`${dataTransferDTO.jurisdiction} county doesn't exist in foreign database`,
);
}
const listingTransferMap = await this.prisma.listingTransferMap.findMany({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused on what the purpose of this listingTransferMap layer. The listing_id is always the same as the old_id and doesn't take jurisdiction into account. So when we go to use this for San Jose it will also try to do the listings of the Alameda listings we transferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants