Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRI Violated scans] Have the minc file linked to brainbrowser (Redmine 10928) #2219

Merged
merged 6 commits into from
Oct 4, 2016

Conversation

gluneau
Copy link
Contributor

@gluneau gluneau commented Sep 28, 2016

The MRI Violated Scans now has a new column named MincFileViolated with a link that opens the scan in Brain Browser even if it is in the trashbin directory.

@gluneau gluneau added the Category: Feature PR or issue that aims to introduce a new feature label Sep 28, 2016
@gluneau gluneau added this to the 17.0 milestone Sep 28, 2016
@codecov-io
Copy link

codecov-io commented Sep 28, 2016

Current coverage is 13.80% (diff: 0.00%)

Merging #2219 into 17.0-dev will increase coverage by <.01%

@@           17.0-dev      #2219   diff @@
==========================================
  Files           121        121          
  Lines         20191      20196     +5   
  Methods        1133       1133          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2787       2788     +1   
- Misses        17404      17408     +4   
  Partials          0          0          

Powered by Codecov. Last update 4b94cee...ace9db3

@gluneau gluneau added the Passed manual tests PR has been successfully tested by at least one peer label Oct 3, 2016
@gluneau
Copy link
Contributor Author

gluneau commented Oct 3, 2016

Tested on IBIS production

Copy link
Collaborator

@driusan driusan left a comment

Choose a reason for hiding this comment

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

Some of the functions in the select statements seem overly complex. They could use proper quoting and comments.

@@ -171,6 +174,7 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
c.ProjectID as Project,
s.SubprojectID as Subproject,
minc_location as MincFile,
CONCAT_WS('','" . $dir_path . "','trashbin/',SUBSTRING_INDEX(minc_location, '/', -2)) as MincFileViolated,
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 use the $DB->quote function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -195,6 +199,7 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
c.ProjectID as Project,
s.SubprojectID as Subproject,
MincFile,
IF(INSTR(`MincFile`, 'assembly'), IF(LEFT(`MincFile`, 8) = 'assembly',CONCAT_WS('','" . $dir_path . "',`MincFile`),`MincFile`), CONCAT_WS('','" . $dir_path . "','trashbin/',SUBSTRING_INDEX(MincFile, '/', -2))) as MincFileViolated,
Copy link
Collaborator

@driusan driusan Oct 3, 2016

Choose a reason for hiding this comment

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

This line seems really complex and hard to understand. Does whatever it's doing need to be in SQL or can it be done in code?

Either way, it should at least have a comment explaining it. (dir_path should also be quoted using the db quote function)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments added.

@driusan driusan added Release: Add to release notes PR whose changes should be highlighted in the release notes and removed Request Code Review labels Oct 3, 2016
@driusan driusan merged commit 1bf483f into aces:17.0-dev Oct 4, 2016
driusan added a commit to driusan/Loris that referenced this pull request May 10, 2017
…r (Redmine 10928) (aces#2219)"

This reverts commit 1bf483f.

The addition of the parameter "minc_location" parameter added by this commit introduces
a severe security hole, where user input is getting passed directly to the PHP
passthru command.
driusan added a commit that referenced this pull request May 10, 2017
…r (Redmine 10928) (#2219)" (#2794)

This reverts commit 1bf483f.

The addition of the parameter "minc_location" parameter added by this commit introduces
a severe security hole, where user input is getting passed directly to the PHP
passthru command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature Passed manual tests PR has been successfully tested by at least one peer Release: Add to release notes PR whose changes should be highlighted in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants