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

Do not wrap __class__ attribute of SafeStringWrapper #3429

Merged
merged 1 commit into from Jan 19, 2017

Conversation

Projects
None yet
3 participants
@nsoranzo
Copy link
Member

commented Jan 16, 2017

Fix #3353 by not recursing on the parent classes, which may lead to an abstract class.
Also fix a method name of object class.

This is an alternative to #3359, which fixes the issue only for the classes derived from TabularData, but any datatype derived from an abstract class would exhibit the same problem, e.g. if samtool_filter2.xml is modified with this patch:

diff --git a/tools/samtool_filter2/samtool_filter2.xml b/tools/samtool_filter2/samtool_filter2.xml
index a5d2b9a..bd2f2b2 100644
--- a/tools/samtool_filter2/samtool_filter2.xml
+++ b/tools/samtool_filter2/samtool_filter2.xml
@@ -16,6 +16,9 @@
 #elif isinstance($input1.datatype, $__app__.datatypes_registry.get_datatype_by_extension('sam').__class__):
   #set $input = 'input.sam'
   ln -s "$input1" $input &&
+#elif isinstance($input1.datatype, $__app__.datatypes_registry.get_datatype_by_extension('hmm3').__class__):
+  #set $input = 'input'
+  ln -s "$input1" $input &&
 #end if
 samtools view $possibly_select_inverse "$output1" $header
 
@@ -66,7 +69,7 @@ samtools view $possibly_select_inverse "$output1" $header
 ]]>
   </command>
   <inputs>
-    <param name="input1" type="data" format="sam,bam" label="SAM or BAM file to filter" />
+    <param name="input1" type="data" format="sam,bam,hmm3" label="SAM or BAM file to filter" />
     <param name="header" type="select" label="Header in output">
       <option value="-h">Include Header</option>
       <option value="">Exclude Header</option>
@@ -134,7 +137,7 @@ samtools view $possibly_select_inverse "$output1" $header
   </outputs>
   <tests>
     <test>
-      <param name="input1" value="bam_to_sam_in2.sam" ftype="sam" />
+      <param name="input1" value="bam_to_sam_in2.sam" ftype="hmm3" />
       <param name="header" value=""/>
       <param name="filter" value="yes"/>
       <param name="reqBits" value="0x0080"/>

the problem reappears.

Do not wrap __class__ attribute of SafeStringWrapper
Fix #3353 by not recursing on the parent classes, which may lead to
an abstract class.
Also fix a method name of "object" class.
@jmchilton

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

I'm always worried about changing this piece of code since it is so complicated, has large security implications, and no tests - but I think this looks really good. I can't imagine a case where we would want to walk down __class__ and then still expect escaped values.

Big 👍 from me.

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

I'm always worried about changing this piece of code since it is so complicated, has large security implications, and no tests - but I think this looks really good. I can't imagine a case where we would want to walk down __class__ and then still expect escaped values.

Thanks @jmchilton, indeed it took me a whole day of step-by-step debugging to find out what was happening and fix it!

@dannon

dannon approved these changes Jan 19, 2017

@dannon dannon merged commit b4475ba into galaxyproject:dev Jan 19, 2017

4 checks passed

api test Build finished. 256 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 134 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 580 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

Merging forward now

@dannon

This comment has been minimized.

Copy link
Member

commented Jan 19, 2017

@nsoranzo This was targeted at dev.

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2017

Damn, targeted the wrong branch! Thanks for noticing and opening the backport PR @dannon!

Edit: I've updated the PR title to reflect the reality.

@nsoranzo nsoranzo changed the title [16.10] Do not wrap __class__ attribute of SafeStringWrapper Do not wrap __class__ attribute of SafeStringWrapper Jan 19, 2017

@nsoranzo nsoranzo deleted the nsoranzo:release_16.10_fix_3353 branch Jan 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.