From: Paul <pa...@qu...> - 2007-03-27 21:57:38
|
This patch should be reverted imo. String_display already calls htmlspecialchars + others function string_display( $p_string ) { $p_string = string_strip_hrefs( $p_string ); $p_string = string_html_specialchars( $p_string ); $p_string = string_restore_valid_html_tags( $p_string ); $p_string = string_preserve_spaces_at_bol( $p_string ); $p_string = string_nl2br( $p_string ); Whats the purpose of this ? Am i missing something? Paul -----Original Message----- From: man...@li... [mailto:man...@li...] On Behalf Of zakman Sent: 23 March 2007 22:56 To: man...@li... Subject: [mantisbt-cvs] mantisbt adm_config_report.php,1.6,1.7 Update of /cvsroot/mantisbt/mantisbt In directory sc8-pr-cvs7.sourceforge.net:/tmp/cvs-serv29617 Modified Files: adm_config_report.php Log Message: Fixed: 0007545: config values are not escaped Index: adm_config_report.php =================================================================== RCS file: /cvsroot/mantisbt/mantisbt/adm_config_report.php,v retrieving revision 1.6 retrieving revision 1.7 diff -u -d -r1.6 -r1.7 --- adm_config_report.php 6 Mar 2007 07:05:18 -0000 1.6 +++ adm_config_report.php 23 Mar 2007 22:55:55 -0000 1.7 @@ -1,4 +1,4 @@ -<?php +CONFIG_TYPE_STRING<?php # Mantis - a php based bugtracking system # Copyright (C) 2000 - 2002 Kenzaburo Ito - ke...@30... # Copyright (C) 2002 - 2006 Mantis Team - man...@li... @@ -41,7 +41,7 @@ return; case CONFIG_TYPE_STRING: $t_value = config_eval( $p_value ); - echo "'" . string_display( $t_value ) . "'"; + echo htmlspecialchars( "'$t_value'" ); return; case CONFIG_TYPE_COMPLEX: $t_value = unserialize( $p_value ); ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV _______________________________________________ mantisbt-cvs mailing list man...@li... https://lists.sourceforge.net/lists/listinfo/mantisbt-cvs |
From: Paul <pa...@qu...> - 2007-03-28 18:05:02
|
P.S. apologies for not setting up mail client properly on new pc yet. To be honest, probably not, i've not done a proper analysis. In some cases, the additional functions might be helpful, in some cases, I could imagine them to be a hinderance - (what happens if you want to set a config variable to be <b>MyTitle</b> in both cases?) I was more looking at the original issue: The attached patch is to do the following change: - echo "'$t_value'"; + echo htmlspecialchars( "'$t_value'" ); You fixed private bug #7795 and #7784 in http://mantisbt.cvs.sourceforge.net/mantisbt/mantisbt/adm_config_report.php? r1=1.5&r2=1.6 by replacing the echo with string_display. We've now fixed the same issue (previously fixed) by removing string_display and replacing it with htmlspecialchars, which incidently, the call to htmlspecialchars should be replaced with string_html_specialchars as it deals with the case that a user is on php < 4.1.0 and htmlspecialchars does not exist. The seemingly lack of care at looking at the issue, patch, existing API's is what I took issue with. I'd previously read the change to default config patch, which again, was slightly incorrect. Obviously, it's hard for someone to learn a new codebase, and zakman's help is appreciated, however, i'm also quite conscious that the last few times with done a X release, we normally follow up with a X.1 release, as someone broke something 1-2 weeks before the release. (IIRC, i've done that a couple of times :)) In terms of the config values, at a guess, string_display is probably wrong, string_html_specialchars is probably more appropriate, but even then might not need the best choice - can any config values be long paragraphs? Paul -----Original Message----- From: man...@li... [mailto:man...@li...] On Behalf Of Victor Boctor Sent: 28 March 2007 18:20 To: developer discussions Subject: Re: [mantisbt-dev] [mantisbt-cvs] mantisbt adm_config_report.php, 1.6, 1.7 Hi Paul, I am wondering if we need to call all of these on configuration values. For example, do we need to enable some of the html tags? Do we need to hyper link URLs? Do we need to hyperlink issue numbers? Interested to know your thoughts. I don't have the code handy now. On 3/27/07, Paul <pa...@qu...> wrote: > This patch should be reverted imo. > > String_display already calls htmlspecialchars + others > > function string_display( $p_string ) { > $p_string = string_strip_hrefs( $p_string ); > $p_string = string_html_specialchars( $p_string ); > $p_string = string_restore_valid_html_tags( $p_string ); > $p_string = string_preserve_spaces_at_bol( $p_string ); > $p_string = string_nl2br( $p_string ); > > Whats the purpose of this ? Am i missing something? > > Paul > > > > -----Original Message----- > From: man...@li... > [mailto:man...@li...] On Behalf Of zakman > Sent: 23 March 2007 22:56 > To: man...@li... > Subject: [mantisbt-cvs] mantisbt adm_config_report.php,1.6,1.7 > > Update of /cvsroot/mantisbt/mantisbt > In directory sc8-pr-cvs7.sourceforge.net:/tmp/cvs-serv29617 > > Modified Files: > adm_config_report.php > Log Message: > Fixed: 0007545: config values are not escaped > > Index: adm_config_report.php > =================================================================== > RCS file: /cvsroot/mantisbt/mantisbt/adm_config_report.php,v > retrieving revision 1.6 > retrieving revision 1.7 > diff -u -d -r1.6 -r1.7 > --- adm_config_report.php 6 Mar 2007 07:05:18 -0000 1.6 > +++ adm_config_report.php 23 Mar 2007 22:55:55 -0000 1.7 > @@ -1,4 +1,4 @@ > -<?php > +CONFIG_TYPE_STRING<?php > # Mantis - a php based bugtracking system > # Copyright (C) 2000 - 2002 Kenzaburo Ito - ke...@30... > # Copyright (C) 2002 - 2006 Mantis Team - > man...@li... > @@ -41,7 +41,7 @@ > return; > case CONFIG_TYPE_STRING: > $t_value = config_eval( $p_value ); > - echo "'" . string_display( $t_value ) . "'"; > + echo htmlspecialchars( "'$t_value'" ); > return; > case CONFIG_TYPE_COMPLEX: > $t_value = unserialize( $p_value ); > > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys-and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > mantisbt-cvs mailing list > man...@li... > https://lists.sourceforge.net/lists/listinfo/mantisbt-cvs > > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys-and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > mantisbt-dev mailing list > man...@li... > https://lists.sourceforge.net/lists/listinfo/mantisbt-dev > ------------------------------------------------------------------------- Take Surveys. Earn Cash. Influence the Future of IT Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys-and earn cash http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV _______________________________________________ mantisbt-dev mailing list man...@li... https://lists.sourceforge.net/lists/listinfo/mantisbt-dev |
From: Victor B. <vb...@gm...> - 2007-04-01 06:31:44
|
Hi Paul, I updated the fix to: echo string_nl2br( string_html_specialchars( "'$t_value'" ) ); On 3/28/07, Paul <pa...@qu...> wrote: > P.S. apologies for not setting up mail client properly on new pc yet. > > To be honest, probably not, i've not done a proper analysis. In some cases, > the additional functions might be helpful, in some cases, I could imagine > them to be a hinderance - (what happens if you want to set a config variable > to be <b>MyTitle</b> in both cases?) > > I was more looking at the original issue: > The attached patch is to do the following change: > > - echo "'$t_value'"; > + echo htmlspecialchars( "'$t_value'" ); > > You fixed private bug #7795 and #7784 in > http://mantisbt.cvs.sourceforge.net/mantisbt/mantisbt/adm_config_report.php? > r1=1.5&r2=1.6 by replacing the echo with string_display. > > We've now fixed the same issue (previously fixed) by removing string_display > and replacing it with htmlspecialchars, which incidently, the call to > htmlspecialchars should be replaced with string_html_specialchars as it > deals with the case that a user is on php < 4.1.0 and htmlspecialchars does > not exist. > > The seemingly lack of care at looking at the issue, patch, existing API's is > what I took issue with. I'd previously read the change to default config > patch, which again, was slightly incorrect. > > Obviously, it's hard for someone to learn a new codebase, and zakman's help > is appreciated, however, i'm also quite conscious that the last few times > with done a X release, we normally follow up with a X.1 release, as someone > broke something 1-2 weeks before the release. (IIRC, i've done that a couple > of times :)) > > In terms of the config values, at a guess, string_display is probably wrong, > string_html_specialchars is probably more appropriate, but even then might > not need the best choice - can any config values be long paragraphs? > > Paul > > -----Original Message----- > From: man...@li... > [mailto:man...@li...] On Behalf Of Victor > Boctor > Sent: 28 March 2007 18:20 > To: developer discussions > Subject: Re: [mantisbt-dev] [mantisbt-cvs] mantisbt adm_config_report.php, > 1.6, 1.7 > > Hi Paul, > > I am wondering if we need to call all of these on configuration > values. For example, do we need to enable some of the html tags? Do > we need to hyper link URLs? Do we need to hyperlink issue numbers? > > Interested to know your thoughts. I don't have the code handy now. > > On 3/27/07, Paul <pa...@qu...> wrote: > > This patch should be reverted imo. > > > > String_display already calls htmlspecialchars + others > > > > function string_display( $p_string ) { > > $p_string = string_strip_hrefs( $p_string ); > > $p_string = string_html_specialchars( $p_string ); > > $p_string = string_restore_valid_html_tags( $p_string ); > > $p_string = string_preserve_spaces_at_bol( $p_string ); > > $p_string = string_nl2br( $p_string ); > > > > Whats the purpose of this ? Am i missing something? > > > > Paul > > > > > > > > -----Original Message----- > > From: man...@li... > > [mailto:man...@li...] On Behalf Of zakman > > Sent: 23 March 2007 22:56 > > To: man...@li... > > Subject: [mantisbt-cvs] mantisbt adm_config_report.php,1.6,1.7 > > > > Update of /cvsroot/mantisbt/mantisbt > > In directory sc8-pr-cvs7.sourceforge.net:/tmp/cvs-serv29617 > > > > Modified Files: > > adm_config_report.php > > Log Message: > > Fixed: 0007545: config values are not escaped > > > > Index: adm_config_report.php > > =================================================================== > > RCS file: /cvsroot/mantisbt/mantisbt/adm_config_report.php,v > > retrieving revision 1.6 > > retrieving revision 1.7 > > diff -u -d -r1.6 -r1.7 > > --- adm_config_report.php 6 Mar 2007 07:05:18 -0000 1.6 > > +++ adm_config_report.php 23 Mar 2007 22:55:55 -0000 1.7 > > @@ -1,4 +1,4 @@ > > -<?php > > +CONFIG_TYPE_STRING<?php > > # Mantis - a php based bugtracking system > > # Copyright (C) 2000 - 2002 Kenzaburo Ito - ke...@30... > > # Copyright (C) 2002 - 2006 Mantis Team - > > man...@li... > > @@ -41,7 +41,7 @@ > > return; > > case CONFIG_TYPE_STRING: > > $t_value = config_eval( $p_value ); > > - echo "'" . string_display( $t_value ) . > "'"; > > + echo htmlspecialchars( "'$t_value'" ); > > return; > > case CONFIG_TYPE_COMPLEX: > > $t_value = unserialize( $p_value ); > > > > > > ------------------------------------------------------------------------- > > Take Surveys. Earn Cash. Influence the Future of IT > > Join SourceForge.net's Techsay panel and you'll get the chance to share > your > > opinions on IT & business topics through brief surveys-and earn cash > > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > > _______________________________________________ > > mantisbt-cvs mailing list > > man...@li... > > https://lists.sourceforge.net/lists/listinfo/mantisbt-cvs > > > > > > ------------------------------------------------------------------------- > > Take Surveys. Earn Cash. Influence the Future of IT > > Join SourceForge.net's Techsay panel and you'll get the chance to share > your > > opinions on IT & business topics through brief surveys-and earn cash > > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > > _______________________________________________ > > mantisbt-dev mailing list > > man...@li... > > https://lists.sourceforge.net/lists/listinfo/mantisbt-dev > > > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys-and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > mantisbt-dev mailing list > man...@li... > https://lists.sourceforge.net/lists/listinfo/mantisbt-dev > > > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys-and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > mantisbt-dev mailing list > man...@li... > https://lists.sourceforge.net/lists/listinfo/mantisbt-dev > |
From: Victor B. <vb...@gm...> - 2007-03-28 17:19:44
|
Hi Paul, I am wondering if we need to call all of these on configuration values. For example, do we need to enable some of the html tags? Do we need to hyper link URLs? Do we need to hyperlink issue numbers? Interested to know your thoughts. I don't have the code handy now. On 3/27/07, Paul <pa...@qu...> wrote: > This patch should be reverted imo. > > String_display already calls htmlspecialchars + others > > function string_display( $p_string ) { > $p_string = string_strip_hrefs( $p_string ); > $p_string = string_html_specialchars( $p_string ); > $p_string = string_restore_valid_html_tags( $p_string ); > $p_string = string_preserve_spaces_at_bol( $p_string ); > $p_string = string_nl2br( $p_string ); > > Whats the purpose of this ? Am i missing something? > > Paul > > > > -----Original Message----- > From: man...@li... > [mailto:man...@li...] On Behalf Of zakman > Sent: 23 March 2007 22:56 > To: man...@li... > Subject: [mantisbt-cvs] mantisbt adm_config_report.php,1.6,1.7 > > Update of /cvsroot/mantisbt/mantisbt > In directory sc8-pr-cvs7.sourceforge.net:/tmp/cvs-serv29617 > > Modified Files: > adm_config_report.php > Log Message: > Fixed: 0007545: config values are not escaped > > Index: adm_config_report.php > =================================================================== > RCS file: /cvsroot/mantisbt/mantisbt/adm_config_report.php,v > retrieving revision 1.6 > retrieving revision 1.7 > diff -u -d -r1.6 -r1.7 > --- adm_config_report.php 6 Mar 2007 07:05:18 -0000 1.6 > +++ adm_config_report.php 23 Mar 2007 22:55:55 -0000 1.7 > @@ -1,4 +1,4 @@ > -<?php > +CONFIG_TYPE_STRING<?php > # Mantis - a php based bugtracking system > # Copyright (C) 2000 - 2002 Kenzaburo Ito - ke...@30... > # Copyright (C) 2002 - 2006 Mantis Team - > man...@li... > @@ -41,7 +41,7 @@ > return; > case CONFIG_TYPE_STRING: > $t_value = config_eval( $p_value ); > - echo "'" . string_display( $t_value ) . "'"; > + echo htmlspecialchars( "'$t_value'" ); > return; > case CONFIG_TYPE_COMPLEX: > $t_value = unserialize( $p_value ); > > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys-and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > mantisbt-cvs mailing list > man...@li... > https://lists.sourceforge.net/lists/listinfo/mantisbt-cvs > > > ------------------------------------------------------------------------- > Take Surveys. Earn Cash. Influence the Future of IT > Join SourceForge.net's Techsay panel and you'll get the chance to share your > opinions on IT & business topics through brief surveys-and earn cash > http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV > _______________________________________________ > mantisbt-dev mailing list > man...@li... > https://lists.sourceforge.net/lists/listinfo/mantisbt-dev > |