* [PATCH v10 5/6] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-14 22:39 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, peff, sbeller, bburky, jrnieder
In-Reply-To: <1481755195-174539-1-git-send-email-bmwill@google.com>
Add a from_user parameter to is_transport_allowed() to allow http to be
able to distinguish between protocol restrictions for redirects versus
initial requests. CURLOPT_REDIR_PROTOCOLS can now be set differently
from CURLOPT_PROTOCOLS to disallow use of protocols with the "user"
policy in redirects.
This change allows callers to query if a transport protocol is allowed,
given that the caller knows that the protocol is coming from the user
(1) or not from the user (0) such as redirects in libcurl. If unknown a
-1 should be provided which falls back to reading
`GIT_PROTOCOL_FROM_USER` to determine if the protocol came from the
user.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
http.c | 14 +++++++-------
t/t5812-proto-disable-http.sh | 7 +++++++
transport.c | 8 +++++---
transport.h | 13 ++++++++++---
4 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/http.c b/http.c
index f7c488a..2208269 100644
--- a/http.c
+++ b/http.c
@@ -489,17 +489,17 @@ static void set_curl_keepalive(CURL *c)
}
#endif
-static long get_curl_allowed_protocols(void)
+static long get_curl_allowed_protocols(int from_user)
{
long allowed_protocols = 0;
- if (is_transport_allowed("http"))
+ if (is_transport_allowed("http", from_user))
allowed_protocols |= CURLPROTO_HTTP;
- if (is_transport_allowed("https"))
+ if (is_transport_allowed("https", from_user))
allowed_protocols |= CURLPROTO_HTTPS;
- if (is_transport_allowed("ftp"))
+ if (is_transport_allowed("ftp", from_user))
allowed_protocols |= CURLPROTO_FTP;
- if (is_transport_allowed("ftps"))
+ if (is_transport_allowed("ftps", from_user))
allowed_protocols |= CURLPROTO_FTPS;
return allowed_protocols;
@@ -588,9 +588,9 @@ static CURL *get_curl_handle(void)
#endif
#if LIBCURL_VERSION_NUM >= 0x071304
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
- get_curl_allowed_protocols());
+ get_curl_allowed_protocols(0));
curl_easy_setopt(result, CURLOPT_PROTOCOLS,
- get_curl_allowed_protocols());
+ get_curl_allowed_protocols(-1));
#else
warning("protocol restrictions not applied to curl redirects because\n"
"your curl version is too old (>= 7.19.4)");
diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 044cc15..d911afd 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -30,5 +30,12 @@ test_expect_success 'curl limits redirects' '
test_must_fail git clone "$HTTPD_URL/loop-redir/smart/repo.git"
'
+test_expect_success 'http can be limited to from-user' '
+ git -c protocol.http.allow=user \
+ clone "$HTTPD_URL/smart/repo.git" plain.git &&
+ test_must_fail git -c protocol.http.allow=user \
+ clone "$HTTPD_URL/smart-redir-perm/repo.git" redir.git
+'
+
stop_httpd
test_done
diff --git a/transport.c b/transport.c
index fbd799d..f50c31a 100644
--- a/transport.c
+++ b/transport.c
@@ -676,7 +676,7 @@ static enum protocol_allow_config get_protocol_config(const char *type)
return PROTOCOL_ALLOW_USER_ONLY;
}
-int is_transport_allowed(const char *type)
+int is_transport_allowed(const char *type, int from_user)
{
const struct string_list *whitelist = protocol_whitelist();
if (whitelist)
@@ -688,7 +688,9 @@ int is_transport_allowed(const char *type)
case PROTOCOL_ALLOW_NEVER:
return 0;
case PROTOCOL_ALLOW_USER_ONLY:
- return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ if (from_user < 0)
+ from_user = git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ return from_user;
}
die("BUG: invalid protocol_allow_config type");
@@ -696,7 +698,7 @@ int is_transport_allowed(const char *type)
void transport_check_allowed(const char *type)
{
- if (!is_transport_allowed(type))
+ if (!is_transport_allowed(type, -1))
die("transport '%s' not allowed", type);
}
diff --git a/transport.h b/transport.h
index 3396e1d..4f1c801 100644
--- a/transport.h
+++ b/transport.h
@@ -142,10 +142,17 @@ struct transport {
struct transport *transport_get(struct remote *, const char *);
/*
- * Check whether a transport is allowed by the environment. Type should
- * generally be the URL scheme, as described in Documentation/git.txt
+ * Check whether a transport is allowed by the environment.
+ *
+ * Type should generally be the URL scheme, as described in
+ * Documentation/git.txt
+ *
+ * from_user specifies if the transport was given by the user. If unknown pass
+ * a -1 to read from the environment to determine if the transport was given by
+ * the user.
+ *
*/
-int is_transport_allowed(const char *type);
+int is_transport_allowed(const char *type, int from_user);
/*
* Check whether a transport is allowed by the environment,
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v10 0/6] transport protocol policy configuration
From: Brandon Williams @ 2016-12-14 22:39 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, peff, sbeller, bburky, jrnieder
In-Reply-To: <1481679637-133137-1-git-send-email-bmwill@google.com>
v10 of this series fixes the following:
* A few updates to the commit messages in order to better convey the reasoning
behind the a few of the patches.
* Additional test to verify that curl redirects respect configured protocol
policies.
* Patch added by Jeff King to make http alternates respect configured
protocol policies.
Brandon Williams (5):
lib-proto-disable: variable name fix
http: always warn if libcurl version is too old
transport: add protocol policy config option
http: create function to get curl allowed protocols
transport: add from_user parameter to is_transport_allowed
Jeff King (1):
http: respect protocol.*.allow=user for http-alternates
Documentation/config.txt | 46 +++++++++++++
Documentation/git.txt | 38 ++++-------
git-submodule.sh | 12 ++--
http-walker.c | 52 +++++++++++---
http.c | 36 ++++++----
t/lib-proto-disable.sh | 142 ++++++++++++++++++++++++++++++++++++---
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5550-http-fetch-dumb.sh | 10 +++
t/t5802-connect-helper.sh | 1 +
t/t5812-proto-disable-http.sh | 7 ++
transport.c | 84 ++++++++++++++++++++---
transport.h | 19 +++---
12 files changed, 363 insertions(+), 85 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [PATCH v10 3/6] transport: add protocol policy config option
From: Brandon Williams @ 2016-12-14 22:39 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, peff, sbeller, bburky, jrnieder
In-Reply-To: <1481755195-174539-1-git-send-email-bmwill@google.com>
Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands. This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols. This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.
Now users can specify a policy to be used for each type of protocol via
the 'protocol.<name>.allow' config option. A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option. If no user configured default is made git will allow known-safe
protocols (http, https, git, ssh, file), disallow known-dangerous
protocols (ext), and have a default policy of `user` for all other
protocols.
The supported policies are `always`, `never`, and `user`. The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules). Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.
Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.
Based on a patch by Jeff King <peff@peff.net>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/config.txt | 46 ++++++++++++++
Documentation/git.txt | 38 +++++-------
git-submodule.sh | 12 ++--
t/lib-proto-disable.sh | 130 +++++++++++++++++++++++++++++++++++++--
t/t5509-fetch-push-namespaces.sh | 1 +
t/t5802-connect-helper.sh | 1 +
transport.c | 75 +++++++++++++++++++++-
7 files changed, 264 insertions(+), 39 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8153336..50d3d06 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2260,6 +2260,52 @@ pretty.<name>::
Note that an alias with the same name as a built-in format
will be silently ignored.
+protocol.allow::
+ If set, provide a user defined default policy for all protocols which
+ don't explicitly have a policy (`protocol.<name>.allow`). By default,
+ if unset, known-safe protocols (http, https, git, ssh, file) have a
+ default policy of `always`, known-dangerous protocols (ext) have a
+ default policy of `never`, and all other protocols have a default
+ policy of `user`. Supported policies:
++
+--
+
+* `always` - protocol is always able to be used.
+
+* `never` - protocol is never able to be used.
+
+* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
+ either unset or has a value of 1. This policy should be used when you want a
+ protocol to be directly usable by the user but don't want it used by commands which
+ execute clone/fetch/push commands without user input, e.g. recursive
+ submodule initialization.
+
+--
+
+protocol.<name>.allow::
+ Set a policy to be used by protocol `<name>` with clone/fetch/push
+ commands. See `protocol.allow` above for the available policies.
++
+The protocol names currently used by git are:
++
+--
+ - `file`: any local file-based path (including `file://` URLs,
+ or local paths)
+
+ - `git`: the anonymous git protocol over a direct TCP
+ connection (or proxy, if configured)
+
+ - `ssh`: git over ssh (including `host:path` syntax,
+ `ssh://`, etc).
+
+ - `http`: git over http, both "smart http" and "dumb http".
+ Note that this does _not_ include `https`; if you want to configure
+ both, you must do so individually.
+
+ - any external helpers are named by their protocol (e.g., use
+ `hg` to allow the `git-remote-hg` helper)
+--
+
pull.ff::
By default, Git does not create an extra merge commit when merging
a commit that is a descendant of the current commit. Instead, the
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 923aa49..d9fb937 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1129,30 +1129,20 @@ of clones and fetches.
cloning a repository to make a backup).
`GIT_ALLOW_PROTOCOL`::
- If set, provide a colon-separated list of protocols which are
- allowed to be used with fetch/push/clone. This is useful to
- restrict recursive submodule initialization from an untrusted
- repository. Any protocol not mentioned will be disallowed (i.e.,
- this is a whitelist, not a blacklist). If the variable is not
- set at all, all protocols are enabled. The protocol names
- currently used by git are:
-
- - `file`: any local file-based path (including `file://` URLs,
- or local paths)
-
- - `git`: the anonymous git protocol over a direct TCP
- connection (or proxy, if configured)
-
- - `ssh`: git over ssh (including `host:path` syntax,
- `ssh://`, etc).
-
- - `http`: git over http, both "smart http" and "dumb http".
- Note that this does _not_ include `https`; if you want both,
- you should specify both as `http:https`.
-
- - any external helpers are named by their protocol (e.g., use
- `hg` to allow the `git-remote-hg` helper)
-
+ If set to a colon-separated list of protocols, behave as if
+ `protocol.allow` is set to `never`, and each of the listed
+ protocols has `protocol.<name>.allow` set to `always`
+ (overriding any existing configuration). In other words, any
+ protocol not mentioned will be disallowed (i.e., this is a
+ whitelist, not a blacklist). See the description of
+ `protocol.allow` in linkgit:git-config[1] for more details.
+
+`GIT_PROTOCOL_FROM_USER`::
+ Set to 0 to prevent protocols used by fetch/push/clone which are
+ configured to the `user` state. This is useful to restrict recursive
+ submodule initialization from an untrusted repository or for programs
+ which feed potentially-untrusted URLS to git commands. See
+ linkgit:git-config[1] for more details.
Discussion[[Discussion]]
------------------------
diff --git a/git-submodule.sh b/git-submodule.sh
index 78fdac9..fc44076 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -22,14 +22,10 @@ require_work_tree
wt_prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
-# Restrict ourselves to a vanilla subset of protocols; the URLs
-# we get are under control of a remote repository, and we do not
-# want them kicking off arbitrary git-remote-* programs.
-#
-# If the user has already specified a set of allowed protocols,
-# we assume they know what they're doing and use that instead.
-: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
-export GIT_ALLOW_PROTOCOL
+# Tell the rest of git that any URLs we get don't come
+# directly from the user, so it can apply policy as appropriate.
+GIT_PROTOCOL_FROM_USER=0
+export GIT_PROTOCOL_FROM_USER
command=
branch=
diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index be88e9a..02f49cb 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -1,10 +1,7 @@
# Test routines for checking protocol disabling.
-# test cloning a particular protocol
-# $1 - description of the protocol
-# $2 - machine-readable name of the protocol
-# $3 - the URL to try cloning
-test_proto () {
+# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist
+test_whitelist () {
desc=$1
proto=$2
url=$3
@@ -62,6 +59,129 @@ test_proto () {
test_must_fail git clone --bare "$url" tmp.git
)
'
+
+ test_expect_success "clone $desc (env var has precedence)" '
+ rm -rf tmp.git &&
+ (
+ GIT_ALLOW_PROTOCOL=none &&
+ export GIT_ALLOW_PROTOCOL &&
+ test_must_fail git -c protocol.allow=always clone --bare "$url" tmp.git &&
+ test_must_fail git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
+ )
+ '
+}
+
+test_config () {
+ desc=$1
+ proto=$2
+ url=$3
+
+ # Test clone/fetch/push with protocol.<type>.allow config
+ test_expect_success "clone $desc (enabled with config)" '
+ rm -rf tmp.git &&
+ git -c protocol.$proto.allow=always clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=always fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=always push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ test_must_fail git -C tmp.git -c protocol.$proto.allow=never push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ test_must_fail git -C tmp.git -c protocol.$proto.allow=never fetch
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git -c protocol.$proto.allow=never clone --bare "$url" tmp.git
+ '
+
+ # Test clone/fetch/push with protocol.user.allow and its env var
+ test_expect_success "clone $desc (enabled)" '
+ rm -rf tmp.git &&
+ git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=user fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git -c protocol.$proto.allow=user push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ (
+ cd tmp.git &&
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user push origin HEAD:pushed
+ )
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ (
+ cd tmp.git &&
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user fetch
+ )
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ (
+ GIT_PROTOCOL_FROM_USER=0 &&
+ export GIT_PROTOCOL_FROM_USER &&
+ test_must_fail git -c protocol.$proto.allow=user clone --bare "$url" tmp.git
+ )
+ '
+
+ # Test clone/fetch/push with protocol.allow user defined default
+ test_expect_success "clone $desc (enabled)" '
+ rm -rf tmp.git &&
+ git config --global protocol.allow always &&
+ git clone --bare "$url" tmp.git
+ '
+
+ test_expect_success "fetch $desc (enabled)" '
+ git -C tmp.git fetch
+ '
+
+ test_expect_success "push $desc (enabled)" '
+ git -C tmp.git push origin HEAD:pushed
+ '
+
+ test_expect_success "push $desc (disabled)" '
+ git config --global protocol.allow never &&
+ test_must_fail git -C tmp.git push origin HEAD:pushed
+ '
+
+ test_expect_success "fetch $desc (disabled)" '
+ test_must_fail git -C tmp.git fetch
+ '
+
+ test_expect_success "clone $desc (disabled)" '
+ rm -rf tmp.git &&
+ test_must_fail git clone --bare "$url" tmp.git
+ '
+}
+
+# test cloning a particular protocol
+# $1 - description of the protocol
+# $2 - machine-readable name of the protocol
+# $3 - the URL to try cloning
+test_proto () {
+ test_whitelist "$@"
+
+ test_config "$@"
}
# set up an ssh wrapper that will access $host/$repo in the
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index bc44ac3..75c570a 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git init original &&
(
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index b7a7f9d..c6c2661 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -4,6 +4,7 @@ test_description='ext::cmd remote "connect" helper'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git commit --allow-empty -m initial &&
test_tick &&
diff --git a/transport.c b/transport.c
index dff929e..fbd799d 100644
--- a/transport.c
+++ b/transport.c
@@ -617,10 +617,81 @@ static const struct string_list *protocol_whitelist(void)
return enabled ? &allowed : NULL;
}
+enum protocol_allow_config {
+ PROTOCOL_ALLOW_NEVER = 0,
+ PROTOCOL_ALLOW_USER_ONLY,
+ PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+ const char *value)
+{
+ if (!strcasecmp(value, "always"))
+ return PROTOCOL_ALLOW_ALWAYS;
+ else if (!strcasecmp(value, "never"))
+ return PROTOCOL_ALLOW_NEVER;
+ else if (!strcasecmp(value, "user"))
+ return PROTOCOL_ALLOW_USER_ONLY;
+
+ die("unknown value for config '%s': %s", key, value);
+}
+
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+ char *key = xstrfmt("protocol.%s.allow", type);
+ char *value;
+
+ /* first check the per-protocol config */
+ if (!git_config_get_string(key, &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config(key, value);
+ free(key);
+ free(value);
+ return ret;
+ }
+ free(key);
+
+ /* if defined, fallback to user-defined default for unknown protocols */
+ if (!git_config_get_string("protocol.allow", &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config("protocol.allow", value);
+ free(value);
+ return ret;
+ }
+
+ /* fallback to built-in defaults */
+ /* known safe */
+ if (!strcmp(type, "http") ||
+ !strcmp(type, "https") ||
+ !strcmp(type, "git") ||
+ !strcmp(type, "ssh") ||
+ !strcmp(type, "file"))
+ return PROTOCOL_ALLOW_ALWAYS;
+
+ /* known scary; err on the side of caution */
+ if (!strcmp(type, "ext"))
+ return PROTOCOL_ALLOW_NEVER;
+
+ /* unknown; by default let them be used only directly by the user */
+ return PROTOCOL_ALLOW_USER_ONLY;
+}
+
int is_transport_allowed(const char *type)
{
- const struct string_list *allowed = protocol_whitelist();
- return !allowed || string_list_has_string(allowed, type);
+ const struct string_list *whitelist = protocol_whitelist();
+ if (whitelist)
+ return string_list_has_string(whitelist, type);
+
+ switch (get_protocol_config(type)) {
+ case PROTOCOL_ALLOW_ALWAYS:
+ return 1;
+ case PROTOCOL_ALLOW_NEVER:
+ return 0;
+ case PROTOCOL_ALLOW_USER_ONLY:
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ }
+
+ die("BUG: invalid protocol_allow_config type");
}
void transport_check_allowed(const char *type)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [PATCH v10 2/6] http: always warn if libcurl version is too old
From: Brandon Williams @ 2016-12-14 22:39 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, gitster, peff, sbeller, bburky, jrnieder
In-Reply-To: <1481755195-174539-1-git-send-email-bmwill@google.com>
Always warn if libcurl version is too old because:
1. Even without a protocol whitelist, newer versions of curl have all
non-http protocols disabled by default.
2. A future patch will introduce default "known-good" and "known-bad"
protocols which are allowed/disallowed by 'is_transport_allowed'
which older version of libcurl can't respect.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
http.c | 5 ++---
transport.c | 5 -----
transport.h | 6 ------
3 files changed, 2 insertions(+), 14 deletions(-)
diff --git a/http.c b/http.c
index 5cd3ffd..034426e 100644
--- a/http.c
+++ b/http.c
@@ -583,9 +583,8 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
#else
- if (transport_restrict_protocols())
- warning("protocol restrictions not applied to curl redirects because\n"
- "your curl version is too old (>= 7.19.4)");
+ warning("protocol restrictions not applied to curl redirects because\n"
+ "your curl version is too old (>= 7.19.4)");
#endif
if (getenv("GIT_CURL_VERBOSE"))
diff --git a/transport.c b/transport.c
index 41eb82c..dff929e 100644
--- a/transport.c
+++ b/transport.c
@@ -629,11 +629,6 @@ void transport_check_allowed(const char *type)
die("transport '%s' not allowed", type);
}
-int transport_restrict_protocols(void)
-{
- return !!protocol_whitelist();
-}
-
struct transport *transport_get(struct remote *remote, const char *url)
{
const char *helper;
diff --git a/transport.h b/transport.h
index c681408..3396e1d 100644
--- a/transport.h
+++ b/transport.h
@@ -153,12 +153,6 @@ int is_transport_allowed(const char *type);
*/
void transport_check_allowed(const char *type);
-/*
- * Returns true if the user has attempted to turn on protocol
- * restrictions at all.
- */
-int transport_restrict_protocols(void);
-
/* Transport options which apply to git:// and scp-style URLs */
/* The program to use on the remote side to send a pack */
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-14 21:58 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214211229.mpgu3hahlfbdxys6@sigill.intra.peff.net>
On 12/14, Jeff King wrote:
> On Wed, Dec 14, 2016 at 03:33:49PM -0500, Jeff King wrote:
>
> > One other "simple" fix is at the moment we parse http-alternates, to
> > parse the URL ourself and check the policy. Like:
> > [...]
> > I may have convinced myself this is a reasonable approach.
>
> So here it is in patch form, with a test.
>
> I also took a look at how bad it would be to plumb through the "this is
> an alternate" flag. And it's actually a little nasty, because the
> http-walker isn't calling get_active_slot() itself. It's relying on
> http_get_file() and other wrappers. The recent http_get_options makes
> this slightly less terrible, but I'd still rather avoid infecting the
> general http code that is used for the smart-http code paths.
>
> So I think this patch on top of your series, plus the other minor fixes
> we've discussed, the topic should be ready for 'next'.
Sounds good, I'll make those few changes, place this patch ontop of
the series and send out v10 of the series in just a bit.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-14 21:12 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214203349.6ym3v2ny2gonovx2@sigill.intra.peff.net>
On Wed, Dec 14, 2016 at 03:33:49PM -0500, Jeff King wrote:
> One other "simple" fix is at the moment we parse http-alternates, to
> parse the URL ourself and check the policy. Like:
> [...]
> I may have convinced myself this is a reasonable approach.
So here it is in patch form, with a test.
I also took a look at how bad it would be to plumb through the "this is
an alternate" flag. And it's actually a little nasty, because the
http-walker isn't calling get_active_slot() itself. It's relying on
http_get_file() and other wrappers. The recent http_get_options makes
this slightly less terrible, but I'd still rather avoid infecting the
general http code that is used for the smart-http code paths.
So I think this patch on top of your series, plus the other minor fixes
we've discussed, the topic should be ready for 'next'.
-- >8 --
Subject: http: respect protocol.*.allow=user for http-alternates
The http-walker may fetch the http-alternates (or
alternates) file from a remote in order to find more
objects. This should count as a "not from the user" use of
the protocol. But because we implement the redirection
ourselves and feed the new URL to curl, it will use the
CURLOPT_PROTOCOLS rules, not the more restrictive
CURLOPT_REDIR_PROTOCOLS.
The ideal solution would be for each curl request we make to
know whether or not is directly from the user or part of an
alternates redirect, and then set CURLOPT_PROTOCOLS as
appropriate. However, that would require plumbing that
information through all of the various layers of the http
code.
Instead, let's check the protocol at the source: when we are
parsing the remote http-alternates file. The only downside
is that if there's any mismatch between what protocol we
think it is versus what curl thinks it is, it could violate
the policy.
To address this, we'll make the parsing err on the picky
side, and only allow protocols that it can parse
definitively. So for example, you can't elude the "http"
policy by asking for "HTTP://", even though curl might
handle it; we would reject it as unknown. The only unsafe
case would be if you have a URL that starts with "http://"
but curl interprets as another protocol. That seems like an
unlikely failure mode (and we are still protected by our
base CURLOPT_PROTOCOL setting, so the worst you could do is
trigger one of https, ftp, or ftps).
Signed-off-by: Jeff King <peff@peff.net>
---
I actually do reject "HTTP://" here, though I suspect curl does take it.
I notice that you cannot ask to clone "HTTP://..." in the first place,
as our remote-helper interface is case sensitive (or maybe it would even
respect "HTTP://" on a case-insensitive file system!).
Possibly we should be more consistent about down-casing protocols when
comparing them, but I'm not sure if anybody actually cares in practice.
http-walker.c | 52 ++++++++++++++++++++++++++++++++++++----------
t/t5550-http-fetch-dumb.sh | 10 +++++++++
2 files changed, 51 insertions(+), 11 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index c2f81cd6a..b34b6ace7 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -3,6 +3,7 @@
#include "walker.h"
#include "http.h"
#include "list.h"
+#include "transport.h"
struct alt_base {
char *base;
@@ -160,6 +161,32 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
#endif
}
+static int is_alternate_allowed(const char *url)
+{
+ const char *protocols[] = {
+ "http", "https", "ftp", "ftps"
+ };
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(protocols); i++) {
+ const char *end;
+ if (skip_prefix(url, protocols[i], &end) &&
+ starts_with(end, "://"))
+ break;
+ }
+
+ if (i >= ARRAY_SIZE(protocols)) {
+ warning("ignoring alternate with unknown protocol: %s", url);
+ return 0;
+ }
+ if (!is_transport_allowed(protocols[i], 0)) {
+ warning("ignoring alternate with restricted protocol: %s", url);
+ return 0;
+ }
+
+ return 1;
+}
+
static void process_alternates_response(void *callback_data)
{
struct alternates_request *alt_req =
@@ -274,17 +301,20 @@ static void process_alternates_response(void *callback_data)
struct strbuf target = STRBUF_INIT;
strbuf_add(&target, base, serverlen);
strbuf_add(&target, data + i, posn - i - 7);
- warning("adding alternate object store: %s",
- target.buf);
- newalt = xmalloc(sizeof(*newalt));
- newalt->next = NULL;
- newalt->base = strbuf_detach(&target, NULL);
- newalt->got_indices = 0;
- newalt->packs = NULL;
-
- while (tail->next != NULL)
- tail = tail->next;
- tail->next = newalt;
+
+ if (is_alternate_allowed(target.buf)) {
+ warning("adding alternate object store: %s",
+ target.buf);
+ newalt = xmalloc(sizeof(*newalt));
+ newalt->next = NULL;
+ newalt->base = strbuf_detach(&target, NULL);
+ newalt->got_indices = 0;
+ newalt->packs = NULL;
+
+ while (tail->next != NULL)
+ tail = tail->next;
+ tail->next = newalt;
+ }
}
}
i = posn + 1;
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 22011f0b6..c0ee29c65 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -360,5 +360,15 @@ test_expect_success 'http-alternates cannot point at funny protocols' '
clone "$HTTPD_URL/dumb/evil.git" evil-file
'
+test_expect_success 'http-alternates triggers not-from-user protocol check' '
+ echo "$HTTPD_URL/dumb/victim.git/objects" \
+ >"$evil/objects/info/http-alternates" &&
+ test_config_global http.followRedirects true &&
+ test_must_fail git -c protocol.http.allow=user \
+ clone $HTTPD_URL/dumb/evil.git evil-user &&
+ git -c protocol.http.allow=always \
+ clone $HTTPD_URL/dumb/evil.git evil-user
+'
+
stop_httpd
test_done
--
2.11.0.341.g202cd3142
^ permalink raw reply related
* Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-14 20:50 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214204102.hwjb3i4aaxf3oigq@sigill.intra.peff.net>
On 12/14, Jeff King wrote:
> On Wed, Dec 14, 2016 at 12:37:52PM -0800, Brandon Williams wrote:
>
> > Naively looking at the code (and your longer suggestion), is there a
> > reason why we couldn't simply have http-walker set CURLOPT_PROTOCOLS
> > with get_curl_allowed_protocols(0) in the fetch_alternates() function?
> > That way we just override the CURLOPT_PROTOCOLS value when alternates
> > are involved.
>
> No, because we may have many curl handles (especially for the
> http-walker, which wants to fetch several objects simultaneously), and
> they get recycled as needed for many requests.
>
> So setting a restriction there on slot->curl will only cover the one
> handle, and miss other ones which may be used later (and likewise, that
> one handle with the restriction may get recycled and used for a
> non-alternate fetch, and would be unnecessarily restrictive).
>
> That's why any curl-level settings have to happen when we call
> get_active_slot(), since that's when we know what we're actually using
> the handle for.
Fair enough, I figured there may be some reuse happening with the curl
handles but didn't do enough digging to discover that myself. Thanks :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-14 20:41 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214203752.GD20063@google.com>
On Wed, Dec 14, 2016 at 12:37:52PM -0800, Brandon Williams wrote:
> Naively looking at the code (and your longer suggestion), is there a
> reason why we couldn't simply have http-walker set CURLOPT_PROTOCOLS
> with get_curl_allowed_protocols(0) in the fetch_alternates() function?
> That way we just override the CURLOPT_PROTOCOLS value when alternates
> are involved.
No, because we may have many curl handles (especially for the
http-walker, which wants to fetch several objects simultaneously), and
they get recycled as needed for many requests.
So setting a restriction there on slot->curl will only cover the one
handle, and miss other ones which may be used later (and likewise, that
one handle with the restriction may get recycled and used for a
non-alternate fetch, and would be unnecessarily restrictive).
That's why any curl-level settings have to happen when we call
get_active_slot(), since that's when we know what we're actually using
the handle for.
-Peff
^ permalink raw reply
* Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-14 20:37 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214201323.GC20063@google.com>
On 12/14, Brandon Williams wrote:
> On 12/14, Jeff King wrote:
> > On Tue, Dec 13, 2016 at 05:40:37PM -0800, Brandon Williams wrote:
> >
> > > Add the from_user parameter to the 'is_transport_allowed' function.
> > > This allows callers to query if a transport protocol is allowed, given
> > > that the caller knows that the protocol is coming from the user (1) or
> > > not from the user (0) such as redirects in libcurl. If unknown a -1
> > > should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
> > > to determine if the protocol came from the user.
> >
> > I think your commit message is upside-down with respect to the purpose
> > of the patch. The end goal we want is for http to distinguish between
> > protocol restrictions for redirects versus initial requests. The rest is
> > an implementation detail. It's definitely still worth discussing that
> > implementation detail (though I think your in-code comments may be
> > sufficient), but I don't see the rationale discussed here at all.
>
> I'll fix the commit message to better discuss the reasoning behind the
> change.
>
> > > Signed-off-by: Brandon Williams <bmwill@google.com>
> > > ---
> > > http.c | 14 +++++++-------
> > > transport.c | 8 +++++---
> > > transport.h | 13 ++++++++++---
> > > 3 files changed, 22 insertions(+), 13 deletions(-)
> >
> > I'm trying to think of a way to test this. I guess the case we are
> > covering here is when a server redirects, but the protocol is only
> > allowed from the user. So:
> >
> > diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
> > index 044cc152f..d911afd24 100755
> > --- a/t/t5812-proto-disable-http.sh
> > +++ b/t/t5812-proto-disable-http.sh
> > @@ -30,5 +30,12 @@ test_expect_success 'curl limits redirects' '
> > test_must_fail git clone "$HTTPD_URL/loop-redir/smart/repo.git"
> > '
> >
> > +test_expect_success 'http can be limited to from-user' '
> > + git -c protocol.http.allow=user \
> > + clone "$HTTPD_URL/smart/repo.git" plain.git &&
> > + test_must_fail git -c protocol.http.allow=user \
> > + clone "$HTTPD_URL/smart-redir-perm/repo.git" redir.git
> > +'
> > +
> > stop_httpd
> > test_done
> >
> > It's an oddball configuration, and you'd probably just set
> > http.followRedirects=false in practice, but it does correctly check this
> > case.
>
> K I'll add this in as a test.
>
> > > @@ -588,9 +588,9 @@ static CURL *get_curl_handle(void)
> > > #endif
> > > #if LIBCURL_VERSION_NUM >= 0x071304
> > > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> > > - get_curl_allowed_protocols());
> > > + get_curl_allowed_protocols(0));
> > > curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> > > - get_curl_allowed_protocols());
> > > + get_curl_allowed_protocols(-1));
> >
> > This covers internal redirects done by libcurl, but not the dumb-walker
> > http-alternates nonsense. We have to feed the URL from http-alternates
> > back to curl ourselves, so it uses CURLOPT_PROTOCOLS even though it
> > should count as "not from the user".
> >
> > To fix that, I think we'd need something like:
> >
> > - get_curl_handle() stops setting these options, as it is done only
> > once when the curl handle is initialized. Instead, the protocol
> > restrictions should go into get_active_slot(), which is called for
> > each request. The values set would remain the same, and be the
> > baseline.
> >
> > - the http-walker.c code would need to know when it's requesting from
> > the base URL, and when it's an alternate. I think this would depend
> > on the position of the "alt" in in the linked list it keeps.
> >
> > - when requesting from an alternate, http-walker would set
> > CURLOPT_PROTOCOLS with get_curl_allowed_protocols(0)
> >
> > I have to admit that it sounds like a fair bit of work for a pretty
> > obscure case. You'd have to:
> >
> > 1. Turn http.allowRedirects to "true", to allow redirects even for
> > non-initial contact.
> >
> > 2. Turn one of protocol.{http,https,ftp,ftps}.allow to "user" to
> > restrict it from being used in a redirect.
> >
> > I'm tempted to punt on it and just do:
> >
> > if (http_follow_config == HTTP_FOLLOW_ALWAYS &&
> > get_curl_allowed_protocols(0) != get_curl_allowed_protocols(-1))
> > die("user-only protocol restrictions not implemented for http-alternates");
> >
> > which errs on the safe side. We could even shove that down into the case
> > where we actually see some alternates, like:
> >
> > diff --git a/http-walker.c b/http-walker.c
> > index c2f81cd6a..5bcc850b1 100644
> > --- a/http-walker.c
> > +++ b/http-walker.c
> > @@ -160,6 +160,12 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
> > #endif
> > }
> >
> > +static void check_alternates_protocol_restrictions(void)
> > +{
> > + if (get_curl_allowed_protocols(0) != get_curl_allowed_protocol(-1))
> > + die("user-only protocol restrictions not implemented for http alternates");
> > +}
> > +
> > static void process_alternates_response(void *callback_data)
> > {
> > struct alternates_request *alt_req =
> > @@ -272,6 +278,7 @@ static void process_alternates_response(void *callback_data)
> > /* skip "objects\n" at end */
> > if (okay) {
> > struct strbuf target = STRBUF_INIT;
> > + check_alternates_protocol_restrictions();
> > strbuf_add(&target, base, serverlen);
> > strbuf_add(&target, data + i, posn - i - 7);
> > warning("adding alternate object store: %s",
> >
> > I find it unlikely that anybody would ever care, but at least we'd do
> > the safe thing. I dunno. Maybe I am just being lazy.
>
> Well, that's unfortunate! It does sound like a more full-proof solution
> to these dumb http alternates could be involved. I don't think your
> simple "lazy" solution may be enough to not just die by default.
>
> By default ftp/ftps will have a policy of "user only" which means they
> will be set by the call to get_curl_allowed_protocol(-1) but not set by
> get_curl_allowed_protocol(0). This would result in the call to
> check_alternates_protocol_restrictions failing all the time unless the
> user explicitly sets ftp/ftps to "always" or "never". If that is the
> desired behavior then your proposed solution would be fine, otherwise we
> may have to do the more involved approach.
Naively looking at the code (and your longer suggestion), is there a
reason why we couldn't simply have http-walker set CURLOPT_PROTOCOLS
with get_curl_allowed_protocols(0) in the fetch_alternates() function?
That way we just override the CURLOPT_PROTOCOLS value when alternates
are involved.
Like so:
diff --git a/http-walker.c b/http-walker.c
index c2f81cd..b284cec 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -339,6 +339,8 @@ static void fetch_alternates(struct walker *walker, const char *base)
curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
curl_easy_setopt(slot->curl, CURLOPT_URL, url.buf);
+ curl_easy_setopt(slot->curl, CURLOPT_PROTOCOLS,
+ get_curl_allowed_protocols(0));
alt_req.base = base;
alt_req.url = &url;
--
Brandon Williams
^ permalink raw reply related
* Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-14 20:33 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214201323.GC20063@google.com>
On Wed, Dec 14, 2016 at 12:13:23PM -0800, Brandon Williams wrote:
> > I find it unlikely that anybody would ever care, but at least we'd do
> > the safe thing. I dunno. Maybe I am just being lazy.
>
> Well, that's unfortunate! It does sound like a more full-proof solution
> to these dumb http alternates could be involved. I don't think your
> simple "lazy" solution may be enough to not just die by default.
>
> By default ftp/ftps will have a policy of "user only" which means they
> will be set by the call to get_curl_allowed_protocol(-1) but not set by
> get_curl_allowed_protocol(0). This would result in the call to
> check_alternates_protocol_restrictions failing all the time unless the
> user explicitly sets ftp/ftps to "always" or "never". If that is the
> desired behavior then your proposed solution would be fine, otherwise we
> may have to do the more involved approach.
Oh, hrm, you're right. I was definitely meaning for this to kick in only
when you had explicitly configured a protocol to "user" (they'd still
need to enable redirects, but that's much more likely).
Arguably ftp should be on the "safe" list, since it's implemented via
the exact same code as https. That list comes originally from 33cfccbbf
(submodule: allow only certain protocols for submodule fetches,
2015-09-16), but that was just based on what I thought was reasonable at
the time.
I find it hard to believe anybody actually uses git-over-ftp these days,
let alone as a protocol redirect via http-alternates. But AFAIK it does
work.
One other "simple" fix is at the moment we parse http-alternates, to
parse the URL ourself and check the policy. Like:
const char *protocols[] = {
"http", "https", "ftp", "ftps"
};
for (i = 0; i < ARRAY_SIZE(protocols); i++) {
const char *end;
if (skip_prefix(url, protocols[i], end) && starts_with(end, "://"))
break;
}
if (i >= ARRAY_SIZE(protocols))
warning("ignoring alternate with unknown protocol: %s", url);
else if (!is_transport_allowed(protocols[i], 0))
warning("ignoring alternate with restricted protocol: %s", url);
else {
/* actually set up the alt struct */
}
That keeps the logic nicely contained. The downside is that if our
interpretation of the URL is ever different than curl's, it could create
a security problem. The bit above seems fairly foolproof, though,
because it functions as a whitelist. So it doesn't matter if you can
trigger an http request via curl with exotic syntax; it matters whether
curl will trigger anything _besides_ an http syntax if the string starts
with "http://". Which seems unlikely.
I may have convinced myself this is a reasonable approach.
-Peff
^ permalink raw reply
* Re: [PATCH v9 5/5] transport: add from_user parameter to is_transport_allowed
From: Brandon Williams @ 2016-12-14 20:13 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214164050.uxk434kzhw6au4c2@sigill.intra.peff.net>
On 12/14, Jeff King wrote:
> On Tue, Dec 13, 2016 at 05:40:37PM -0800, Brandon Williams wrote:
>
> > Add the from_user parameter to the 'is_transport_allowed' function.
> > This allows callers to query if a transport protocol is allowed, given
> > that the caller knows that the protocol is coming from the user (1) or
> > not from the user (0) such as redirects in libcurl. If unknown a -1
> > should be provided which falls back to reading `GIT_PROTOCOL_FROM_USER`
> > to determine if the protocol came from the user.
>
> I think your commit message is upside-down with respect to the purpose
> of the patch. The end goal we want is for http to distinguish between
> protocol restrictions for redirects versus initial requests. The rest is
> an implementation detail. It's definitely still worth discussing that
> implementation detail (though I think your in-code comments may be
> sufficient), but I don't see the rationale discussed here at all.
I'll fix the commit message to better discuss the reasoning behind the
change.
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > http.c | 14 +++++++-------
> > transport.c | 8 +++++---
> > transport.h | 13 ++++++++++---
> > 3 files changed, 22 insertions(+), 13 deletions(-)
>
> I'm trying to think of a way to test this. I guess the case we are
> covering here is when a server redirects, but the protocol is only
> allowed from the user. So:
>
> diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
> index 044cc152f..d911afd24 100755
> --- a/t/t5812-proto-disable-http.sh
> +++ b/t/t5812-proto-disable-http.sh
> @@ -30,5 +30,12 @@ test_expect_success 'curl limits redirects' '
> test_must_fail git clone "$HTTPD_URL/loop-redir/smart/repo.git"
> '
>
> +test_expect_success 'http can be limited to from-user' '
> + git -c protocol.http.allow=user \
> + clone "$HTTPD_URL/smart/repo.git" plain.git &&
> + test_must_fail git -c protocol.http.allow=user \
> + clone "$HTTPD_URL/smart-redir-perm/repo.git" redir.git
> +'
> +
> stop_httpd
> test_done
>
> It's an oddball configuration, and you'd probably just set
> http.followRedirects=false in practice, but it does correctly check this
> case.
K I'll add this in as a test.
> > @@ -588,9 +588,9 @@ static CURL *get_curl_handle(void)
> > #endif
> > #if LIBCURL_VERSION_NUM >= 0x071304
> > curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS,
> > - get_curl_allowed_protocols());
> > + get_curl_allowed_protocols(0));
> > curl_easy_setopt(result, CURLOPT_PROTOCOLS,
> > - get_curl_allowed_protocols());
> > + get_curl_allowed_protocols(-1));
>
> This covers internal redirects done by libcurl, but not the dumb-walker
> http-alternates nonsense. We have to feed the URL from http-alternates
> back to curl ourselves, so it uses CURLOPT_PROTOCOLS even though it
> should count as "not from the user".
>
> To fix that, I think we'd need something like:
>
> - get_curl_handle() stops setting these options, as it is done only
> once when the curl handle is initialized. Instead, the protocol
> restrictions should go into get_active_slot(), which is called for
> each request. The values set would remain the same, and be the
> baseline.
>
> - the http-walker.c code would need to know when it's requesting from
> the base URL, and when it's an alternate. I think this would depend
> on the position of the "alt" in in the linked list it keeps.
>
> - when requesting from an alternate, http-walker would set
> CURLOPT_PROTOCOLS with get_curl_allowed_protocols(0)
>
> I have to admit that it sounds like a fair bit of work for a pretty
> obscure case. You'd have to:
>
> 1. Turn http.allowRedirects to "true", to allow redirects even for
> non-initial contact.
>
> 2. Turn one of protocol.{http,https,ftp,ftps}.allow to "user" to
> restrict it from being used in a redirect.
>
> I'm tempted to punt on it and just do:
>
> if (http_follow_config == HTTP_FOLLOW_ALWAYS &&
> get_curl_allowed_protocols(0) != get_curl_allowed_protocols(-1))
> die("user-only protocol restrictions not implemented for http-alternates");
>
> which errs on the safe side. We could even shove that down into the case
> where we actually see some alternates, like:
>
> diff --git a/http-walker.c b/http-walker.c
> index c2f81cd6a..5bcc850b1 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -160,6 +160,12 @@ static void prefetch(struct walker *walker, unsigned char *sha1)
> #endif
> }
>
> +static void check_alternates_protocol_restrictions(void)
> +{
> + if (get_curl_allowed_protocols(0) != get_curl_allowed_protocol(-1))
> + die("user-only protocol restrictions not implemented for http alternates");
> +}
> +
> static void process_alternates_response(void *callback_data)
> {
> struct alternates_request *alt_req =
> @@ -272,6 +278,7 @@ static void process_alternates_response(void *callback_data)
> /* skip "objects\n" at end */
> if (okay) {
> struct strbuf target = STRBUF_INIT;
> + check_alternates_protocol_restrictions();
> strbuf_add(&target, base, serverlen);
> strbuf_add(&target, data + i, posn - i - 7);
> warning("adding alternate object store: %s",
>
> I find it unlikely that anybody would ever care, but at least we'd do
> the safe thing. I dunno. Maybe I am just being lazy.
Well, that's unfortunate! It does sound like a more full-proof solution
to these dumb http alternates could be involved. I don't think your
simple "lazy" solution may be enough to not just die by default.
By default ftp/ftps will have a policy of "user only" which means they
will be set by the call to get_curl_allowed_protocol(-1) but not set by
get_curl_allowed_protocol(0). This would result in the call to
check_alternates_protocol_restrictions failing all the time unless the
user explicitly sets ftp/ftps to "always" or "never". If that is the
desired behavior then your proposed solution would be fine, otherwise we
may have to do the more involved approach.
--
Brandon Williams
^ permalink raw reply
* [PATCH v2] fix pushing to //server/share/dir on Windows
From: Johannes Sixt @ 2016-12-14 19:37 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Git Mailing List
In-Reply-To: <20161214173034.inbyakdykjv5j7ua@sigill.intra.peff.net>
normalize_path_copy() is not prepared to keep the double-slash of a
//server/share/dir kind of path, but treats it like a regular POSIX
style path and transforms it to /server/share/dir.
The bug manifests when 'git push //server/share/dir master' is run,
because tmp_objdir_add_as_alternate() uses the path in normalized
form when it registers the quarantine object database via
link_alt_odb_entries(). Needless to say that the directory cannot be
accessed using the wrongly normalized path.
Fix it by skipping all of the root part, not just a potential drive
prefix. offset_1st_component takes care of this, see the
implementation in compat/mingw.c::mingw_offset_1st_component().
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 14.12.2016 um 18:30 schrieb Jeff King:
> Would it be reasonable to
> write:
>
> /* Copy initial part of absolute path, converting separators on Windows */
> const char *end = src + offset_1st_component(src);
> while (src < end) {
> char c = *src++;
> if (c == '\\')
> c = '/';
> *dst++ = c;
> }
Makes a lot of sense! I haven't had an opportunity, though, to test
on Windows.
> ? I'm not sure if it's wrong to convert backslashes in that first
> component or not (but certainly we were before). I don't think we'd need
> is_dir_sep() in that "if()", because we can leave slashes as-is. But
> maybe it would make the code easier to read.
is_dir_sep() is preferable, IMO.
I also changed the commit message and subject line slightly.
path.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/path.c b/path.c
index 52d889c88e..efcedafba6 100644
--- a/path.c
+++ b/path.c
@@ -991,7 +991,7 @@ const char *remove_leading_path(const char *in, const char *prefix)
*
* Performs the following normalizations on src, storing the result in dst:
* - Ensures that components are separated by '/' (Windows only)
- * - Squashes sequences of '/'.
+ * - Squashes sequences of '/' except "//server/share" on Windows
* - Removes "." components.
* - Removes ".." components, and the components the precede them.
* Returns failure (non-zero) if a ".." component appears as first path
@@ -1014,17 +1014,22 @@ const char *remove_leading_path(const char *in, const char *prefix)
int normalize_path_copy_len(char *dst, const char *src, int *prefix_len)
{
char *dst0;
- int i;
+ const char *end;
- for (i = has_dos_drive_prefix(src); i > 0; i--)
- *dst++ = *src++;
+ /*
+ * Copy initial part of absolute path: "/", "C:/", "//server/share/".
+ */
+ end = src + offset_1st_component(src);
+ while (src < end) {
+ char c = *src++;
+ if (is_dir_sep(c))
+ c = '/';
+ *dst++ = c;
+ }
dst0 = dst;
- if (is_dir_sep(*src)) {
- *dst++ = '/';
- while (is_dir_sep(*src))
- src++;
- }
+ while (is_dir_sep(*src))
+ src++;
for (;;) {
char c = *src;
--
2.11.0.79.g263f27a
^ permalink raw reply related
* Re: [PATCH v2 01/34] sequencer: support a new action: 'interactive rebase'
From: Junio C Hamano @ 2016-12-14 19:29 UTC (permalink / raw)
To: Johannes Schindelin, Stephan Beyer; +Cc: git, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <297140020a7312af03136848dcdd0353ee3abdfe.1481642927.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> -/* We will introduce the 'interactive rebase' mode later */
> static inline int is_rebase_i(const struct replay_opts *opts)
> {
> - return 0;
> + return opts->action == REPLAY_INTERACTIVE_REBASE;
> }
>
> static const char *get_dir(const struct replay_opts *opts)
> {
> + if (is_rebase_i(opts))
> + return rebase_path();
> return git_path_seq_dir();
> }
This obviously makes the assumption made by 39784cd362 ("sequencer:
remove useless get_dir() function", 2016-12-07) invalid, where the
"remove useless" thought that the callers of this function wants a
single answer that does not depend on opts.
I'll revert that commit from the sb/sequencer-abort-safety topic (as
the topic is in 'next' already) to keep this one. Please holler if
that is a mistaken decision.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 28/34] run_command_opt(): optionally hide stderr when the command succeeds
From: Johannes Sixt @ 2016-12-14 19:17 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, git, Junio C Hamano, Kevin Daudt,
Dennis Kaarsemaker
In-Reply-To: <20161214130640.ginadvry7wor3tkc@sigill.intra.peff.net>
Am 14.12.2016 um 14:06 schrieb Jeff King:
> On Wed, Dec 14, 2016 at 07:53:23AM -0500, Jeff King wrote:
>
>> On Wed, Dec 14, 2016 at 09:34:20AM +0100, Johannes Sixt wrote:
>>
>>> I wanted to see what it would look like if we make it the caller's
>>> responsibility to throw away stderr. The patch is below, as fixup
>>> of patch 29/34. The change is gross, but the end result is not that
>>> bad, though not really a delightful read, either, mostly due to the
>>> strange cleanup semantics of the start_command/finish_command combo,
>>> so... I dunno.
The cleanup semantics of start_command and finish_command are not that
strange as I thought first. I just hadn't looked well enough.
>>
>> I don't have a strong opinion on the patches under discussion, but here
>> are a few pointers on the run-command interface:
>> [...]
>
> And here is a patch representing my suggestions, on top of yours. Not
> tested beyond "make test".
Thank you, that looks way better.
If there is agreement that this approach is preferable, I think we can
have patches on top of the series; they would be orthogonal and do not
have to take hostage of it. (And it looks like I won't be able to follow
up until later this week[end].)
-- Hannes
^ permalink raw reply
* Re: [PATCH v7 00/16] Mark strings in Perl scripts for translation
From: Junio C Hamano @ 2016-12-14 19:02 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <20161214125439.8822-1-vascomalmeida@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> Changes is this re-roll v7:
> * Add get_comment_line_char subroutine to perl/Git.pm and use it.
> * get_comment_line_char gets the value of core.commentchar configuration
> variable. It handles the 'auto' value taking '#' in this case as the
> comment line character.
> * When core.commentchar is not set to one single character, take '#' as the
> comment line character. This follows the system behaviour programmed in
> config.c::git_default_core_config.
I gave it a read and I think it is ready to move forward.
Thanks.
^ permalink raw reply
* Re: [PATCHv2 5/5] rm: add absorb a submodules git dir before deletion
From: Junio C Hamano @ 2016-12-14 18:55 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, David.Turner, bmwill, sandals
In-Reply-To: <20161213205622.841-6-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> diff --git a/builtin/rm.c b/builtin/rm.c
> index fdd7183f61..8c9c535a88 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -392,28 +392,14 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
> for (i = 0; i < list.nr; i++) {
> const char *path = list.entry[i].name;
> if (list.entry[i].is_submodule) {
> - if (is_empty_dir(path)) {
> - if (!rmdir(path)) {
> - removed = 1;
> - if (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> - continue;
> - }
> - } else {
> - strbuf_reset(&buf);
> - strbuf_addstr(&buf, path);
> - if (!remove_dir_recursively(&buf, 0)) {
> - removed = 1;
> - if (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> - strbuf_release(&buf);
> - continue;
> - } else if (!file_exists(path))
> - /* Submodule was removed by user */
> - if (!remove_path_from_gitmodules(path))
> - gitmodules_modified = 1;
> - /* Fallthrough and let remove_path() fail. */
> - }
> + if (is_empty_dir(path) && rmdir(path))
> + die_errno("git rm: '%s'", path);
> + if (file_exists(path))
> + depopulate_submodule(path);
> + removed = 1;
> + if (!remove_path_from_gitmodules(path))
> + gitmodules_modified = 1;
> + continue;
The updated code structure is somewhat nicer for understanding the
flow than the original, but it can further be improved.
A step "If empty directory, we try to rmdir and we check its return
status and die ourselves if we couldn't remove it" reads very
sensible, but coming immediately after that, "if it still exists,
call depop()" looks a bit strange. I would have expected a similar
construct, i.e.
if (directory_exists(path) && depop_submodlue(path))
die("git rm: '%s' submodule still populated", path);
there. Also,
if (is_empty_dir(path)) {
if (rmdir(path))
die_errno(...);
} else if (is_nonempty_dir(path)) {
if (depop_subm(path))
die(...);
}
would have clarified the structure even further.
Yes, I know that you made depop_subm() to die on error, and the
above is questioning if that is a sensible design choice.
^ permalink raw reply
* Re: [PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir
From: Stefan Beller @ 2016-12-14 18:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, David Turner, Brandon Williams,
brian m. carlson
In-Reply-To: <xmqqbmwexs9x.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 14, 2016 at 10:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> It is a major reason to say no, when deciding if a submodule can be
>> deleted, if the git directory of the submodule being contained in the
>> submodule's working directory.
>>
>> Migrate the git directory into the superproject instead of failing,
>> and proceed with the other checks.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> submodule.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/submodule.c b/submodule.c
>> index 2d13744b06..e42efa2337 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1026,11 +1026,22 @@ int ok_to_remove_submodule(const char *path, unsigned flags)
>> struct strbuf buf = STRBUF_INIT;
>> int ok_to_remove = 1;
>>
>> + /* Is it there? */
>> if (!file_exists(path) || is_empty_dir(path))
>> return 1;
>>
>> - if (!submodule_uses_gitfile(path))
>> - return 0;
>> + /* Does it have a .git directory? */
>> + if (!submodule_uses_gitfile(path)) {
>> + absorb_git_dir_into_superproject("", path,
>> + ABSORB_GITDIR_RECURSE_SUBMODULES);
>> +
>> + /*
>> + * We should be using a gitfile by now. Let's double
>> + * check as losing the git dir would be fatal.
>> + */
>> + if (!submodule_uses_gitfile(path))
>> + return 0;
>> + }
>
> It feels a bit funny for a function that answers yes/no question to
> actually have a side effect, but probably it is OK. It feels dirty,
> but I dunno.
Another approach would be to return more than yes/no but the reason
why it is a no. And then the caller would call absorb_git_dir_into_superproject.
> A brief reading of the above makes us wonder what should happen if
> the absorb_git_dir_into_superproject() call fails, but a separate
> "submodule_uses_gitfile()" is needed to see if it failed? Perhaps
> the function needs to tell the caller if it succeeded?
I don't know about the double check as I think we should really be sure before
deleting that repo, but absorb_git_dir_into_superproject would fail/die
if it would not actually absorb it, so maybe it is too much caution.
So I'd omit the double check in a reroll.
>
>>
>> argv_array_pushl(&cp.args, "status", "--porcelain",
>> "--ignore-submodules=none", NULL);
^ permalink raw reply
* Re: [PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir
From: Junio C Hamano @ 2016-12-14 18:46 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, David.Turner, bmwill, sandals
In-Reply-To: <20161213205622.841-5-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> It is a major reason to say no, when deciding if a submodule can be
> deleted, if the git directory of the submodule being contained in the
> submodule's working directory.
>
> Migrate the git directory into the superproject instead of failing,
> and proceed with the other checks.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> submodule.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 2d13744b06..e42efa2337 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1026,11 +1026,22 @@ int ok_to_remove_submodule(const char *path, unsigned flags)
> struct strbuf buf = STRBUF_INIT;
> int ok_to_remove = 1;
>
> + /* Is it there? */
> if (!file_exists(path) || is_empty_dir(path))
> return 1;
>
> - if (!submodule_uses_gitfile(path))
> - return 0;
> + /* Does it have a .git directory? */
> + if (!submodule_uses_gitfile(path)) {
> + absorb_git_dir_into_superproject("", path,
> + ABSORB_GITDIR_RECURSE_SUBMODULES);
> +
> + /*
> + * We should be using a gitfile by now. Let's double
> + * check as losing the git dir would be fatal.
> + */
> + if (!submodule_uses_gitfile(path))
> + return 0;
> + }
It feels a bit funny for a function that answers yes/no question to
actually have a side effect, but probably it is OK. It feels dirty,
but I dunno.
A brief reading of the above makes us wonder what should happen if
the absorb_git_dir_into_superproject() call fails, but a separate
"submodule_uses_gitfile()" is needed to see if it failed? Perhaps
the function needs to tell the caller if it succeeded?
>
> argv_array_pushl(&cp.args, "status", "--porcelain",
> "--ignore-submodules=none", NULL);
^ permalink raw reply
* Re: [PATCHv2 2/5] submodule: modernize ok_to_remove_submodule to use argv_array
From: Junio C Hamano @ 2016-12-14 18:39 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, David.Turner, bmwill, sandals
In-Reply-To: <20161213205622.841-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Instead of constructing the NULL terminated array ourselves, we
> should make use of the argv_array infrastructure.
>
> While at it, adapt the error messages to reflect the actual invocation.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> submodule.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
Looks good.
>
> diff --git a/submodule.c b/submodule.c
> index 45ccfb7ab4..9f0b544ebe 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1023,13 +1023,6 @@ int ok_to_remove_submodule(const char *path)
> {
> ssize_t len;
> struct child_process cp = CHILD_PROCESS_INIT;
> - const char *argv[] = {
> - "status",
> - "--porcelain",
> - "-u",
> - "--ignore-submodules=none",
> - NULL,
> - };
> struct strbuf buf = STRBUF_INIT;
> int ok_to_remove = 1;
>
> @@ -1039,14 +1032,15 @@ int ok_to_remove_submodule(const char *path)
> if (!submodule_uses_gitfile(path))
> return 0;
>
> - cp.argv = argv;
> + argv_array_pushl(&cp.args, "status", "--porcelain", "-u",
> + "--ignore-submodules=none", NULL);
> prepare_submodule_repo_env(&cp.env_array);
> cp.git_cmd = 1;
> cp.no_stdin = 1;
> cp.out = -1;
> cp.dir = path;
> if (start_command(&cp))
> - die("Could not run 'git status --porcelain -uall --ignore-submodules=none' in submodule %s", path);
> + die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
>
> len = strbuf_read(&buf, cp.out, 1024);
> if (len > 2)
> @@ -1054,7 +1048,7 @@ int ok_to_remove_submodule(const char *path)
> close(cp.out);
>
> if (finish_command(&cp))
> - die("'git status --porcelain -uall --ignore-submodules=none' failed in submodule %s", path);
> + die(_("'git status --porcelain -u --ignore-submodules=none' failed in submodule %s"), path);
>
> strbuf_release(&buf);
> return ok_to_remove;
^ permalink raw reply
* Re: [PATCHv3 1/3] merge: Add '--continue' option as a synonym for 'git commit'
From: Junio C Hamano @ 2016-12-14 18:04 UTC (permalink / raw)
To: Chris Packham; +Cc: git, mah, peff, jacob.keller
In-Reply-To: <20161214083757.26412-1-judge.packham@gmail.com>
The last one 3/3 is a nice touch that makes sure that we do not
forget what we discovered during the discussion. Very much
appreciated.
Will queue. Thanks.
^ permalink raw reply
* Re: [PATCH v9 4/5] http: create function to get curl allowed protocols
From: Brandon Williams @ 2016-12-14 18:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214160330.iqvwxshsgk4n2gm7@sigill.intra.peff.net>
On 12/14, Jeff King wrote:
> On Tue, Dec 13, 2016 at 05:40:36PM -0800, Brandon Williams wrote:
>
> > Move the creation of an allowed protocols whitelist to a helper
> > function.
>
> This is "what" but not "why". You can figure it out if you see the next
> patch, but it's often nice to make a brief mention, like:
>
> This will be useful when we need to compute the set of allowed
> protocols differently for normal and redirect cases.
Commit message writing is hard (at least for me :). I'll update the
message to indicate the why.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Junio C Hamano @ 2016-12-14 17:57 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161214151009.4wdzjb44f6aki5ug@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Programs may use usage_msg_opt() to print a brief message
> followed by the program usage, and then exit. The message
> isn't prefixed at all, though, so it doesn't match our usual
> error output and is easy to overlook:
>
> $ git clone 1 2 3
> Too many arguments.
>
> usage: git clone [<options>] [--] <repo> [<dir>]
>
> -v, --verbose be more verbose
> -q, --quiet be more quiet
> --progress force progress reporting
> -n, --no-checkout don't create a checkout
> --bare create a bare repository
> [...and so on for another 31 lines...]
>
> It looks especially bad when the message starts with an
> option, like:
>
> $ git replace -e
> -e needs exactly one argument
>
> usage: git replace [-f] <object> <replacement>
> or: git replace [-f] --edit <object>
> [...etc...]
>
> Let's put our usual "fatal:" prefix in front of it.
I briefly wondered if any caller uses this in a non-fatal situation,
but usage_with_options() always dies, so this looks like the right
thing to do. Thanks.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Some of the message in git-clone could stand to be rewritten to match
> our usual style, too (no capitals, no trailing period), but that's
> obviously out of scope for this patch. I don't think this change makes
> them look any worse.
>
> parse-options.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 312a85dbd..4fbe924a5 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
> const char * const *usagestr,
> const struct option *options)
> {
> - fprintf(stderr, "%s\n\n", msg);
> + fprintf(stderr, "fatal: %s\n\n", msg);
> usage_with_options(usagestr, options);
> }
^ permalink raw reply
* Re: [PATCH v9 3/5] http: always warn if libcurl version is too old
From: Brandon Williams @ 2016-12-14 17:56 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, bburky, gitster, jrnieder
In-Reply-To: <20161214160123.a6a7fve5qz5xgg7n@sigill.intra.peff.net>
On 12/14, Jeff King wrote:
> On Tue, Dec 13, 2016 at 05:40:35PM -0800, Brandon Williams wrote:
>
> > diff --git a/transport.c b/transport.c
> > index e1ba78b..fbd799d 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -700,11 +700,6 @@ void transport_check_allowed(const char *type)
> > die("transport '%s' not allowed", type);
> > }
> >
> > -int transport_restrict_protocols(void)
> > -{
> > - return !!protocol_whitelist();
> > -}
> > -
>
> This function was subtly broken as of patch 2 of the series. It's
> probably not a big deal in the long run, but should the series be
> re-ordered to put this one first?
>
> I think the commit message would need adjusted, but it probably should
> mention the reasons this is a good idea even _without_ the new config
> system. Namely that even when there's no protocol whitelist, newer
> versions of curl have all of the other non-http protocols disabled.
>
> I wonder if anybody is actually using a version of curl old enough to
> trigger this. If so, they're going to get the warning every time they
> fetch via http. We might need to stick it behind an "advice.*" config
> option, though I'm inclined to leave it as-is and see if anybody
> actually complains.
>
> -Peff
Yeah you're right, transport_restrict_protocols() is definitely broken
after patch 2. Since I'm probably going to need to do a reroll based on
some of your comments in the other patches in the series we might as
well reorder patch 2 and 3 so this isn't broken between patches.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 4/6] update-unicode.sh: automatically download newer definition files
From: Junio C Hamano @ 2016-12-14 17:50 UTC (permalink / raw)
To: Beat Bolli; +Cc: Git List
In-Reply-To: <b137249e-728a-5d3c-4993-5ed5a1593737@drbeat.li>
Beat Bolli <dev+git@drbeat.li> writes:
> On 14.12.16 00:31, Beat Bolli wrote:
>
>> [PATCH v2 4/6] update-unicode.sh: automatically download newer definition files
>
> Dang! And again I'm not capable of putting an underline instead of the
> dash...
>
> Junio, would you please reword the subject to
>
> Re: [PATCH v2 4/6] update_unicode.sh: automatically download newer
> definition files
Will do.
This is an indication that the script is probably named against
people's expectation. We may want to rename it after the dust
settles.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 4/6] update-unicode.sh: automatically download newer definition files
From: Beat Bolli @ 2016-12-14 17:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <1481671904-1143-5-git-send-email-dev+git@drbeat.li>
On 14.12.16 00:31, Beat Bolli wrote:
> [PATCH v2 4/6] update-unicode.sh: automatically download newer definition files
Dang! And again I'm not capable of putting an underline instead of the
dash...
Junio, would you please reword the subject to
Re: [PATCH v2 4/6] update_unicode.sh: automatically download newer
definition files
Thanks,
Beat
> we should also download them if a newer version exists on the Unicode
> consortium's servers. Option -N of wget does this nicely for us.
>
> Reviewed-by: Torsten Bögershausen <tboegi@web.de>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
> contrib/update-unicode/update_unicode.sh | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/update-unicode/update_unicode.sh b/contrib/update-unicode/update_unicode.sh
> index 9f1bf31..56871a1 100755
> --- a/contrib/update-unicode/update_unicode.sh
> +++ b/contrib/update-unicode/update_unicode.sh
> @@ -8,12 +8,8 @@
> cd "$(dirname "$0")"
> UNICODEWIDTH_H=$(git rev-parse --show-toplevel)/unicode_width.h
>
> -if ! test -f UnicodeData.txt; then
> - wget http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt
> -fi &&
> -if ! test -f EastAsianWidth.txt; then
> - wget http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt
> -fi &&
> +wget -N http://www.unicode.org/Public/UCD/latest/ucd/UnicodeData.txt \
> + http://www.unicode.org/Public/UCD/latest/ucd/EastAsianWidth.txt &&
> if ! test -d uniset; then
> git clone https://github.com/depp/uniset.git &&
> ( cd uniset && git checkout 4b186196dd )
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox