neurobooth / neurobooth-os

Python package for digital phenotyping data synchronization and acquisition
http://neurobooth.github.io
BSD 3-Clause "New" or "Revised" License
6 stars 6 forks source link

Minor gui issues #408

Closed lwhite1 closed 4 months ago

lwhite1 commented 5 months ago

Two small usability enhancements:

  1. Strips leading and trailing whitespace from the subject's first and last name before searching
  2. Saves any unsaved notes on terminate servers. If there is note text, but no task has been selected, a popup tells the user to delete the note or select a task. After the unsaved note is fixed, terminate server can be performed.
lwhite1 commented 4 months ago

agreed, but:

On Wed, May 8, 2024 at 12:32 PM Brandon Oubre @.***> wrote:

@.**** approved this pull request.

LGTM

In neurobooth_os/iout/metadator.py https://github.com/neurobooth/neurobooth-os/pull/408#discussion_r1594311880 :

 table_subject = Table("subject", conn=conn)

subject_df = table_subject.query(

  • where=f"LOWER(first_name_birth)=LOWER('{first_name}') AND LOWER(last_name_birth)=LOWER('{last_name}')"
  • where=f"LOWER(first_name_birth)=LOWER('{f_name}') AND LOWER(last_name_birth)=LOWER('{l_name}')"

I'm not suggesting we fix at the moment given the use case, but looks like SQL injection potential. Could be the source of some special character issues.

— Reply to this email directly, view it on GitHub https://github.com/neurobooth/neurobooth-os/pull/408#pullrequestreview-2046131227, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA2FPAT3TTDFUPT7FWT66XDZBJHQVAVCNFSM6AAAAABHNFZ2DGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANBWGEZTCMRSG4 . You are receiving this because you authored the thread.Message ID: @.***>

boubre commented 4 months ago

agreed, but: - a fix would mean rewriting the terra query code - the sql injection would require access to the machine (either directly or via chrome) - we might be going to change the gui to query by id - (unfortunately) there's probably a ton of these issues

Agreed on all points.