On Mon, Dec 27, 2010 at 1:56 AM, Augie Fackler <[email protected]> wrote: > On Dec 26, 2010, at 12:45 PM, Jelmer Vernooij wrote: >> >> On Mon, Dec 27, 2010 at 12:45:14AM +0800, Tay Ray Chuan wrote: >>> On Mon, Dec 27, 2010 at 12:36 AM, Jelmer Vernooij <[email protected]> wrote: >>>> On Mon, Dec 27, 2010 at 12:15:45AM +0800, Tay Ray Chuan wrote: >>> [snip] >>>>> +def _norm_path(path): >>>>> + ? ?return os.path.normcase(os.path.realpath(path)) >>>> Thanks for the patches. >>>> >>>> I'm not sure this is a useful thing to factor out. >>> >>> It makes things neater. In the next patch (#4), we go through the >>> whole gamut again for the parent directory. >>> >>>> Also, why the os.path.realpath? We're just going to open these files, why >>>> do we care >>>> about their canonical location? >>> >>> A malicious user could ask for an path like >>> >>> /../some/file >>> >>> realpath "escapes" these for us. >> dulwich.rpeo is the wrong place for server-side permission checks IMHO. At >> the >> very least we should raise an exception if the path is outside of the >> repository rather than returning None. >> >> In genera though, I think we should put checks like this into the server >> rather than here. > > +1 on both points.
(Strictly speaking, looking at the http server urls allowed, it's close to impossible to get ".." in paths from the server.) In any case, outside of the web server, do want to allow such "bad" file paths to be accessed? I think not. By having this at the repo-level, we can be sure of "safe" file accesses, regardless of who's requesting the file - the server, or elsewhere. Implementation-wise, like patch #2, putting it on the server-side makes things complicated because such "escaping" shouldn't be done for in-memory repos. But I agree on the "return None" point - the failure is too mild. -- Cheers, Ray Chuan _______________________________________________ Mailing list: https://launchpad.net/~dulwich-users Post to : [email protected] Unsubscribe : https://launchpad.net/~dulwich-users More help : https://help.launchpad.net/ListHelp
