Code review comment for lp:~jelmer/bzr/safe-open

Moving this up to the bzrlib level seems reasonable, but it really needs a better name. The problem with 'safe' is you need context to understand what it actually means and the term is too widely used for completely different things.

+"""Safe branch opening."""

Okay, something to do with branches.

+ def check_one_url(self, url):
+ """Check the safety of the source URL.

Something like same domain restrictions?

+class SafeBranchOpener(object):
+ """Safe branch opener.

Hm.

+def safe_open(allowed_scheme, url):
+ """Open the branch at `url`, only accessing URLs on `allowed_scheme`.

Aha! A useful docstring on this function... which actually does something much more specific than the name or module content implies.

So this is really about opening branches with certain URL based checks added. The naming should really reflect that limited scope.

+class BlacklistPolicy(BranchOpenPolicy):
+ """Branch policy that forbids certain URLs."""
+
+ def __init__(self, should_follow_references, unsafe_urls=None):
+ if unsafe_urls is None:
+ unsafe_urls = set()
+ self._unsafe_urls = unsafe_urls

This is unsafe, bzrlib doesn't normalise URLs enough to ensure there's only one canonical form. Also, forgetting a param meaning "no restrictions" isn't a great interface. I'd just add an underscore to the class name for now to discourage use.

There's some more to nitpick over with the code, but it doesn't seem that important.