[PATCH v5 0/2] fetch-pack: redact packfile urls in traces

["Ivan Frade via GitGitGadget" <gitgitgadget@gmail.com>]

Changes since v4:

 * Use "uri" instead of "url"
 * Look specifically for a line with packfile-uri format (instead of for a
   URL in general)
 * Limit the redacting to the packfile-uri section in do_fetch_pack_v2
 * Use "%.*s" instead of duplicating parts of the string to print

Changes since v3:

 * Enable redacting URLs for all sections
 * Redact only URL path (it was until the end of line)
 * Redact URL in die() with more friendly message
 * Update doc to mention that packfile URIs are also redacted.

Changes since v2:

 * Redact only the path of the URL
 * Test are now strict, validating the exact line expected in the log

Changes since v1:

 * Removed non-POSIX flags in tests
 * More accurate regex for the non-encrypted packfile line
 * Dropped documentation change
 * Dropped redacting the die message in http-fetch

Ivan Frade (2):
  fetch-pack: redact packfile urls in traces
  http-fetch: redact url on die() message

 Documentation/git.txt  |  5 +++--
 fetch-pack.c           |  4 ++++
 http-fetch.c           | 14 ++++++++++--
 pkt-line.c             | 39 +++++++++++++++++++++++++++++++-
 pkt-line.h             |  1 +
 t/t5702-protocol-v2.sh | 51 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 109 insertions(+), 5 deletions(-)


base-commit: e9e5ba39a78c8f5057262d49e261b42a8660d5b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1052%2Fifradeo%2Fredact-packfile-uri-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1052/ifradeo/redact-packfile-uri-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1052

Range-diff vs v4:

 1:  973a250752c ! 1:  c95b3cafcd6 fetch-pack: redact packfile urls in traces
     @@ Documentation/git.txt: for full details.
      -	cookies, the "Authorization:" header, and the "Proxy-Authorization:"
      -	header. Set this variable to `0` to prevent this redaction.
      +	cookies, the "Authorization:" header, the "Proxy-Authorization:"
     -+	header and packfile URLs. Set this variable to `0` to prevent this
     ++	header and packfile URIs. Set this variable to `0` to prevent this
      +	redaction.
       
       `GIT_LITERAL_PATHSPECS`::
     @@ Documentation/git.txt: for full details.
      
       ## fetch-pack.c ##
      @@ fetch-pack.c: static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
     - 		reader.me = "fetch-pack";
     - 	}
     + 				receive_wanted_refs(&reader, sought, nr_sought);
       
     -+	if (git_env_bool("GIT_TRACE_REDACT", 1))
     -+		reader.options |= PACKET_READ_REDACT_URL_PATH;
     -+
     - 	while (state != FETCH_DONE) {
     - 		switch (state) {
     - 		case FETCH_CHECK_LOCAL:
     + 			/* get the pack(s) */
     ++			if (git_env_bool("GIT_TRACE_REDACT", 1))
     ++				reader.options |= PACKET_READ_REDACT_URI_PATH;
     + 			if (process_section_header(&reader, "packfile-uris", 1))
     + 				receive_packfile_uris(&reader, &packfile_uris);
     ++			reader.options &= ~PACKET_READ_REDACT_URI_PATH;
     ++
     + 			process_section_header(&reader, "packfile", 0);
     + 
     + 			/*
      
       ## pkt-line.c ##
      @@ pkt-line.c: int packet_length(const char lenbuf_hex[4])
       	return (val < 0) ? val : (val << 8) | hex2chr(lenbuf_hex + 2);
       }
       
     -+static char *find_url_path(const char* buffer, int *path_len)
     ++static char *find_packfile_uri_path(const char *buffer)
      +{
     -+	const char *URL_MARK = "://";
     -+	char *path = strstr(buffer, URL_MARK);
     -+	if (!path)
     -+		return NULL;
     ++	const char *URI_MARK = "://";
     ++	char *path;
     ++	int len;
      +
     -+	path += strlen(URL_MARK);
     -+	while (*path && *path != '/')
     -+		path++;
     ++	/* First char is sideband mark */
     ++	buffer += 1;
      +
     -+	if (!*path || !*(path + 1))
     -+		return NULL;
     ++	len = strspn(buffer, "0123456789abcdefABCDEF");
     ++	if (!(len == 40 || len == 64) || buffer[len] != ' ')
     ++		return NULL; /* required "<hash>SP" not seen */
      +
     -+	// position after '/'
     -+	path++;
     ++	path = strstr(buffer + len + 1, URI_MARK);
     ++	if (!path)
     ++		return NULL;
      +
     -+	if (path_len) {
     -+		char *url_end = strchrnul(path, ' ');
     -+		*path_len = url_end - path;
     -+	}
     ++	path = strchr(path + strlen(URI_MARK), '/');
     ++	if (!path || !*(path + 1))
     ++		return NULL;
      +
     -+	return path;
     ++	/* position after '/' */
     ++	return ++path;
      +}
      +
       enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
     @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
       {
       	int len;
       	char linelen[4];
     -+	char *url_path_start;
     -+	int url_path_len;
     ++	char *uri_path_start;
       
       	if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0) {
       		*pktlen = -1;
     @@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
       
       	buffer[len] = 0;
      -	packet_trace(buffer, len, 0);
     -+	if (options & PACKET_READ_REDACT_URL_PATH &&
     -+	    (url_path_start = find_url_path(buffer, &url_path_len))) {
     ++	if (options & PACKET_READ_REDACT_URI_PATH &&
     ++	    (uri_path_start = find_packfile_uri_path(buffer))) {
      +		const char *redacted = "<redacted>";
      +		struct strbuf tracebuf = STRBUF_INIT;
      +		strbuf_insert(&tracebuf, 0, buffer, len);
     -+		strbuf_splice(&tracebuf, url_path_start - buffer,
     -+			      url_path_len, redacted, strlen(redacted));
     ++		strbuf_splice(&tracebuf, uri_path_start - buffer,
     ++			      strlen(uri_path_start), redacted, strlen(redacted));
      +		packet_trace(tracebuf.buf, tracebuf.len, 0);
      +		strbuf_release(&tracebuf);
      +	} else {
     @@ pkt-line.h: void packet_fflush(FILE *f);
       #define PACKET_READ_CHOMP_NEWLINE        (1u<<1)
       #define PACKET_READ_DIE_ON_ERR_PACKET    (1u<<2)
       #define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
     -+#define PACKET_READ_REDACT_URL_PATH      (1u<<4)
     ++#define PACKET_READ_REDACT_URI_PATH      (1u<<4)
       int packet_read(int fd, char *buffer, unsigned size, int options);
       
       /*
 2:  c7f0977cabd ! 2:  6912a690197 http-fetch: redact url on die() message
     @@ Commit message
          http-fetch: redact url on die() message
      
          http-fetch prints the URL after failing to fetch it. This can be
     -    confusing to users (they cannot really do anything with it) but even
     -    worse, they can share by accident a sensitive URL (e.g. with
     -    credentials) while looking for help.
     +    confusing to users (they cannot really do anything with it), and they
     +    can share by accident a sensitive URL (e.g. with credentials) while
     +    looking for help.
      
          Redact the URL unless the GIT_TRACE_REDACT variable is set to false. This
          mimics the redaction of other sensitive information in git, like the
          Authorization header in HTTP.
      
     +    Fix also capitalization of previous die() message (must start in
     +    lowercase).
     +
          Signed-off-by: Ivan Frade <ifrade@google.com>
      
       ## http-fetch.c ##
     @@ http-fetch.c: static void fetch_single_packfile(struct object_id *packfile_hash,
      -			    curl_errorstr);
      +			struct url_info url;
      +			char *nurl = url_normalize(preq->url, &url);
     -+			if (!git_env_bool("GIT_TRACE_REDACT", 1) || !nurl) {
     -+				die("Unable to get pack file %s\n%s", preq->url,
     ++			if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
     ++				die("unable to get pack file '%s'\n%s", preq->url,
      +				    curl_errorstr);
      +			} else {
     -+				char *schema = xstrndup(url.url, url.scheme_len);
     -+				char *host = xstrndup(&url.url[url.host_off], url.host_len);
     -+				die("failed to get '%s' url from '%s' "
     ++				die("failed to get '%.*s' url from '%.*s' "
      +				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
     -+				    schema, host, curl_errorstr);
     ++				    (int)url.scheme_len, url.url,
     ++				    (int)url.host_len, &url.url[url.host_off], curl_errorstr);
      +			}
       		}
       	} else {

-- 
gitgitgadget