[Analyze-1468] Add d2e-webapi function logic #663
Conversation
…d_webapi_function_logic
…d_webapi_function_logic
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| params.append("datasetId", datasetId); | ||
|
|
||
| const url = `${this.baseURL}/user-artifact/${UserArtifactServiceNames.ATLAS_COHORT_DEFINITIONS}/${atlasCohortDefinitionId}`; | ||
| const result = await axios.get(url, { params, ...options }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 2 days ago
To fix the SSRF vulnerability, we need to validate and sanitize the atlasCohortDefinitionId before using it in the URL. Since atlasCohortDefinitionId is expected to be a number, we can ensure it is a valid number and within an acceptable range. Additionally, we should use a more secure method to construct the URL, ensuring that only valid and expected paths are used.
- Validate the
atlasCohortDefinitionIdto ensure it is a positive integer. - Construct the URL using a template string with validated parameters.
| @@ -74,2 +74,5 @@ | ||
| ): Promise<IUserArtifactAtlasCohortDefinitionDto> { | ||
| if (!Number.isInteger(atlasCohortDefinitionId) || atlasCohortDefinitionId <= 0) { | ||
| throw new Error("Invalid atlasCohortDefinitionId"); | ||
| } | ||
| try { |
| order by | ||
| CONCEPT_NAME ASC | ||
| `; | ||
| const result = await client.query(sql); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 2 days ago
To fix the problem, we need to ensure that user input is safely embedded into the SQL query. Since the current database does not support array SQL parameter types, we can manually construct the query with placeholders for each element in the searchConceptIds array and then pass the values as parameters.
- Modify the
getConceptsFromIdentifiersmethod infunctions/d2e-webapi/src/dao/cachedb.dao.tsto construct the query with placeholders. - Pass the
searchConceptIdsarray elements as parameters to the query.
| @@ -59,2 +59,3 @@ | ||
|
|
||
| const placeholders = searchConceptIds.map((_, index) => `$${index + 1}`).join(", "); | ||
| const sql = ` | ||
| @@ -74,3 +75,3 @@ | ||
| where | ||
| CONCEPT_ID in (${searchConceptIds.join(", ")}) | ||
| CONCEPT_ID in (${placeholders}) | ||
| order by | ||
| @@ -78,3 +79,3 @@ | ||
| `; | ||
| const result = await client.query(sql); | ||
| const result = await client.query(sql, searchConceptIds); | ||
| return result.rows; |
| ", " | ||
| )}) and descendant_concept_id in (${descendants.join(", ")}); | ||
| `; | ||
| const result = await client.query(sql); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 2 days ago
To fix the problem, we need to ensure that user-provided values are safely embedded into SQL queries. Since the current database does not support array SQL parameter types, we can use a combination of query parameterization for individual values and validation to ensure the integrity of the data.
- Validation: Ensure that the user-provided values (
ancestorsanddescendants) are arrays of numbers and do not contain any malicious content. - Query Construction: Construct the SQL query using parameterized queries for individual values to prevent SQL injection.
| @@ -95,5 +95,13 @@ | ||
| try { | ||
| // TODO: Move ancestors and descendants as a sql parameter instead of being in the sql statement itself. | ||
| // ancestors and descendants has to be in sql statement now as cachedb does not support array sql parameter types | ||
| // https://github.com/alp-os/internal/issues/1411 | ||
| // Validate ancestors and descendants to ensure they are arrays of numbers | ||
| if (!Array.isArray(ancestors) || !ancestors.every(Number.isInteger)) { | ||
| throw new Error("Invalid ancestors array"); | ||
| } | ||
| if (!Array.isArray(descendants) || !descendants.every(Number.isInteger)) { | ||
| throw new Error("Invalid descendants array"); | ||
| } | ||
|
|
||
| // Construct the SQL query using parameterized queries for individual values | ||
| const ancestorParams = ancestors.map((_, i) => `$${i + 1}`).join(", "); | ||
| const descendantParams = descendants.map((_, i) => `$${i + ancestors.length + 1}`).join(", "); | ||
| const sql = ` | ||
| @@ -101,7 +109,5 @@ | ||
| from ${vocabSchemaName}.concept_ancestor | ||
| where ancestor_concept_id in (${ancestors.join( | ||
| ", " | ||
| )}) and descendant_concept_id in (${descendants.join(", ")}); | ||
| where ancestor_concept_id in (${ancestorParams}) and descendant_concept_id in (${descendantParams}); | ||
| `; | ||
| const result = await client.query(sql); | ||
| const result = await client.query(sql, [...ancestors, ...descendants]); | ||
| return result.rows; |
| ", " | ||
| )}); | ||
| `; | ||
| const result = await client.query(sql); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 2 days ago
To fix the problem, we need to ensure that user-provided data is safely embedded into the SQL query. Since the current database does not support array SQL parameter types, we can use a library like sqlstring to escape the user input before embedding it into the query string. This will prevent SQL injection attacks by ensuring that the user input is treated as a literal value.
- Import the
sqlstringlibrary. - Use
sqlstring.escapeto escape the user-provided data before embedding it into the SQL query string.
| @@ -2,2 +2,3 @@ | ||
| import pg from "pg"; | ||
| import SqlString from "sqlstring"; | ||
| import { | ||
| @@ -124,6 +125,5 @@ | ||
| // https://github.com/alp-os/internal/issues/1411 | ||
| const escapedConceptIds = searchConceptIds.map(id => SqlString.escape(id)).join(", "); | ||
| const sql = ` | ||
| select concept_id_1, concept_id_2, relationship_id from ${vocabSchemaName}.concept_recommended WHERE concept_id_1 IN (${searchConceptIds.join( | ||
| ", " | ||
| )}); | ||
| select concept_id_1, concept_id_2, relationship_id from ${vocabSchemaName}.concept_recommended WHERE concept_id_1 IN (${escapedConceptIds}); | ||
| `; | ||
| @@ -152,2 +152,3 @@ | ||
| // https://github.com/alp-os/internal/issues/1411 | ||
| const escapedConceptIds = conceptIds.map(id => SqlString.escape(id)).join(", "); | ||
| const sql = ` | ||
| @@ -156,3 +157,3 @@ | ||
| WHERE | ||
| concept_id IN (${conceptIds.join(", ")}) | ||
| concept_id IN (${escapedConceptIds}) | ||
| ${invalidReasonWhereClause} |
| @@ -2264,2 +2264,5 @@ | ||
| } | ||
| }, | ||
| "dependencies": { | ||
| "sqlstring": "^2.3.3" | ||
| } |
| Package | Version | Security advisories |
| sqlstring (npm) | 2.3.3 | None |
| order by domain_id, vocabulary_id; | ||
| `; | ||
|
|
||
| const result = await client.query<ICachedbConcept>(sql); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
resolves: https://github.com/data2evidence/internal/issues/1468
analytics-svc
rename cohort definitionto `update cohort definitionterminology-svc
resolveConceptSetExpressionto get included concepts for a list of concept ids for webapi, reusing logic fromincluded-conceptsendpoint.d2e-webapi
preHandlerto expect token and datasetid to be in request headers.conceptset, and endpoints requiring more complicated webapi logic, e.g/check, /sql.Followup tasks:
Merge Checklist
Please cross check this list if additions / modifications needs to be done on top of your core changes and tick them off. Reviewer can as well glance through and help the developer if something is missed out.
developbranch)