unknown
5/20/25

[#5051] SVN integration is not robust and has poor error reporting
Summary SVN integration is not robust and has poor error reporting
Queue Horde Framework Packages
Queue Version HEAD
Type Bug
State Resolved
Priority 1. Low
Owners
Requester meyer (at) mesw (dot) de
Created 02/28/2007 (6656 days ago)
Due
Updated 03/29/2007 (6627 days ago)
Assigned 02/28/2007 (6656 days ago)
Resolved 03/29/2007 (6627 days ago)
Milestone
Patch No

History
03/29/2007 05:02:36 PM Jan Schneider Comment #5
State ⇒ Resolved
Reply to this comment
Committed, thanks.
03/01/2007 09:16:03 AM meyer (at) mesw (dot) de Comment #4
New Attachment: chora_new_setting.patch Download
Reply to this comment
...and here's another patch for chora to show the new svn_home setting 
to the user.
03/01/2007 09:15:26 AM meyer (at) mesw (dot) de Comment #3
New Attachment: vc_new_setting.patch Download
Reply to this comment
Okay, I changed the patch as you requested, see attachment.


02/28/2007 11:46:11 PM Jan Schneider Comment #2
State ⇒ Feedback
Reply to this comment
This makes sense and patch looks very clean. The only change I'm 
requesting is, that instead of reusing the cvsps_home path, introduce 
a new configuration variable in Chora, and for backward compatibility 
reasons fall back to Util::getTempDir() if the value is not set.
02/28/2007 11:10:54 PM meyer (at) mesw (dot) de Comment #1
Priority ⇒ 1. Low
Type ⇒ Bug
Summary ⇒ SVN integration is not robust and has poor error reporting
Queue ⇒ Horde Framework Packages
New Attachment: vc.patch Download
State ⇒ Unconfirmed
Reply to this comment
Judging from posts on the mailing lists, it is common that a user 
installs Chora and configures it to use SVN but the content of the 
repositories are not shown, yet there are no error messages and no 
messages in the logs and there does not seem to be an apparent 
misconfiguration.



The attached patch tries to improve this situation by making the 
following changes:



(1)



In VC::getPath(), the path of the SVN (or any other) binary is only 
returned if php determines it is executable. This was introduced 
without explanation in log message or comment in version 1.7:



http://cvs.horde.org/diff.php?r1=1.6&r2=1.7&f=framework%2FVC%2FVC.php



This is wrong for two reasons:



First, when a PHP basedir restriction to the htdocs/ directory is 
active (common on many webservers), PHP's built-in function 
"is_executable" cannot access files in, say, /usr/bin although it can 
execute this file. Infact, the only way to see if the file can be 
executed is to try to execute it. Thus, the use of "is_executable" 
here forces the user to extend the basedir restriction to /usr/bin, 
which is a bad idea from a security point of view.



Second, the error return value "false" is never checked within 
"svn.php" and therefore wrong commands will be executed. E.g. when the 
main module list of the SVN repository is supposed to be shown, Chora 
will build a string of the form "/usr/bin/svn ls /path/to/repository". 
If "/usr/bin/svn" is not found, "ls /path/to/repository" will be 
executed instead. This will succeed (=> no error message), but the 
returned output will not match the expected output (=> modules list 
will be empty). Thus, the user has no chance to find out what is wrong 
than to debug the code.



Therefore the patch restores the code which was present in rev 1.6 of 
VC.php. This will always try to execute the SVN binary. If the SVN 
binary is not found, a sensible error message will be shown.



(2)



A common configuration on web servers (e.g. the default configuration 
on SUSE 9.x) is that the apache process runs as root but the web pages 
are handled by the "wwwroot" user which has low privileges. Since the 
apache process is started as root, its HOME dir will point to "/root". 
In this case, svn, when started by the "wwwroot" user will look for 
the presence of "/root/.svn" and fail because it obviously is denied 
access. This behaviour can be prevented by explicitely setting the 
"--config-dir" directive to a directory the current user has access to 
(say, something like "/tmp"). The attached patch automates this 
behaviour.


Saved Queries