Skip to content

FIX - Export segmentation in OHIF Viewer#121

Open
erinaldidb wants to merge 3 commits intomainfrom
bugfix/save_segmentation_ohif
Open

FIX - Export segmentation in OHIF Viewer#121
erinaldidb wants to merge 3 commits intomainfrom
bugfix/save_segmentation_ohif

Conversation

@erinaldidb
Copy link
Collaborator

Refactor SQL insert statement in persistMetadata function to remove thumbnail field

@erinaldidb erinaldidb requested a review from dmoore247 July 11, 2025 15:57
Copy link
Collaborator

@dmoore247 dmoore247 left a comment

Choose a reason for hiding this comment

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

Tighten up security

@@ -2103,11 +2103,11 @@ async function persistMetadata(databricksClient, warehouseId, pixelsTable, datas
"warehouse_id": warehouseId,
"statement": `INSERT INTO ${pixelsTable}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be in client side code: INSERT INTO ${pixelsTable}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The client prepares the statement and calls the databricks app apis, the execution of the statement and the call of the sql warehouse api is happening in the databricks app. Any suggestion to improve it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@erinaldidb The recommendation is to pass simple parameters to the server.
Passing commands to the server requires complex parsing and safety checks.
Passing commands invites people to test diff command strings and either succeed in corrupting data or running UDFs or denial of service functions.

extension, file_type, path_tags, is_anon, meta, thumbnail)
extension, file_type, path_tags, is_anon, meta)
VALUES (
'dbfs:/${dataset.path}', to_timestamp(unix_timestamp('${dataset.datetime}', 'yyyyMMddHHmmss')), '${dataset.length}', 'dbfs:/${dataset.path}', '${dataset.path}', '/${dataset.path}',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Leaving tables and paths open the to the client to overwrite with malicious values can be dangerous.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any changes in the client can always be overwritten, the security measures should be applied on server side ( databricks app )

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants