You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Mark Phippard <ma...@gmail.com> on 2009/10/14 12:16:18 UTC

RE: svn commit: r40008 - trunk/subversion/mod_dav_svn

> -      if (gen_html)
> -        ap_fputs(output, bb,
> -                 " </ul>\n <hr noshade><em>Powered by "
> -                 "<a href=\"http://subversion.tigris.org/\">Subversion</a> "
> -                 "version " SVN_VERSION "."
> -                 "</em>\n</body></html>");
> +      if (gen_html
> +          && (strcmp(ap_psignature("FOO", resource->info->r), "") != 0))
> +        {
> +          /* Apache's signature generation code didn't eat our prefix.
> +             ServerSignature must be enabled.  Print our version info.  */
> +          ap_fputs(output, bb,
> +                   " </ul>\n <hr noshade><em>Powered by "
> +                   "<a href=\"http://subversion.tigris.org/\">Subversion</a> "
> +                   "version " SVN_VERSION "."
> +                   "</em>\n</body></html>");
> +        }
>        else
>          ap_fputs(output, bb, "  </index>\n</svn>\n");

I do not really understand what that check is doing related to FOO, but I trust it is right.  That said, the change itself is not right.  It is suppressing the closing </body> and </html> tags, which means the page will be invalid HTML.  It should only be supressing the "Powered by Subversion version X.Y.Z" line.

Mark

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2407512

Re: svn commit: r40008 - trunk/subversion/mod_dav_svn

Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Oct 14, 2009, at 8:21 AM, Senthil Kumaran S wrote:

> Mark Phippard wrote:
>>> -      if (gen_html)
>>> -        ap_fputs(output, bb,
>>> -                 " </ul>\n <hr noshade><em>Powered by "
>>> -                 "<a href=\"http://subversion.tigris.org/ 
>>> \">Subversion</a> "
>>> -                 "version " SVN_VERSION "."
>>> -                 "</em>\n</body></html>");
>>> +      if (gen_html
>>> +          && (strcmp(ap_psignature("FOO", resource->info->r),  
>>> "") != 0))
>>> +        {
>>> +          /* Apache's signature generation code didn't eat our  
>>> prefix.
>>> +             ServerSignature must be enabled.  Print our version  
>>> info.  */
>>> +          ap_fputs(output, bb,
>>> +                   " </ul>\n <hr noshade><em>Powered by "
>>> +                   "<a href=\"http://subversion.tigris.org/ 
>>> \">Subversion</a> "
>>> +                   "version " SVN_VERSION "."
>>> +                   "</em>\n</body></html>");
>>> +        }
>>>       else
>>>         ap_fputs(output, bb, "  </index>\n</svn>\n");
>>
>> I do not really understand what that check is doing related to FOO,  
>> but I trust it is right.  That said, the change itself is not  
>> right.  It is suppressing the closing </body> and </html> tags,  
>> which means the page will be invalid HTML.  It should only be  
>> supressing the "Powered by Subversion version X.Y.Z" line.
>
> The "ap_psignature" takes two arguments a "const char *" and the  
> apache
> "request_rec", when this is passed we ll get back a string formatted  
> like the
> apache signature we see in the directory listing if ServerSignature is
> 'enabled', else this function will return a empty string. In the above
> implementation we check the same if we get some string, then we know
> ServerSignature is enabled and we print the "Powered by" thingy,  
> else if we get
> an empty string, we just print nothing.
>
> Relevant part of code from apache-2.2.13 (server/core.c) is here,
>
> <snip>
> AP_DECLARE(const char *) ap_psignature(const char *prefix,  
> request_rec *r)
> {
>     char sport[20];
>     core_dir_config *conf;
>
>     conf = (core_dir_config *)ap_get_module_config(r->per_dir_config,
>                                                    &core_module);
>     if ((conf->server_signature == srv_sig_off)
>             || (conf->server_signature == srv_sig_unset)) {
>         return "";
>     }
>
>     apr_snprintf(sport, sizeof sport, "%u", (unsigned)  
> ap_get_server_port(r));
>
>     if (conf->server_signature == srv_sig_withmail) {
>         return apr_pstrcat(r->pool, prefix, "<address>",
>                            ap_get_server_banner(),
>                            " Server at <a href=\"",
>                            ap_is_url(r->server->server_admin) ? "" :  
> "mailto:",
>                            ap_escape_html(r->pool, r->server- 
> >server_admin),
>                            "\">",
>                            ap_escape_html(r->pool, ap_get_server_name 
> (r)),
>                            "</a> Port ", sport,
>                            "</address>\n", NULL);
>     }
>
>     return apr_pstrcat(r->pool, prefix, "<address>",  
> ap_get_server_banner(),
>                        " Server at ",
>                        ap_escape_html(r->pool, ap_get_server_name(r)),
>                        " Port ", sport,
>                        "</address>\n", NULL);
> }
> </snip>
>
> The closing tag problem is fixed in r40021.

So, should r40021 be nominated along with r40008?

-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2407605

Re: svn commit: r40008 - trunk/subversion/mod_dav_svn

Posted by Senthil Kumaran S <se...@collab.net>.
Mark Phippard wrote:
>> -      if (gen_html)
>> -        ap_fputs(output, bb,
>> -                 " </ul>\n <hr noshade><em>Powered by "
>> -                 "<a href=\"http://subversion.tigris.org/\">Subversion</a> "
>> -                 "version " SVN_VERSION "."
>> -                 "</em>\n</body></html>");
>> +      if (gen_html
>> +          && (strcmp(ap_psignature("FOO", resource->info->r), "") != 0))
>> +        {
>> +          /* Apache's signature generation code didn't eat our prefix.
>> +             ServerSignature must be enabled.  Print our version info.  */
>> +          ap_fputs(output, bb,
>> +                   " </ul>\n <hr noshade><em>Powered by "
>> +                   "<a href=\"http://subversion.tigris.org/\">Subversion</a> "
>> +                   "version " SVN_VERSION "."
>> +                   "</em>\n</body></html>");
>> +        }
>>        else
>>          ap_fputs(output, bb, "  </index>\n</svn>\n");
> 
> I do not really understand what that check is doing related to FOO, but I trust it is right.  That said, the change itself is not right.  It is suppressing the closing </body> and </html> tags, which means the page will be invalid HTML.  It should only be supressing the "Powered by Subversion version X.Y.Z" line.

The "ap_psignature" takes two arguments a "const char *" and the apache 
"request_rec", when this is passed we ll get back a string formatted like the 
apache signature we see in the directory listing if ServerSignature is 
'enabled', else this function will return a empty string. In the above 
implementation we check the same if we get some string, then we know 
ServerSignature is enabled and we print the "Powered by" thingy, else if we get 
an empty string, we just print nothing.

Relevant part of code from apache-2.2.13 (server/core.c) is here,

<snip>
AP_DECLARE(const char *) ap_psignature(const char *prefix, request_rec *r)
{
     char sport[20];
     core_dir_config *conf;

     conf = (core_dir_config *)ap_get_module_config(r->per_dir_config,
                                                    &core_module);
     if ((conf->server_signature == srv_sig_off)
             || (conf->server_signature == srv_sig_unset)) {
         return "";
     }

     apr_snprintf(sport, sizeof sport, "%u", (unsigned) ap_get_server_port(r));

     if (conf->server_signature == srv_sig_withmail) {
         return apr_pstrcat(r->pool, prefix, "<address>",
                            ap_get_server_banner(),
                            " Server at <a href=\"",
                            ap_is_url(r->server->server_admin) ? "" : "mailto:",
                            ap_escape_html(r->pool, r->server->server_admin),
                            "\">",
                            ap_escape_html(r->pool, ap_get_server_name(r)),
                            "</a> Port ", sport,
                            "</address>\n", NULL);
     }

     return apr_pstrcat(r->pool, prefix, "<address>", ap_get_server_banner(),
                        " Server at ",
                        ap_escape_html(r->pool, ap_get_server_name(r)),
                        " Port ", sport,
                        "</address>\n", NULL);
}
</snip>

The closing tag problem is fixed in r40021.

Thank You.
-- 
Senthil Kumaran S
http://www.stylesen.org/

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2407535