Skip to contentSkip to conversation

gitweb: redacted e-mail addresses feature.

PATCH Superseded Georgios Kontaxis via GitGitGadget 6 participants 41 messages Mar 21, 2021 – Apr 8, 2021 Documentation

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 gitweb: redacted e-mail addresses feature.

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

---
gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/910

 Documentation/gitweb.conf.txt | 12 ++++++++++++
 gitweb/gitweb.perl            | 36 ++++++++++++++++++++++++++++++++---
 2 files changed, 45 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +12 -0
index 7963a79ba98b..10653d8670a8 100644
@@ -896,6 +896,18 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git check-ref-format"
 scrutiny. Duplicated values are filtered.
 
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web crawlers.
+    Enabled by default.
++
+It is highly recommended to keep this feature enabled unless web crawlers
+are hindered in some other way. You can disable this feature as shown below:
++
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [0];
+---------------------------------------------------------------------------
+
 
 EXAMPLES
 --------
gitweb/gitweb.perl +33 -3
index 0959a782eccb..9d21c2583e18 100755
@@ -569,6 +569,15 @@ sub evaluate_uri {
 		'sub' => \&feature_extra_branch_refs,
 		'override' => 0,
 		'default' => []},
+
+    # Redact e-mail addresses.
+
+    # To disable system wide have in $GITWEB_CONFIG
+    # $feature{'email_privacy'}{'default'} = [0];
+	'email_privacy' => {
+		'sub' => sub { feature_bool('email_privacy', @_) },
+		'override' => 0,
+		'default' => [1]},
 );
 
 sub gitweb_get_feature {
@@ -3471,6 +3480,10 @@ sub parse_tag {
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$tag{'author_name'}  = $1;
 				$tag{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$tag{'author_email'} = "private";
+					$tag{'author'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$tag{'author_name'} = $tag{'author'};
 			}
@@ -3519,6 +3532,10 @@ sub parse_commit_text {
 			if ($co{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'author_name'}  = $1;
 				$co{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'author_email'} = "private";
+					$co{'author'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$co{'author_name'} = $co{'author'};
 			}
@@ -3529,6 +3546,10 @@ sub parse_commit_text {
 			if ($co{'committer'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'committer_name'}  = $1;
 				$co{'committer_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'committer_email'} = "private";
+					$co{'committer'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$co{'committer_name'} = $co{'committer'};
 			}
@@ -3568,9 +3589,13 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		if (gitweb_check_feature('email_privacy') &&
+			$line =~ m/^([^<]+) <([^>]*)>/) {
+			$line =~ s/<([^>]+)>/<private>/;
+		}
 	}
 	$co{'comment'} = \@commit_lines;
 
@@ -8060,8 +8085,13 @@ sub git_commitdiff {
 		close $fd
 			or print "Reading git-diff-tree failed\n";
 	} elsif ($format eq 'patch') {
-		local $/ = undef;
-		print <$fd>;
+		while (my $line = <$fd>) {
+			if (gitweb_check_feature('email_privacy') &&
+				$line =~ m/^([^<]+) <([^>]*)>/) {
+				$line =~ s/<([^>]+)>/<private>/;
+			}
+			print $line;
+		}
 		close $fd
 			or print "Reading git-format-patch failed\n";
 	}

base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 8 lines · Georgios Kontaxis via GitGitGadget

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

Quoted 4 lines

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

So web crawlers that aren't going to obey robots.txt?

I'm not opposed to this feature, but a glance at gitweb's documentation seems to show that we don't discuss how to set robots.txt up for it at all.

Perhaps having that in the docs or otherwise in the default setup would get us most of the win of this feature?

Quoted 2 lines

---

Odd:

Quoted 7 lines

gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

Quoted 5 lines

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

To have this duplication of the patch here below "---", some GGG feature gone awry?

Quoted 17 lines

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/910

 Documentation/gitweb.conf.txt | 12 ++++++++++++
 gitweb/gitweb.perl            | 36 ++++++++++++++++++++++++++++++++---
 2 files changed, 45 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +1 -0
index 7963a79ba98b..10653d8670a8 100644
@@ -896,6 +896,18 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git check-ref-format"
 scrutiny. Duplicated values are filtered.
 
+email_privacy::

Documentation/gitweb.conf.txt same as of the snippet above: · line 909

Quoted 10 lines
Documentation/gitweb.conf.txt +10 -0
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web crawlers.
+    Enabled by default.
++
+It is highly recommended to keep this feature enabled unless web crawlers
+are hindered in some other way. You can disable this feature as shown below:
++
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [0];
+---------------------------------------------------------------------------

I think there's plenty of gitweb users that are going to be relying on the current behavior, so doesn't it make more sense for this to be opt-in rather than opt-out?

Quoted 60 lines
Documentation/gitweb.conf.txt +0 -0
 EXAMPLES
 --------
gitweb/gitweb.perl +21 -0
index 0959a782eccb..9d21c2583e18 100755
@@ -569,6 +569,15 @@ sub evaluate_uri {
 		'sub' => \&feature_extra_branch_refs,
 		'override' => 0,
 		'default' => []},
+
+    # Redact e-mail addresses.
+
+    # To disable system wide have in $GITWEB_CONFIG
+    # $feature{'email_privacy'}{'default'} = [0];
+	'email_privacy' => {
+		'sub' => sub { feature_bool('email_privacy', @_) },
+		'override' => 0,
+		'default' => [1]},
 );
[...]
 sub gitweb_get_feature {
@@ -3471,6 +3480,10 @@ sub parse_tag {
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$tag{'author_name'}  = $1;
 				$tag{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$tag{'author_email'} = "private";
+					$tag{'author'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$tag{'author_name'} = $tag{'author'};
 			}
@@ -3519,6 +3532,10 @@ sub parse_commit_text {
 			if ($co{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'author_name'}  = $1;
 				$co{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'author_email'} = "private";
+					$co{'author'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$co{'author_name'} = $co{'author'};
 			}
@@ -3529,6 +3546,10 @@ sub parse_commit_text {
 			if ($co{'committer'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'committer_name'}  = $1;
 				$co{'committer_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'committer_email'} = "private";
+					$co{'committer'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$co{'committer_name'} = $co{'committer'};
 			}
@@ -3568,9 +3589,13 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}

gitweb/gitweb.perl sub parse_commit_text { · line 3600

Quoted 10 lines
gitweb/gitweb.perl +5 -1
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		if (gitweb_check_feature('email_privacy') &&
+			$line =~ m/^([^<]+) <([^>]*)>/) {
+			$line =~ s/<([^>]+)>/<private>/;
+		}
 	}
 	$co{'comment'} = \@commit_lines;

All of these hunks (and the below) should use some new function that does this feature check + sanitizing instead of copy/pasting mostly the same code N times. e.g.:

    sub maybe_hide_email {
        my $email = shift;
        return $email unless gitweb_check_feature('email_privacy');
        return hide_email($email);
    }

then:

    $tag{author_email} = maybe_hide_email($2);

Also it looks like this isn't a new issue, but does this need to implement its own E-Mail parser? We ship with Mail::Address for git-send-email, can gitweb (and the elided hide_email() function above) use that too?

Quoted 5 lines
gitweb/gitweb.perl +0 -1
@@ -8060,8 +8085,13 @@ sub git_commitdiff {
 		close $fd
 			or print "Reading git-diff-tree failed\n";
 	} elsif ($format eq 'patch') {
-		local $/ = undef;

gitweb/gitweb.perl sub git_commitdiff { · line 8096

Quoted 10 lines
gitweb/gitweb.perl +7 -1
-		print <$fd>;
+		while (my $line = <$fd>) {
+			if (gitweb_check_feature('email_privacy') &&
+				$line =~ m/^([^<]+) <([^>]*)>/) {
+				$line =~ s/<([^>]+)>/<private>/;
+			}
+			print $line;
+		}
 		close $fd
 			or print "Reading git-format-patch failed\n";

Is that "patch" output meant for "git am"? Won't this severely break that use-case if so?

#2 lore raw
Quoted 16 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Georgios Kontaxis via GitGitGadget

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

Quoted 4 lines

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Quoted 8 lines

So web crawlers that aren't going to obey robots.txt?

I'm not opposed to this feature, but a glance at gitweb's documentation seems to show that we don't discuss how to set robots.txt up for it at all.

Perhaps having that in the docs or otherwise in the default setup would get us most of the win of this feature?

I'm going to guess that the two features are orthogonal. robots.txt is great for communicating to well-meaning actors what you do and don't want crawled. For example, one might ask a web crawler not to crawl individual commits because that creates excessive load on the server.

This option is about preventing email harvesting, usually for the purposes of sending spam. Spam is email abuse and all reasonable people know it's unacceptable, so by definition the people doing this are bad actors and are not likely to honor the robots.txt. As someone who runs his own mail server, that is certainly my experience.

So I am in favor of this feature. I think it mirrors what many other tools do in this space and having it as an option is valuable.

Quoted 8 lines
Quoted 8 lines
Documentation/gitweb.conf.txt +0 -0
index 7963a79ba98b..10653d8670a8 100644
@@ -896,6 +896,18 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git check-ref-format"
 scrutiny. Duplicated values are filtered.
 
Quoted 15 lines

Documentation/gitweb.conf.txt same as of the snippet above: · line 909

Quoted 11 lines
Documentation/gitweb.conf.txt +11 -0
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web crawlers.
+    Enabled by default.
++
+It is highly recommended to keep this feature enabled unless web crawlers
+are hindered in some other way. You can disable this feature as shown below:
++
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [0];
+---------------------------------------------------------------------------

I think there's plenty of gitweb users that are going to be relying on the current behavior, so doesn't it make more sense for this to be opt-in rather than opt-out?

I agree this make sense as an opt-in feature. While many people will want to enable it, users who are performing an upgrade won't necessarily want the behavior to change right away.

Signature · 2 lines
brian m. carlson (he/him or they/them)
Houston, Texas, US
#3 lore raw
In reply to Ævar Arnfjörð Bjarmason
Quoted 16 lines · Georgios Kontaxis via GitGitGadget
Quoted 9 lines

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

Quoted 4 lines

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Quoted 9 lines

So web crawlers that aren't going to obey robots.txt?

I'm not opposed to this feature, but a glance at gitweb's documentation seems to show that we don't discuss how to set robots.txt up for it at all.

Perhaps having that in the docs or otherwise in the default setup would get us most of the win of this feature?

File robots.txt is basically asking nicely and we should work on that. At the same time crawlers that look for addresses to send SPAM to will probably ignore it so this change is meant for them.

Quoted 55 lines
Quoted 2 lines

---

Odd:

Quoted 10 lines

gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible
over
HTTP. As a result, e-mail addresses found in commits are exposed to
web
crawlers. This may result in unsolicited messages. This is a feature
for
redacting e-mail addresses from the generated HTML content.

Quoted 6 lines

This feature does not prevent someone from downloading the
unredacted
commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

To have this duplication of the patch here below "---", some GGG feature gone awry?

Quoted 30 lines

Published-As:
https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-910/kontaxis/kontaxis/email_privacy-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/910

 Documentation/gitweb.conf.txt | 12 ++++++++++++
 gitweb/gitweb.perl            | 36 ++++++++++++++++++++++++++++++++---
 2 files changed, 45 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +0 -0
index 7963a79ba98b..10653d8670a8 100644
@@ -896,6 +896,18 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git

check-ref-format"
scrutiny. Duplicated values are filtered.

Documentation/gitweb.conf.txt +3 -0
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web

crawlers.

Documentation/gitweb.conf.txt +3 -0
+    Enabled by default.
++
+It is highly recommended to keep this feature enabled unless web

crawlers
+are hindered in some other way. You can disable this feature as shown below:

Quoted 9 lines
Quoted 4 lines

++
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [0];
+---------------------------------------------------------------------------

I think there's plenty of gitweb users that are going to be relying on the current behavior, so doesn't it make more sense for this to be opt-in rather than opt-out?

My concern is that Gitweb operators may not understand the need for this feature or maybe won't be aware the feature exists.

Nevertheless, I've changed the feature to be off by default. Perhaps we can revisit this decision in the future? :)

Quoted 81 lines
Quoted 60 lines

EXAMPLES
--------

gitweb/gitweb.perl +21 -0
index 0959a782eccb..9d21c2583e18 100755
@@ -569,6 +569,15 @@ sub evaluate_uri {
 		'sub' => \&feature_extra_branch_refs,
 		'override' => 0,
 		'default' => []},
+
+    # Redact e-mail addresses.
+
+    # To disable system wide have in $GITWEB_CONFIG
+    # $feature{'email_privacy'}{'default'} = [0];
+	'email_privacy' => {
+		'sub' => sub { feature_bool('email_privacy', @_) },
+		'override' => 0,
+		'default' => [1]},
 );
[...]
 sub gitweb_get_feature {
@@ -3471,6 +3480,10 @@ sub parse_tag {
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$tag{'author_name'}  = $1;
 				$tag{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$tag{'author_email'} = "private";
+					$tag{'author'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$tag{'author_name'} = $tag{'author'};
 			}
@@ -3519,6 +3532,10 @@ sub parse_commit_text {
 			if ($co{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'author_name'}  = $1;
 				$co{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'author_email'} = "private";
+					$co{'author'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$co{'author_name'} = $co{'author'};
 			}
@@ -3529,6 +3546,10 @@ sub parse_commit_text {
 			if ($co{'committer'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'committer_name'}  = $1;
 				$co{'committer_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'committer_email'} = "private";
+					$co{'committer'} =~ s/<([^>]+)>/<private>/;
+				}
 			} else {
 				$co{'committer_name'} = $co{'committer'};
 			}
@@ -3568,9 +3589,13 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}

gitweb/gitweb.perl sub parse_commit_text { · line 3600

Quoted 10 lines
gitweb/gitweb.perl +5 -1
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		if (gitweb_check_feature('email_privacy') &&
+			$line =~ m/^([^<]+) <([^>]*)>/) {
+			$line =~ s/<([^>]+)>/<private>/;
+		}
 	}
 	$co{'comment'} = \@commit_lines;

All of these hunks (and the below) should use some new function that does this feature check + sanitizing instead of copy/pasting mostly the same code N times. e.g.:

    sub maybe_hide_email {
        my $email = shift;
        return $email unless gitweb_check_feature('email_privacy');
        return hide_email($email);
    }
Quoted 9 lines

then:

    $tag{author_email} = maybe_hide_email($2);

Also it looks like this isn't a new issue, but does this need to implement its own E-Mail parser? We ship with Mail::Address for git-send-email, can gitweb (and the elided hide_email() function above) use that too?

Thanks for the suggestion.
I've removed the duplicate code.

The places where there are e-mail addresses today are pretty specific. (Pretty much the author, committer fields and the Signed-off-by lines)

In theory someone could write a comment with a bunch of addresses in it but that would be unstructured text and I think Mail::Address is not good with that.

I can definitely keep working on this topic but maybe in subsequent PRs? Assuming we're not exposing any addresses right now or redacting things we shouldn't.

Quoted 11 lines
Quoted 11 lines
gitweb/gitweb.perl +4 -2
@@ -8060,8 +8085,13 @@ sub git_commitdiff {
 		close $fd
 			or print "Reading git-diff-tree failed\n";
 	} elsif ($format eq 'patch') {
-		local $/ = undef;
-		print <$fd>;
+		while (my $line = <$fd>) {
+			if (gitweb_check_feature('email_privacy') &&
+				$line =~ m/^([^<]+) <([^>]*)>/) {
+				$line =~ s/<([^>]+)>/<private>/;
Quoted 9 lines

gitweb/gitweb.perl sub git_commitdiff { · line 8096

Quoted 5 lines
gitweb/gitweb.perl +3 -0
+			}
+			print $line;
+		}
 		close $fd
 			or print "Reading git-format-patch failed\n";

Is that "patch" output meant for "git am"? Won't this severely break that use-case if so?

Not sure who may be consuming that output. I've added a note in the documentation for gitweb.conf.

If a web crawler can get to the information by following URLs then I think we should redact it.

Hopefully by documenting this as a potential issue Gitweb operators can create a workaround specific to their use case. Possibly implementing access control and leaving this feature off.

#4 lore raw

PATCH v2 1/1 gitweb: redacted e-mail addresses feature.

In reply to Georgios Kontaxis via GitGitGadget

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.
This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Changes since v1:
- Turned off the feature by default.
- Removed duplicate code.
- Added note about Gitweb consumers receiving redacted logs.

---
gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v1:

1: 6fe6ebdb8e59 ! 1: 74af11ca8bf2 gitweb: redacted e-mail addresses feature.

     @@ Commit message
      
          Gitweb extracts content from the Git log and makes it accessible
          over HTTP. As a result, e-mail addresses found in commits are
     -    exposed to web crawlers. This may result in unsolicited messages.
     -    This is a feature for redacting e-mail addresses from the generated
     -    HTML content.
     +    exposed to web crawlers and they may not respect robots.txt.
     +    This may result in unsolicited messages.
     +    This is a feature for redacting e-mail addresses
     +    from the generated HTML, etc. content.
      
          This feature does not prevent someone from downloading the
     -    unredacted commit log and extracting information from it.
     +    unredacted commit log, e.g., by cloning the repository, and
     +    extracting information from it.
          It aims to hinder the low-effort bulk collection of e-mail
          addresses by web crawlers.
      
     +    Changes since v1:
     +    - Turned off the feature by default.
     +    - Removed duplicate code.
     +    - Added note about Gitweb consumers receiving redacted logs.
     +
          Signed-off-by: Georgios Kontaxis <[email protected]>
      
       ## Documentation/gitweb.conf.txt ##
     @@ Documentation/gitweb.conf.txt: same as of the snippet above:
      +email_privacy::
      +    Redact e-mail addresses from the generated HTML, etc. content.
      +    This hides e-mail addresses found in the commit log from web crawlers.
     -+    Enabled by default.
     ++    Disabled by default.
      ++
     -+It is highly recommended to keep this feature enabled unless web crawlers
     -+are hindered in some other way. You can disable this feature as shown below:
     ++It is highly recommended to enable this feature unless web crawlers are
     ++hindered in some other way. Note that crawlers intent on harvesting e-mail
     ++addresses may disregard robots.txt. You can enable this feature like so:
      ++
      +---------------------------------------------------------------------------
     -+$feature{'email_privacy'}{'default'} = [0];
     ++$feature{'email_privacy'}{'default'} = [1];
      +---------------------------------------------------------------------------
     +++
     ++Note that if Gitweb is not the final step in a workflow then subsequent
     ++steps may misbehave because of the redacted information they receive.
      +
       
       EXAMPLES
     @@ gitweb/gitweb.perl: sub evaluate_uri {
       		'override' => 0,
       		'default' => []},
      +
     -+    # Redact e-mail addresses.
     ++	# Redact e-mail addresses.
      +
     -+    # To disable system wide have in $GITWEB_CONFIG
     -+    # $feature{'email_privacy'}{'default'} = [0];
     ++	# To enable system wide have in $GITWEB_CONFIG
     ++	# $feature{'email_privacy'}{'default'} = [1];
      +	'email_privacy' => {
      +		'sub' => sub { feature_bool('email_privacy', @_) },
      +		'override' => 0,
     -+		'default' => [1]},
     ++		'default' => [0]},
       );
       
       sub gitweb_get_feature {
     +@@ gitweb/gitweb.perl: sub parse_date {
     + 	return %date;
     + }
     + 
     ++sub hide_mailaddr_if_private {
     ++	my $line = shift;
     ++	return $line unless (gitweb_check_feature('email_privacy') &&
     ++						$line =~ m/^([^<]+) <([^>]*)>/);
     ++	return hide_mailaddr($line)
     ++}
     ++
     ++sub hide_mailaddr {
     ++	my $mailaddr = shift;
     ++	$mailaddr =~ s/<([^>]*)>/<private>/;
     ++	return $mailaddr;
     ++}
     ++
     + sub parse_tag {
     + 	my $tag_id = shift;
     + 	my %tag;
      @@ gitweb/gitweb.perl: sub parse_tag {
       			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
       				$tag{'author_name'}  = $1;
       				$tag{'author_email'} = $2;
      +				if (gitweb_check_feature('email_privacy')) {
      +					$tag{'author_email'} = "private";
     -+					$tag{'author'} =~ s/<([^>]+)>/<private>/;
     ++					$tag{'author'} = hide_mailaddr($tag{'author'});
      +				}
       			} else {
       				$tag{'author_name'} = $tag{'author'};
     @@ gitweb/gitweb.perl: sub parse_commit_text {
       				$co{'author_email'} = $2;
      +				if (gitweb_check_feature('email_privacy')) {
      +					$co{'author_email'} = "private";
     -+					$co{'author'} =~ s/<([^>]+)>/<private>/;
     ++					$co{'author'} = hide_mailaddr($co{'author'});
      +				}
       			} else {
       				$co{'author_name'} = $co{'author'};
     @@ gitweb/gitweb.perl: sub parse_commit_text {
       				$co{'committer_email'} = $2;
      +				if (gitweb_check_feature('email_privacy')) {
      +					$co{'committer_email'} = "private";
     -+					$co{'committer'} =~ s/<([^>]+)>/<private>/;
     ++					$co{'committer'} = hide_mailaddr($co{'committer'});
      +				}
       			} else {
       				$co{'committer_name'} = $co{'committer'};
     @@ gitweb/gitweb.perl: sub parse_commit_text {
      +	# remove added spaces, redact e-mail addresses if applicable.
       	foreach my $line (@commit_lines) {
       		$line =~ s/^    //;
     -+		if (gitweb_check_feature('email_privacy') &&
     -+			$line =~ m/^([^<]+) <([^>]*)>/) {
     -+			$line =~ s/<([^>]+)>/<private>/;
     -+		}
     ++		$line = hide_mailaddr_if_private($line);
       	}
       	$co{'comment'} = \@commit_lines;
       
     @@ gitweb/gitweb.perl: sub git_commitdiff {
      -		local $/ = undef;
      -		print <$fd>;
      +		while (my $line = <$fd>) {
     -+			if (gitweb_check_feature('email_privacy') &&
     -+				$line =~ m/^([^<]+) <([^>]*)>/) {
     -+				$line =~ s/<([^>]+)>/<private>/;
     -+			}
     -+			print $line;
     ++			print hide_mailaddr_if_private($line);
      +		}
       		close $fd
       			or print "Reading git-format-patch failed\n";
 Documentation/gitweb.conf.txt | 16 +++++++++++++
 gitweb/gitweb.perl            | 42 ++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +16 -0
index 7963a79ba98b..b7af3240177d 100644
@@ -896,6 +896,22 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git check-ref-format"
 scrutiny. Duplicated values are filtered.
 
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web crawlers.
+    Disabled by default.
++
+It is highly recommended to enable this feature unless web crawlers are
+hindered in some other way. Note that crawlers intent on harvesting e-mail
+addresses may disregard robots.txt. You can enable this feature like so:
++
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [1];
+---------------------------------------------------------------------------
++
+Note that if Gitweb is not the final step in a workflow then subsequent
+steps may misbehave because of the redacted information they receive.
+
 
 EXAMPLES
 --------
gitweb/gitweb.perl +39 -3

86 lines not on this page · raw message

base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f

Signature · 1 line
gitgitgadget
#5 lore raw

PATCH v3 1/1 gitweb: redacted e-mail addresses feature.

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.
This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Changes since v1:
- Turned off the feature by default.
- Removed duplicate code.
- Added note about Gitweb consumers receiving redacted logs.

Changes since v2:
- The feature can be set on a per-project basis. ('override' => 1)

---
gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v2:

1: 74af11ca8bf2 ! 1: 930cdefe7ee0 gitweb: redacted e-mail addresses feature.

     @@ Commit message
          - Removed duplicate code.
          - Added note about Gitweb consumers receiving redacted logs.
      
     +    Changes since v2:
     +    - The feature can be set on a per-project basis. ('override' => 1)
     +
          Signed-off-by: Georgios Kontaxis <[email protected]>
      
       ## Documentation/gitweb.conf.txt ##
     @@ gitweb/gitweb.perl: sub evaluate_uri {
      +	# $feature{'email_privacy'}{'default'} = [1];
      +	'email_privacy' => {
      +		'sub' => sub { feature_bool('email_privacy', @_) },
     -+		'override' => 0,
     ++		'override' => 1,
      +		'default' => [0]},
       );
 Documentation/gitweb.conf.txt | 16 +++++++++++++
 gitweb/gitweb.perl            | 42 ++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +16 -0
index 7963a79ba98b..b7af3240177d 100644
@@ -896,6 +896,22 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git check-ref-format"
 scrutiny. Duplicated values are filtered.
 
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web crawlers.
+    Disabled by default.
++
+It is highly recommended to enable this feature unless web crawlers are
+hindered in some other way. Note that crawlers intent on harvesting e-mail
+addresses may disregard robots.txt. You can enable this feature like so:
++
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [1];
+---------------------------------------------------------------------------
++
+Note that if Gitweb is not the final step in a workflow then subsequent
+steps may misbehave because of the redacted information they receive.
+
 
 EXAMPLES
 --------
gitweb/gitweb.perl +39 -3

86 lines not on this page · raw message

base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f

Signature · 1 line
gitgitgadget
#6 lore raw
Quoted 118 lines · Georgios Kontaxis via GitGitGadget

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.
This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Changes since v1:
- Turned off the feature by default.
- Removed duplicate code.
- Added note about Gitweb consumers receiving redacted logs.

Changes since v2:
- The feature can be set on a per-project basis. ('override' => 1)

---
gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v2:

1: 74af11ca8bf2 ! 1: 930cdefe7ee0 gitweb: redacted e-mail addresses feature.

     @@ Commit message
          - Removed duplicate code.
          - Added note about Gitweb consumers receiving redacted logs.
      
     +    Changes since v2:
     +    - The feature can be set on a per-project basis. ('override' => 1)
     +
          Signed-off-by: Georgios Kontaxis <[email protected]>
      
       ## Documentation/gitweb.conf.txt ##
     @@ gitweb/gitweb.perl: sub evaluate_uri {
      +	# $feature{'email_privacy'}{'default'} = [1];
      +	'email_privacy' => {
      +		'sub' => sub { feature_bool('email_privacy', @_) },
     -+		'override' => 0,
     ++		'override' => 1,
      +		'default' => [0]},
       );
 Documentation/gitweb.conf.txt | 16 +++++++++++++
 gitweb/gitweb.perl            | 42 ++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +16 -0
index 7963a79ba98b..b7af3240177d 100644
@@ -896,6 +896,22 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git check-ref-format"
 scrutiny. Duplicated values are filtered.
 
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web crawlers.
+    Disabled by default.
++
+It is highly recommended to enable this feature unless web crawlers are
+hindered in some other way. Note that crawlers intent on harvesting e-mail
+addresses may disregard robots.txt. You can enable this feature like so:
++
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [1];
+---------------------------------------------------------------------------
++
+Note that if Gitweb is not the final step in a workflow then subsequent
+steps may misbehave because of the redacted information they receive.
+
 
 EXAMPLES
 --------
gitweb/gitweb.perl +9 -0
index 0959a782eccb..174cc566d97d 100755
@@ -569,6 +569,15 @@ sub evaluate_uri {
 		'sub' => \&feature_extra_branch_refs,
 		'override' => 0,
 		'default' => []},
+
+	# Redact e-mail addresses.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'email_privacy'}{'default'} = [1];
+	'email_privacy' => {
+		'sub' => sub { feature_bool('email_privacy', @_) },
+		'override' => 1,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {

gitweb/gitweb.perl sub parse_date { · line 3460

Quoted 4 lines
gitweb/gitweb.perl +0 -0
@@ -3449,6 +3458,19 @@ sub parse_date {
 	return %date;
 }
 
[snip]

So in the v1 feedback I suggested:

BEGIN QUOTE

    sub maybe_hide_email {
        my $email = shift;
        return $email unless gitweb_check_feature('email_privacy');
        return hide_email($email);
    }

then:

    $tag{author_email} = maybe_hide_email($2);

END QUOTE

But:

gitweb/gitweb.perl sub parse_tag { · line 3499

Quoted 11 lines

sub parse_tag {

gitweb/gitweb.perl +4 -0
 	my $tag_id = shift;
 	my %tag;
@@ -3471,6 +3493,10 @@ sub parse_tag {
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$tag{'author_name'}  = $1;
 				$tag{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$tag{'author_email'} = "private";
+					$tag{'author'} = hide_mailaddr($tag{'author'});
+				}

This code seems quite awkward, we've already done the regex match, but this code:

gitweb/gitweb.perl sub parse_date { · line 3472

Quoted 13 lines

[snip]

gitweb/gitweb.perl +12 -0
+sub hide_mailaddr_if_private {
+	my $line = shift;
+	return $line unless (gitweb_check_feature('email_privacy') &&
+						$line =~ m/^([^<]+) <([^>]*)>/);
+	return hide_mailaddr($line)
+}
+
+sub hide_mailaddr {
+	my $mailaddr = shift;
+	$mailaddr =~ s/<([^>]*)>/<private>/;
+	return $mailaddr;
+}

Is going to do it again incrementally, and then just act on a search-replacement if we've got the feature enabled.

It seems much simpler to just turn your:

gitweb/gitweb.perl sub parse_tag { · line 3499

Quoted 4 lines
gitweb/gitweb.perl +4 -0
+				if (gitweb_check_feature('email_privacy')) {
+					$tag{'author_email'} = "private";
+					$tag{'author'} = hide_mailaddr($tag{'author'});
+				}

Into:

    $tag{'author'} = maybe_hide_mailaddr($tag{author}, \$tag{author_email});

Using:

    sub maybe_hide_email {
        my ($email, $ref) = shift;
        return $email unless gitweb_check_feature('email_privacy');
        $$ref = "private" if $ref;
        return hide_email($email);
    }

Which also works for the case where you don't have a "private" hash key to assign to. But maybe it overcomplicates things...

Quoted 39 lines
gitweb/gitweb.perl +10 -1
 			} else {
 				$tag{'author_name'} = $tag{'author'};
 			}
@@ -3519,6 +3545,10 @@ sub parse_commit_text {
 			if ($co{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'author_name'}  = $1;
 				$co{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'author_email'} = "private";
+					$co{'author'} = hide_mailaddr($co{'author'});
+				}
 			} else {
 				$co{'author_name'} = $co{'author'};
 			}
@@ -3529,6 +3559,10 @@ sub parse_commit_text {
 			if ($co{'committer'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'committer_name'}  = $1;
 				$co{'committer_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'committer_email'} = "private";
+					$co{'committer'} = hide_mailaddr($co{'committer'});
+				}
[...]
 			} else {
 				$co{'committer_name'} = $co{'committer'};
 			}
@@ -3568,9 +3602,10 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		$line = hide_mailaddr_if_private($line);
 	}
 	$co{'comment'} = \@commit_lines;
 

gitweb/gitweb.perl sub git_commitdiff { · line 8100

Quoted 8 lines
gitweb/gitweb.perl +3 -2
@@ -8060,8 +8095,9 @@ sub git_commitdiff {
 		close $fd
 			or print "Reading git-diff-tree failed\n";
 	} elsif ($format eq 'patch') {
-		local $/ = undef;
-		print <$fd>;
+		while (my $line = <$fd>) {
+			print hide_mailaddr_if_private($line);
+		}

Urm, have you tested this? How does a while loop over a <$fd> make sense when $/ is undef, the readline() operator will always return just one record, so having a while loop doesn't make sense.

I'm not sure of the input here, but given that if you're expecting to replace all e-mail addresses on all lines with this function that's not how it'll work, the s/// doesn't have a /g, so it'll stop at the first replacement.

Quoted 5 lines
gitweb/gitweb.perl +0 -0
 		close $fd
 			or print "Reading git-format-patch failed\n";
 	}

base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f

#7 lore raw
Quoted 17 lines · Ævar Arnfjörð Bjarmason

But:

gitweb/gitweb.perl sub parse_tag { · line 3499

Quoted 11 lines

sub parse_tag {

gitweb/gitweb.perl +4 -0
 	my $tag_id = shift;
 	my %tag;
@@ -3471,6 +3493,10 @@ sub parse_tag {
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$tag{'author_name'}  = $1;
 				$tag{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$tag{'author_email'} = "private";
+					$tag{'author'} = hide_mailaddr($tag{'author'});
+				}

This code seems quite awkward, we've already done the regex match, but this code:

Quoted 16 lines

gitweb/gitweb.perl sub parse_date { · line 3472

Quoted 13 lines

[snip]

gitweb/gitweb.perl +12 -0
+sub hide_mailaddr_if_private {
+	my $line = shift;
+	return $line unless (gitweb_check_feature('email_privacy') &&
+						$line =~ m/^([^<]+) <([^>]*)>/);
+	return hide_mailaddr($line)
+}
+
+sub hide_mailaddr {
+	my $mailaddr = shift;
+	$mailaddr =~ s/<([^>]*)>/<private>/;
+	return $mailaddr;
+}

Is going to do it again incrementally, and then just act on a search-replacement if we've got the feature enabled.

I think you misread the patch the same way as I did initially. What is called in parse_tag is not the "if-private" version---the caller knows that $tag{'author'} MUST BE an address so unconditionally redact when the feature is set by bypassing the _if_private variant.

Having said that, I do think

Quoted 6 lines
    sub maybe_hide_email {
        my ($email, $ref) = shift;
        return $email unless gitweb_check_feature('email_privacy');
        $$ref = "private" if $ref;
        return hide_email($email);
    }

this helper is how you would simplify these numerous callers.

Quoted 11 lines

gitweb/gitweb.perl sub git_commitdiff { · line 8100

Quoted 6 lines
gitweb/gitweb.perl +3 -2
 	} elsif ($format eq 'patch') {
-		local $/ = undef;
-		print <$fd>;
+		while (my $line = <$fd>) {
+			print hide_mailaddr_if_private($line);
+		}

Urm, have you tested this? How does a while loop over a <$fd> make sense when $/ is undef, the readline() operator will always return just one record, so having a while loop doesn't make sense.

Quoted 8 lines

I'm not sure of the input here, but given that if you're expecting to replace all e-mail addresses on all lines with this function that's not how it'll work, the s/// doesn't have a /g, so it'll stop at the first replacement.

Quoted 3 lines
gitweb/gitweb.perl +0 -0
 		close $fd
 			or print "Reading git-format-patch failed\n";
 	}

All true. This one is reading from "format-patch --stdout" output, but for the reasons I mentioned in my review, I do not think it should touch the 'patch' output at all.

#8 lore raw
In reply to Ævar Arnfjörð Bjarmason
Quoted 170 lines · Georgios Kontaxis via GitGitGadget
Quoted 132 lines

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.
This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Changes since v1:
- Turned off the feature by default.
- Removed duplicate code.
- Added note about Gitweb consumers receiving redacted logs.

Changes since v2:
- The feature can be set on a per-project basis. ('override' => 1)

---
gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible
over
HTTP. As a result, e-mail addresses found in commits are exposed to
web
crawlers. This may result in unsolicited messages. This is a feature
for
redacting e-mail addresses from the generated HTML content.

This feature does not prevent someone from downloading the
unredacted
commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Published-As:
https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-910/kontaxis/kontaxis/email_privacy-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v2:

1: 74af11ca8bf2 ! 1: 930cdefe7ee0 gitweb: redacted e-mail addresses
feature.
@@ Commit message
- Removed duplicate code.
- Added note about Gitweb consumers receiving redacted logs.

+ Changes since v2:
+ - The feature can be set on a per-project basis. ('override'
=> 1)

     +
          Signed-off-by: Georgios Kontaxis <[email protected]>

       ## Documentation/gitweb.conf.txt ##
     @@ gitweb/gitweb.perl: sub evaluate_uri {
      +	# $feature{'email_privacy'}{'default'} = [1];
      +	'email_privacy' => {
      +		'sub' => sub { feature_bool('email_privacy', @_) },
     -+		'override' => 0,
     ++		'override' => 1,
      +		'default' => [0]},
       );
 Documentation/gitweb.conf.txt | 16 +++++++++++++
 gitweb/gitweb.perl            | 42 ++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +0 -0
index 7963a79ba98b..b7af3240177d 100644
@@ -896,6 +896,22 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git

check-ref-format"
scrutiny. Duplicated values are filtered.

Documentation/gitweb.conf.txt +3 -0
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web

crawlers.

Documentation/gitweb.conf.txt +4 -0
+    Disabled by default.
++
+It is highly recommended to enable this feature unless web crawlers are
+hindered in some other way. Note that crawlers intent on harvesting

e-mail
+addresses may disregard robots.txt. You can enable this feature like so:
++
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [1];
+---------------------------------------------------------------------------
++

Documentation/gitweb.conf.txt +3 -0
+Note that if Gitweb is not the final step in a workflow then subsequent
+steps may misbehave because of the redacted information they receive.
+
 
 EXAMPLES
 --------
gitweb/gitweb.perl +9 -0
index 0959a782eccb..174cc566d97d 100755
@@ -569,6 +569,15 @@ sub evaluate_uri {
 		'sub' => \&feature_extra_branch_refs,
 		'override' => 0,
 		'default' => []},
+
+	# Redact e-mail addresses.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'email_privacy'}{'default'} = [1];
+	'email_privacy' => {
+		'sub' => sub { feature_bool('email_privacy', @_) },
+		'override' => 1,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {

gitweb/gitweb.perl sub parse_date { · line 3460

Quoted 4 lines
gitweb/gitweb.perl +0 -0
@@ -3449,6 +3458,19 @@ sub parse_date {
 	return %date;
 }
 
[snip]

So in the v1 feedback I suggested:

BEGIN QUOTE

    sub maybe_hide_email {
        my $email = shift;
        return $email unless gitweb_check_feature('email_privacy');
        return hide_email($email);
    }

then:

    $tag{author_email} = maybe_hide_email($2);

END QUOTE

But:

gitweb/gitweb.perl sub parse_tag { · line 3499

Quoted 11 lines

sub parse_tag {

gitweb/gitweb.perl +4 -0
 	my $tag_id = shift;
 	my %tag;
@@ -3471,6 +3493,10 @@ sub parse_tag {
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$tag{'author_name'}  = $1;
 				$tag{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$tag{'author_email'} = "private";
+					$tag{'author'} = hide_mailaddr($tag{'author'});
+				}

This code seems quite awkward, we've already done the regex match, but this code:

Quoted 17 lines

gitweb/gitweb.perl sub parse_date { · line 3472

Quoted 13 lines

[snip]

gitweb/gitweb.perl +12 -0
+sub hide_mailaddr_if_private {
+	my $line = shift;
+	return $line unless (gitweb_check_feature('email_privacy') &&
+						$line =~ m/^([^<]+) <([^>]*)>/);
+	return hide_mailaddr($line)
+}
+
+sub hide_mailaddr {
+	my $mailaddr = shift;
+	$mailaddr =~ s/<([^>]*)>/<private>/;
+	return $mailaddr;
+}

Is going to do it again incrementally, and then just act on a search-replacement if we've got the feature enabled.

I think hide_mailaddr_if_private (which checks the feature flag and looks for the pattern) is only called when we haven't done so already. In every other case we call hide_mailaddr directly. So there's no duplication of checks.

Quoted 62 lines

It seems much simpler to just turn your:

gitweb/gitweb.perl sub parse_tag { · line 3499

Quoted 4 lines
gitweb/gitweb.perl +4 -0
+				if (gitweb_check_feature('email_privacy')) {
+					$tag{'author_email'} = "private";
+					$tag{'author'} = hide_mailaddr($tag{'author'});
+				}

Into:

    $tag{'author'} = maybe_hide_mailaddr($tag{author},

\$tag{author_email});

Using:

    sub maybe_hide_email {
        my ($email, $ref) = shift;
        return $email unless gitweb_check_feature('email_privacy');
        $$ref = "private" if $ref;
        return hide_email($email);
    }

Which also works for the case where you don't have a "private" hash key to assign to. But maybe it overcomplicates things...

Quoted 38 lines
gitweb/gitweb.perl +10 -1
 			} else {
 				$tag{'author_name'} = $tag{'author'};
 			}
@@ -3519,6 +3545,10 @@ sub parse_commit_text {
 			if ($co{'author'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'author_name'}  = $1;
 				$co{'author_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'author_email'} = "private";
+					$co{'author'} = hide_mailaddr($co{'author'});
+				}
 			} else {
 				$co{'author_name'} = $co{'author'};
 			}
@@ -3529,6 +3559,10 @@ sub parse_commit_text {
 			if ($co{'committer'} =~ m/^([^<]+) <([^>]*)>/) {
 				$co{'committer_name'}  = $1;
 				$co{'committer_email'} = $2;
+				if (gitweb_check_feature('email_privacy')) {
+					$co{'committer_email'} = "private";
+					$co{'committer'} = hide_mailaddr($co{'committer'});
+				}
[...]
 			} else {
 				$co{'committer_name'} = $co{'committer'};
 			}
@@ -3568,9 +3602,10 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		$line = hide_mailaddr_if_private($line);
 	}
 	$co{'comment'} = \@commit_lines;
 
Quoted 14 lines

gitweb/gitweb.perl sub git_commitdiff { · line 8100

Quoted 9 lines
gitweb/gitweb.perl +3 -2
@@ -8060,8 +8095,9 @@ sub git_commitdiff {
 		close $fd
 			or print "Reading git-diff-tree failed\n";
 	} elsif ($format eq 'patch') {
-		local $/ = undef;
-		print <$fd>;
+		while (my $line = <$fd>) {
+			print hide_mailaddr_if_private($line);
+		}

Urm, have you tested this? How does a while loop over a <$fd> make sense when $/ is undef, the readline() operator will always return just one record, so having a while loop doesn't make sense.

$/ is not undef, the opposite, it's the default value which results in reading from <$fd> one line at a time. (where line is a sequence terminated by CRLF or LF)

Quoted 5 lines

I'm not sure of the input here, but given that if you're expecting to replace all e-mail addresses on all lines with this function that's not how it'll work, the s/// doesn't have a /g, so it'll stop at the first replacement.

So we are reading from <$fd> one line at a time and looking for the first "expr1 <expr2>" occurrence in each line. This successfully redacts the author, committer, signed-off-by lines.

Not doing a global replacement per line is intentional so as to avoid false positives.
So far I haven't noticed missing any addresses that should have been redacted.

Quoted 7 lines
Quoted 5 lines
gitweb/gitweb.perl +0 -0
 		close $fd
 			or print "Reading git-format-patch failed\n";
 	}

base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f

#9 lore raw
In reply to Georgios Kontaxis via GitGitGadget

"Georgios Kontaxis via GitGitGadget" <[email protected]>

Quoted 9 lines · writes:

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.
This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

Quoted 5 lines

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Lines from here ...

Quoted 8 lines

Changes since v1:
- Turned off the feature by default.
- Removed duplicate code.
- Added note about Gitweb consumers receiving redacted logs.

Changes since v2:
- The feature can be set on a per-project basis. ('override' => 1)

... to here are to help reviewers on the mailing list while iterations of the same change is being reviewed and polished.

But it is useless noise for those who only read "git log". They simply do not have access to earlier iterations.

Such "changes between iterations" comments needs to be written after the three-dash lines.

Quoted 53 lines

---
gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML content.

This feature does not prevent someone from downloading the unredacted commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v2:

1: 74af11ca8bf2 ! 1: 930cdefe7ee0 gitweb: redacted e-mail addresses feature.

     @@ Commit message
          - Removed duplicate code.
          - Added note about Gitweb consumers receiving redacted logs.
      
     +    Changes since v2:
     +    - The feature can be set on a per-project basis. ('override' => 1)
     +
          Signed-off-by: Georgios Kontaxis <[email protected]>
      
       ## Documentation/gitweb.conf.txt ##
     @@ gitweb/gitweb.perl: sub evaluate_uri {
      +	# $feature{'email_privacy'}{'default'} = [1];
      +	'email_privacy' => {
      +		'sub' => sub { feature_bool('email_privacy', @_) },
     -+		'override' => 0,
     ++		'override' => 1,
      +		'default' => [0]},
       );
 Documentation/gitweb.conf.txt | 16 +++++++++++++
 gitweb/gitweb.perl            | 42 ++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +0 -0
index 7963a79ba98b..b7af3240177d 100644
@@ -896,6 +896,22 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git check-ref-format"

Documentation/gitweb.conf.txt same as of the snippet above: · line 906

Quoted 10 lines
Documentation/gitweb.conf.txt +8 -0
 scrutiny. Duplicated values are filtered.
 
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web crawlers.
+    Disabled by default.
++
+It is highly recommended to enable this feature unless web crawlers are
+hindered in some other way. Note that crawlers intent on harvesting e-mail
+addresses may disregard robots.txt.

Up to this line is more-or-less OK, but with a few comments:

- "This hides ... from web crawlers"? Doesn't this hide it indiscriminately, whether the requester is an interactive end-user or a crawler?

- The name of the configured feature, 'email_privacy'. There is another existing feature with underscore in its name (remote_heads), but I think it is an odd-ball mistake we do not want to imitate. Instead, I think the "extra branch refs" may be a good example to follow. The feature name (written in Perl code) is "extra-branch-refs" (downcased words with inter-word dashes) and the corresponding configuration (I am not saying we should add one to support this feature, if we do not have one already) is "gitweb.extraBranchRefs" (camelCased words).

- Do not exaggerate with words like "highly", but trust the intelligence of your readers to make the right decision when they understand the reason why this feature exists in the first place.

- I do not think this entry should be added to the end of feature list. How about listing it just after 'avatar', which is also about how author/committer identity is shown?

Documentation/gitweb.conf.txt same as of the snippet above: · line 910

Quoted 3 lines
Documentation/gitweb.conf.txt +3 -0
+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [1];
+---------------------------------------------------------------------------

I do not think this should be here. None of the boolean features listed early in "Features in %feature" section like blame, snapshot, grep, pickaxe, ... features tell readers how to enable a specific feature like that.

Unless the syntax to configure a feature is one-off oddball that is different from all other features, we shouldn't clutter the description with an example like this.

At the beginning of the section "Configuring Gitweb Features" there is a general description and that should be sufficient (and if it is not sufficient, it is OK to add an example to that section).

Documentation/gitweb.conf.txt same as of the snippet above: · line 913

Quoted 2 lines
Documentation/gitweb.conf.txt +2 -0
+Note that if Gitweb is not the final step in a workflow then subsequent
+steps may misbehave because of the redacted information they receive.

In other words, this will break crawlers that expect email addresses are there and want to use it for some good purpose. But that is an natural consequence of the "feature", and should be described as such when we said "Redact e-mail addresses", not as a "by the way" mention in a footnote.

... after reading more, readers realize that the damage is far worse in the current incarnation of the patch, but let's read on ...

Documentation/gitweb.conf.txt sub parse_date { · line 3464

Quoted 8 lines
Documentation/gitweb.conf.txt +4 -0
@@ -3449,6 +3458,19 @@ sub parse_date {
 	return %date;
 }
 
+sub hide_mailaddr_if_private {
+	my $line = shift;
+	return $line unless (gitweb_check_feature('email_privacy') &&
+						$line =~ m/^([^<]+) <([^>]*)>/);

I find that the second line is way too deeply indented. Wouldn't

Documentation/gitweb.conf.txt sub parse_date { · line 3466

Quoted 2 lines
Documentation/gitweb.conf.txt +2 -0
+	return $line unless (gitweb_check_feature('email_privacy') &&
+				$line =~ m/^([^<]+) <([^>]*)>/);

be enough?

Documentation/gitweb.conf.txt sub parse_date { · line 3472

Quoted 6 lines
Documentation/gitweb.conf.txt +6 -0
+	return hide_mailaddr($line)
+}
+
+sub hide_mailaddr {
+	my $mailaddr = shift;
+	$mailaddr =~ s/<([^>]*)>/<private>/;

s/private/redacted/ perhaps?

Documentation/gitweb.conf.txt sub parse_commit_text { · line 3608

Quoted 9 lines
Documentation/gitweb.conf.txt +2 -1
@@ -3568,9 +3602,10 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		$line = hide_mailaddr_if_private($line);

A commit log that ends with

Thank you, A <[email protected]> and B <[email protected]>, for discovering this bug and quickly proposing a solution.

would recact only the first one but not the other, I suspect.

Much more problematic is that I see too many hits from:

    $ git log v2.30.0..v2.31.0 | grep ' <[^>@]*>'

That is, "find a line in the commit log message that has a SP, open <bra, run of characters that are not ket> or at@sign, closed with ket>". These are clearly not e-mail addresses.

This "feature" butchers a commit title:

mktag doc: say <hash> not <sha1>

to

mktag doc: say <private> not <sha1>

doesn't it?

Documentation/gitweb.conf.txt sub git_commitdiff { · line 8100

Quoted 9 lines
Documentation/gitweb.conf.txt +3 -2
@@ -8060,8 +8095,9 @@ sub git_commitdiff {
 		close $fd
 			or print "Reading git-diff-tree failed\n";
 	} elsif ($format eq 'patch') {
-		local $/ = undef;
-		print <$fd>;
+		while (my $line = <$fd>) {
+			print hide_mailaddr_if_private($line);
+		}

And this is even worse.

 $ git log -p --no-merges v2.30.0..v2.31.0 | grep ' <[^>@]*>' | wc -l

gives me ~4700 hits. Because the patch text as well as the log message is munged, the "feature" makes the output utterly unreliable as an input to "git am" by munging too much. Interesting examples are like these:

        -	for (i = 0; i < pairs->nr; ++i) {
                'git <command> [<revision>...] -- [<file>...]'
        +	for (i = split; i < geometry->pack_nr; i++) {

Now, I am *NOT* saying that you should tighten the e-mail address syntax detection and keep munging the patch text. The lines that begin with minus and SP in a patch must match the preimage text, so munging out existing e-mail addresses from them will make the patch fail to find the part that needs to be modified. And replacing an e-mail address on a line that begins with plus would redact it from the source---if they wrote an address, they must have meant it to be available to those who consume the source, so I doubt the wisdom of munging the patch part at all.

I may be sympathetic to the cause of the patch, but, I do not agree with its execution in this iteration of the patch.

Thanks.

#10 lore raw
Quoted 11 lines · Junio C Hamano

gitweb/gitweb.perl sub git_commitdiff { · line 8100

Quoted 9 lines
gitweb/gitweb.perl +3 -2
@@ -8060,8 +8095,9 @@ sub git_commitdiff {
 		close $fd
 			or print "Reading git-diff-tree failed\n";
 	} elsif ($format eq 'patch') {
-		local $/ = undef;
-		print <$fd>;
+		while (my $line = <$fd>) {
+			print hide_mailaddr_if_private($line);
+		}

And this is even worse.

And also Ævar is right. The original just has a format-patch output in $_ and prints it to the output stream in one go. It won't make any sense to attempt reading from that stream, which the original code used to write to, at all.

In any case, I've already said that I do not think it is a good idea to touch the 'patch' codepath at all, so this code won't do any damage in the end by reading from the output pipe connected to end-user's browser ;-)

Thanks.

#11 lore raw
Quoted 2 lines

And also Ævar is right. The original just has a format-patch output in $_ and prints it to the output stream in one go.

Sorry but I misread the patch. By dropping the $/ = undef, it wants to read from $fd (which is "format-patch --stdout" output) one line at a time. So unless a line has two addresses, this would "work" as intended.

Again, as I said, I do not agree with the definition of "work" here, though ;-).

#12 lore raw
In reply to Junio C Hamano
Quoted 116 lines

"Georgios Kontaxis via GitGitGadget" <[email protected]>

Quoted 9 lines · writes:

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.
This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

Quoted 5 lines

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Lines from here ...

Quoted 8 lines

Changes since v1:
- Turned off the feature by default.
- Removed duplicate code.
- Added note about Gitweb consumers receiving redacted logs.

Changes since v2:
- The feature can be set on a per-project basis. ('override' => 1)

... to here are to help reviewers on the mailing list while iterations of the same change is being reviewed and polished.

But it is useless noise for those who only read "git log". They simply do not have access to earlier iterations.

Such "changes between iterations" comments needs to be written after the three-dash lines.

Quoted 73 lines

---
gitweb: Redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible
over
HTTP. As a result, e-mail addresses found in commits are exposed to
web
crawlers. This may result in unsolicited messages. This is a feature
for
redacting e-mail addresses from the generated HTML content.

This feature does not prevent someone from downloading the
unredacted
commit log and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Published-As:
https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-910/kontaxis/kontaxis/email_privacy-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v2:

1: 74af11ca8bf2 ! 1: 930cdefe7ee0 gitweb: redacted e-mail addresses
feature.
@@ Commit message
- Removed duplicate code.
- Added note about Gitweb consumers receiving redacted logs.

+ Changes since v2:
+ - The feature can be set on a per-project basis. ('override'
=> 1)

     +
          Signed-off-by: Georgios Kontaxis <[email protected]>

       ## Documentation/gitweb.conf.txt ##
     @@ gitweb/gitweb.perl: sub evaluate_uri {
      +	# $feature{'email_privacy'}{'default'} = [1];
      +	'email_privacy' => {
      +		'sub' => sub { feature_bool('email_privacy', @_) },
     -+		'override' => 0,
     ++		'override' => 1,
      +		'default' => [0]},
       );
 Documentation/gitweb.conf.txt | 16 +++++++++++++
 gitweb/gitweb.perl            | 42 ++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 3 deletions(-)
Documentation/gitweb.conf.txt +0 -0
index 7963a79ba98b..b7af3240177d 100644
@@ -896,6 +896,22 @@ same as of the snippet above:
 It is an error to specify a ref that does not pass "git

check-ref-format"
scrutiny. Duplicated values are filtered.

Documentation/gitweb.conf.txt +3 -0
+email_privacy::
+    Redact e-mail addresses from the generated HTML, etc. content.
+    This hides e-mail addresses found in the commit log from web

crawlers.

Documentation/gitweb.conf.txt +4 -0
+    Disabled by default.
++
+It is highly recommended to enable this feature unless web crawlers are
+hindered in some other way. Note that crawlers intent on harvesting
Quoted 2 lines

e-mail
+addresses may disregard robots.txt.

Up to this line is more-or-less OK, but with a few comments:

Quoted 4 lines

- "This hides ... from web crawlers"? Doesn't this hide it indiscriminately, whether the requester is an interactive end-user or a crawler?

Correct. It doesn't say "it only hides from".
I can see how it may be confusing. I'll rephrase.

Quoted 10 lines

- The name of the configured feature, 'email_privacy'. There is another existing feature with underscore in its name (remote_heads), but I think it is an odd-ball mistake we do not want to imitate. Instead, I think the "extra branch refs" may be a good example to follow. The feature name (written in Perl code) is "extra-branch-refs" (downcased words with inter-word dashes) and the corresponding configuration (I am not saying we should add one to support this feature, if we do not have one already) is "gitweb.extraBranchRefs" (camelCased words).

I'll rename the feature.

Quoted 4 lines

- Do not exaggerate with words like "highly", but trust the intelligence of your readers to make the right decision when they understand the reason why this feature exists in the first place.

Will remove.

Quoted 4 lines

- I do not think this entry should be added to the end of feature list. How about listing it just after 'avatar', which is also about how author/committer identity is shown?

Will do.

Quoted 9 lines
Quoted 3 lines

+---------------------------------------------------------------------------
+$feature{'email_privacy'}{'default'} = [1];
+---------------------------------------------------------------------------

I do not think this should be here. None of the boolean features listed early in "Features in %feature" section like blame, snapshot, grep, pickaxe, ... features tell readers how to enable a specific feature like that.

Quoted 8 lines

Unless the syntax to configure a feature is one-off oddball that is different from all other features, we shouldn't clutter the description with an example like this.

At the beginning of the section "Configuring Gitweb Features" there is a general description and that should be sufficient (and if it is not sufficient, it is OK to add an example to that section).

Will remove.

Quoted 9 lines

Documentation/gitweb.conf.txt same as of the snippet above: · line 913

Quoted 2 lines
Documentation/gitweb.conf.txt +2 -0
+Note that if Gitweb is not the final step in a workflow then subsequent
+steps may misbehave because of the redacted information they receive.

In other words, this will break crawlers that expect email addresses are there and want to use it for some good purpose. But that is an natural consequence of the "feature", and should be described as such when we said "Redact e-mail addresses", not as a "by the way" mention in a footnote.

Quoted 4 lines

... after reading more, readers realize that the damage is far worse in the current incarnation of the patch, but let's read on ...

Will move this out of the footnote.

Quoted 11 lines

Documentation/gitweb.conf.txt sub parse_date { · line 3464

Quoted 8 lines
Documentation/gitweb.conf.txt +4 -0
@@ -3449,6 +3458,19 @@ sub parse_date {
 	return %date;
 }
 
+sub hide_mailaddr_if_private {
+	my $line = shift;
+	return $line unless (gitweb_check_feature('email_privacy') &&
+						$line =~ m/^([^<]+) <([^>]*)>/);

I find that the second line is way too deeply indented. Wouldn't

Quoted 5 lines

Documentation/gitweb.conf.txt sub parse_date { · line 3466

Quoted 2 lines
Documentation/gitweb.conf.txt +2 -0
+	return $line unless (gitweb_check_feature('email_privacy') &&
+				$line =~ m/^([^<]+) <([^>]*)>/);

be enough?

Will adjust.

Quoted 9 lines

Documentation/gitweb.conf.txt sub parse_date { · line 3472

Quoted 6 lines
Documentation/gitweb.conf.txt +6 -0
+	return hide_mailaddr($line)
+}
+
+sub hide_mailaddr {
+	my $mailaddr = shift;
+	$mailaddr =~ s/<([^>]*)>/<private>/;

s/private/redacted/ perhaps?

Will adjust.

Quoted 27 lines

Documentation/gitweb.conf.txt sub parse_commit_text { · line 3608

Quoted 9 lines
Documentation/gitweb.conf.txt +2 -1
@@ -3568,9 +3602,10 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		$line = hide_mailaddr_if_private($line);

A commit log that ends with

Thank you, A <[email protected]> and B <[email protected]>, for discovering this bug and quickly proposing a solution.

would recact only the first one but not the other, I suspect.

Much more problematic is that I see too many hits from:

    $ git log v2.30.0..v2.31.0 | grep ' <[^>@]*>'

That is, "find a line in the commit log message that has a SP, open <bra, run of characters that are not ket> or at@sign, closed with ket>". These are clearly not e-mail addresses.

This "feature" butchers a commit title:

Quoted 8 lines

mktag doc: say <hash> not <sha1>

to

mktag doc: say <private> not <sha1>

doesn't it?

True. Thanks for pointing these out.
Sounds like the pattern we're looking for needs to be more specific.

Quoted 33 lines

Documentation/gitweb.conf.txt sub git_commitdiff { · line 8100

Quoted 9 lines
Documentation/gitweb.conf.txt +3 -2
@@ -8060,8 +8095,9 @@ sub git_commitdiff {
 		close $fd
 			or print "Reading git-diff-tree failed\n";
 	} elsif ($format eq 'patch') {
-		local $/ = undef;
-		print <$fd>;
+		while (my $line = <$fd>) {
+			print hide_mailaddr_if_private($line);
+		}

And this is even worse.

 $ git log -p --no-merges v2.30.0..v2.31.0 | grep ' <[^>@]*>' | wc -l

gives me ~4700 hits. Because the patch text as well as the log message is munged, the "feature" makes the output utterly unreliable as an input to "git am" by munging too much. Interesting examples are like these:

        -	for (i = 0; i < pairs->nr; ++i) {
                'git <command> [<revision>...] -- [<file>...]'
        +	for (i = split; i < geometry->pack_nr; i++) {

Now, I am *NOT* saying that you should tighten the e-mail address syntax detection and keep munging the patch text. The lines that begin with minus and SP in a patch must match the preimage text, so munging out existing e-mail addresses from them will make the patch fail to find the part that needs to be modified. And replacing an e-mail address on a line that begins with plus would redact it from the source---if they wrote an address, they must have meant it to be available to those who consume the source, so I doubt the wisdom of munging the patch part at all.

Quoted 3 lines

I may be sympathetic to the cause of the patch, but, I do not agree with its execution in this iteration of the patch.

I see your point.

It seems hiding e-mail addresses should be limited to the commit message, i.e., stop at the "---" line.

Quoted 3 lines

Thanks.

#13 lore raw
Quoted 10 lines · Georgios Kontaxis
Quoted 6 lines

... so I doubt
the wisdom of munging the patch part at all.

I may be sympathetic to the cause of the patch, but, I do not agree with its execution in this iteration of the patch.

I see your point.

It seems hiding e-mail addresses should be limited to the commit message, i.e., stop at the "---" line.

I doubt it makes sense to redact anything in the 'patch' view at all, actually. What kind of URL does the crawler need to formulate and what pieces of information (like commit object names or branch names) does it need to fill in the URL to get a series of patches out of gitweb? As long as it takes more effort than running "git clone" against the repository, the crawler would not have much incentive to crawl and harvest addresses from the 'patch' pages, and even in the log message part, the downsides of butchering the payload would outweigh the "privacy benefit", I would have to say.

Quite honestly, if a site claims to offer a 'patch' download UI but returns corrupt data back, I would say it is much worse than not offering the service at all. Perhaps disabling the 'patch' feature in repositories that enable 'privacy' feature may be a much better approach.

#14 lore raw
Quoted 14 lines
Quoted 11 lines · Georgios Kontaxis
Quoted 6 lines

... so I doubt
the wisdom of munging the patch part at all.

I may be sympathetic to the cause of the patch, but, I do not agree with its execution in this iteration of the patch.

I see your point.

It seems hiding e-mail addresses should be limited to the commit message,
i.e., stop at the "---" line.

Quoted 10 lines

I doubt it makes sense to redact anything in the 'patch' view at all, actually. What kind of URL does the crawler need to formulate and what pieces of information (like commit object names or branch names) does it need to fill in the URL to get a series of patches out of gitweb? As long as it takes more effort than running "git clone" against the repository, the crawler would not have much incentive to crawl and harvest addresses from the 'patch' pages, and even in the log message part, the downsides of butchering the payload would outweigh the "privacy benefit", I would have to say.

No effort at all I would say.
E..g, somehow the web crawler gets to git.kernel.org.
It then follows every link, eventually arriving at a commitdiff page. It then follows every link, which includes the URL for the patch output. See how "wget --mirror" behaves for instance.

Just to clarify, my goal is not to stop someone who wants to extract e-mail address from git.kernel.org specifically.
They can just "git clone" the repositories and grep through the logs. My goal is to stop generic crawlers (pretty much "wget --mirror | grep" scripts) from making their way to the logs.

Quoted 6 lines

Quite honestly, if a site claims to offer a 'patch' download UI but returns corrupt data back, I would say it is much worse than not offering the service at all. Perhaps disabling the 'patch' feature in repositories that enable 'privacy' feature may be a much better approach.

Good point. I think I'll try that.

#15 lore raw
Quoted 7 lines · Georgios Kontaxis
Quoted 6 lines

Quite honestly, if a site claims to offer a 'patch' download UI but returns corrupt data back, I would say it is much worse than not offering the service at all. Perhaps disabling the 'patch' feature in repositories that enable 'privacy' feature may be a much better approach.

Good point. I think I'll try that.

Alternatively, we could add 'captcha' to stop crawlers, perhaps?

#16 lore raw

PATCH v4 1/1 gitweb: redacted e-mail addresses feature.

In reply to Georgios Kontaxis via GitGitGadget

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.
This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

---
gitweb: redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML, etc. content.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

Changes since v1:

* Turned off the feature by default.
* Removed duplicate code.
* Added note about Gitweb consumers receiving redacted logs.

Changes since v2:

* The feature can be set on a per-project basis. ('override' => 1)

Changes since v3:

* Renamed feature to "email-privacy" and improved documentation.
* Removed UI elements for git-format-patch since it won't be redacted.
* Simplified calls to the address redaction logic.
* Mail::Address is now used to reduce false-positive redactions.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v3:

1: 930cdefe7ee0 ! 1: 03a3f41c37ef gitweb: redacted e-mail addresses feature.

     @@ Commit message
          It aims to hinder the low-effort bulk collection of e-mail
          addresses by web crawlers.
      
     -    Changes since v1:
     -    - Turned off the feature by default.
     -    - Removed duplicate code.
     -    - Added note about Gitweb consumers receiving redacted logs.
     -
     -    Changes since v2:
     -    - The feature can be set on a per-project basis. ('override' => 1)
     -
          Signed-off-by: Georgios Kontaxis <[email protected]>
      
       ## Documentation/gitweb.conf.txt ##
     -@@ Documentation/gitweb.conf.txt: same as of the snippet above:
     - It is an error to specify a ref that does not pass "git check-ref-format"
     - scrutiny. Duplicated values are filtered.
     +@@ Documentation/gitweb.conf.txt: default font sizes or lineheights are changed (e.g. via adding extra
     + CSS stylesheet in `@stylesheets`), it may be appropriate to change
     + these values.
       
     -+email_privacy::
     -+    Redact e-mail addresses from the generated HTML, etc. content.
     -+    This hides e-mail addresses found in the commit log from web crawlers.
     -+    Disabled by default.
     -++
     -+It is highly recommended to enable this feature unless web crawlers are
     -+hindered in some other way. Note that crawlers intent on harvesting e-mail
     -+addresses may disregard robots.txt. You can enable this feature like so:
     -++
     -+---------------------------------------------------------------------------
     -+$feature{'email_privacy'}{'default'} = [1];
     -+---------------------------------------------------------------------------
     -++
     -+Note that if Gitweb is not the final step in a workflow then subsequent
     -+steps may misbehave because of the redacted information they receive.
     ++email-privacy::
     ++	Redact e-mail addresses from the generated HTML, etc. content.
     ++	This hides e-mail addresses found in the commit log from HTTP clients.
     ++	It is meant to hinder web crawlers that harvest and abuse addresses.
     ++	Such crawlers may not respect robots.txt.
     ++	Note that users and user tools also see the addresses redacted.
     ++	If Gitweb is not the final step in a workflow then subsequent steps
     ++	may misbehave because of the redacted information they receive.
     ++	Disabled by default.
      +
     - 
     - EXAMPLES
     - --------
     + highlight::
     + 	Server-side syntax highlight support in "blob" view.  It requires
     + 	`$highlight_bin` program to be available (see the description of
      
       ## gitweb/gitweb.perl ##
     +@@
     + use File::Basename qw(basename);
     + use Time::HiRes qw(gettimeofday tv_interval);
     + use Digest::MD5 qw(md5_hex);
     ++use Git::LoadCPAN::Mail::Address;
     + 
     + binmode STDOUT, ':utf8';
     + 
      @@ gitweb/gitweb.perl: sub evaluate_uri {
       		'sub' => \&feature_extra_branch_refs,
       		'override' => 0,
     @@ gitweb/gitweb.perl: sub evaluate_uri {
      +	# Redact e-mail addresses.
      +
      +	# To enable system wide have in $GITWEB_CONFIG
     -+	# $feature{'email_privacy'}{'default'} = [1];
     -+	'email_privacy' => {
     -+		'sub' => sub { feature_bool('email_privacy', @_) },
     ++	# $feature{'email-privacy'}{'default'} = [1];
     ++	'email-privacy' => {
     ++		'sub' => sub { feature_bool('email-privacy', @_) },
      +		'override' => 1,
      +		'default' => [0]},
       );
     @@ gitweb/gitweb.perl: sub parse_date {
       	return %date;
       }
       
     -+sub hide_mailaddr_if_private {
     -+	my $line = shift;
     -+	return $line unless (gitweb_check_feature('email_privacy') &&
     -+						$line =~ m/^([^<]+) <([^>]*)>/);
     -+	return hide_mailaddr($line)
     ++sub is_mailaddr {
     ++	my @addrs = Mail::Address->parse(shift);
     ++	if (!@addrs || !$addrs[0]->host || !$addrs[0]->user) {
     ++		return 0;
     ++	}
     ++	return 1;
      +}
      +
     -+sub hide_mailaddr {
     -+	my $mailaddr = shift;
     -+	$mailaddr =~ s/<([^>]*)>/<private>/;
     -+	return $mailaddr;
     ++sub hide_mailaddrs_if_private {
     ++	my $line = shift;
     ++	return $line unless gitweb_check_feature('email-privacy');
     ++	while ($line =~ m/(<[^>]+>)/g) {
     ++		my $match = $1;
     ++		if (!is_mailaddr($match)) {
     ++			next;
     ++		}
     ++		my $offset = pos $line;
     ++		my $head = substr $line, 0, $offset - length($match);
     ++		my $redaction = "<redacted>";
     ++		my $tail = substr $line, $offset;
     ++		$line = $head . $redaction . $tail;
     ++		pos $line = length($head) + length($redaction);
     ++	}
     ++	return $line;
      +}
      +
       sub parse_tag {
       	my $tag_id = shift;
       	my %tag;
      @@ gitweb/gitweb.perl: sub parse_tag {
     + 		} elsif ($line =~ m/^tag (.+)$/) {
     + 			$tag{'name'} = $1;
     + 		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
     +-			$tag{'author'} = $1;
     ++			$tag{'author'} = hide_mailaddrs_if_private($1);
     + 			$tag{'author_epoch'} = $2;
     + 			$tag{'author_tz'} = $3;
       			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
     - 				$tag{'author_name'}  = $1;
     - 				$tag{'author_email'} = $2;
     -+				if (gitweb_check_feature('email_privacy')) {
     -+					$tag{'author_email'} = "private";
     -+					$tag{'author'} = hide_mailaddr($tag{'author'});
     -+				}
     - 			} else {
     - 				$tag{'author_name'} = $tag{'author'};
     - 			}
      @@ gitweb/gitweb.perl: sub parse_commit_text {
     + 		} elsif ((!defined $withparents) && ($line =~ m/^parent ($oid_regex)$/)) {
     + 			push @parents, $1;
     + 		} elsif ($line =~ m/^author (.*) ([0-9]+) (.*)$/) {
     +-			$co{'author'} = to_utf8($1);
     ++			$co{'author'} = hide_mailaddrs_if_private(to_utf8($1));
     + 			$co{'author_epoch'} = $2;
     + 			$co{'author_tz'} = $3;
       			if ($co{'author'} =~ m/^([^<]+) <([^>]*)>/) {
     - 				$co{'author_name'}  = $1;
     - 				$co{'author_email'} = $2;
     -+				if (gitweb_check_feature('email_privacy')) {
     -+					$co{'author_email'} = "private";
     -+					$co{'author'} = hide_mailaddr($co{'author'});
     -+				}
     - 			} else {
     +@@ gitweb/gitweb.perl: sub parse_commit_text {
       				$co{'author_name'} = $co{'author'};
       			}
     -@@ gitweb/gitweb.perl: sub parse_commit_text {
     + 		} elsif ($line =~ m/^committer (.*) ([0-9]+) (.*)$/) {
     +-			$co{'committer'} = to_utf8($1);
     ++			$co{'committer'} = hide_mailaddrs_if_private(to_utf8($1));
     + 			$co{'committer_epoch'} = $2;
     + 			$co{'committer_tz'} = $3;
       			if ($co{'committer'} =~ m/^([^<]+) <([^>]*)>/) {
     - 				$co{'committer_name'}  = $1;
     - 				$co{'committer_email'} = $2;
     -+				if (gitweb_check_feature('email_privacy')) {
     -+					$co{'committer_email'} = "private";
     -+					$co{'committer'} = hide_mailaddr($co{'committer'});
     -+				}
     - 			} else {
     - 				$co{'committer_name'} = $co{'committer'};
     - 			}
      @@ gitweb/gitweb.perl: sub parse_commit_text {
       	if (! defined $co{'title'} || $co{'title'} eq "") {
       		$co{'title'} = $co{'title_short'} = '(no commit message)';
     @@ gitweb/gitweb.perl: sub parse_commit_text {
      +	# remove added spaces, redact e-mail addresses if applicable.
       	foreach my $line (@commit_lines) {
       		$line =~ s/^    //;
     -+		$line = hide_mailaddr_if_private($line);
     ++		$line = hide_mailaddrs_if_private($line);
       	}
       	$co{'comment'} = \@commit_lines;
       
     -@@ gitweb/gitweb.perl: sub git_commitdiff {
     - 		close $fd
     - 			or print "Reading git-diff-tree failed\n";
     - 	} elsif ($format eq 'patch') {
     --		local $/ = undef;
     --		print <$fd>;
     -+		while (my $line = <$fd>) {
     -+			print hide_mailaddr_if_private($line);
     -+		}
     - 		close $fd
     - 			or print "Reading git-format-patch failed\n";
     +@@ gitweb/gitweb.perl: sub git_log_generic {
     + 			         -accesskey => "n", -title => "Alt-n"}, "next");
     + 	}
     + 	my $patch_max = gitweb_get_feature('patches');
     +-	if ($patch_max && !defined $file_name) {
     ++	if ($patch_max && !defined $file_name &&
     ++		!gitweb_check_feature('email-privacy')) {
     + 		if ($patch_max < 0 || @commitlist <= $patch_max) {
     + 			$paging_nav .= " &sdot; " .
     + 				$cgi->a({-href => href(action=>"patches", -replay=>1)},
     +@@ gitweb/gitweb.perl: sub git_commit {
     + 			} @$parents ) .
     + 			')';
       	}
     +-	if (gitweb_check_feature('patches') && @$parents <= 1) {
     ++	if (gitweb_check_feature('patches') && @$parents <= 1 &&
     ++		!gitweb_check_feature('email-privacy')) {
     + 		$formats_nav .= " | " .
     + 			$cgi->a({-href => href(action=>"patch", -replay=>1)},
     + 				"patch");
     +@@ gitweb/gitweb.perl: sub git_commitdiff {
     + 		$formats_nav =
     + 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
     + 			        "raw");
     +-		if ($patch_max && @{$co{'parents'}} <= 1) {
     ++		if ($patch_max && @{$co{'parents'}} <= 1 &&
     ++			!gitweb_check_feature('email-privacy')) {
     + 			$formats_nav .= " | " .
     + 				$cgi->a({-href => href(action=>"patch", -replay=>1)},
     + 					"patch");
     +
     + ## t/lib-gitweb.sh ##
     +@@ t/lib-gitweb.sh: gitweb_run () {
     + 	GITWEB_CONFIG=$(pwd)/gitweb_config.perl
     + 	export GITWEB_CONFIG
     + 
     ++	PERL5LIB="$GIT_BUILD_DIR/perl:$GIT_BUILD_DIR/perl/FromCPAN"
     ++	export PERL5LIB
     ++
     + 	# some of git commands write to STDERR on error, but this is not
     + 	# written to web server logs, so we are not interested in that:
     + 	# we are interested only in properly formatted errors/warnings
 Documentation/gitweb.conf.txt | 10 +++++++
 gitweb/gitweb.perl            | 54 ++++++++++++++++++++++++++++++-----
 t/lib-gitweb.sh               |  3 ++
 3 files changed, 60 insertions(+), 7 deletions(-)
Documentation/gitweb.conf.txt +10 -0
index 7963a79ba98b..47d85717ccaa 100644
@@ -751,6 +751,16 @@ default font sizes or lineheights are changed (e.g. via adding extra
 CSS stylesheet in `@stylesheets`), it may be appropriate to change
 these values.
 
+email-privacy::
+	Redact e-mail addresses from the generated HTML, etc. content.
+	This hides e-mail addresses found in the commit log from HTTP clients.
+	It is meant to hinder web crawlers that harvest and abuse addresses.
+	Such crawlers may not respect robots.txt.
+	Note that users and user tools also see the addresses redacted.
+	If Gitweb is not the final step in a workflow then subsequent steps
+	may misbehave because of the redacted information they receive.
+	Disabled by default.
+
 highlight::
 	Server-side syntax highlight support in "blob" view.  It requires
 	`$highlight_bin` program to be available (see the description of
gitweb/gitweb.perl +47 -7

116 lines not on this page · raw message

t/lib-gitweb.sh +3 -0
index 1f32ca66ea51..77fc1298d4c6 100644
@@ -67,6 +67,9 @@ gitweb_run () {
 	GITWEB_CONFIG=$(pwd)/gitweb_config.perl
 	export GITWEB_CONFIG
 
+	PERL5LIB="$GIT_BUILD_DIR/perl:$GIT_BUILD_DIR/perl/FromCPAN"
+	export PERL5LIB
+
 	# some of git commands write to STDERR on error, but this is not
 	# written to web server logs, so we are not interested in that:
 	# we are interested only in properly formatted errors/warnings

base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f

Signature · 1 line
gitgitgadget
#17 lore raw

"Georgios Kontaxis via GitGitGadget" <[email protected]> writes:

[note to other reviewers. input from those who are more familiar with gitweb and Perl is very much appreciated on this patch].

Quoted 6 lines

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.

"... are exposed to web crawlers, which spammers may use." would be sufficient as a problem description.

After giving a problem description, it is customery to describe the solution as if you are ordering the codebase to "be like so", so instead of this ...

Quoted 2 lines

This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

... we may say something like

Introduce an 'email-privacy' feature, which defaults to false, that redacts e-mail addresses that appear as author/committer info and in log messages from the generated HTML content.

Quoted 5 lines

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

And this is a good thing to add. Overall, nicely written.

Quoted 10 lines

---
gitweb: redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages. This is a feature for redacting e-mail addresses from the generated HTML, etc. content.

Quoted 4 lines

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

You do not need to repeat the above, which is in the log message above.

Quoted 8 lines

Changes since v1:

* Turned off the feature by default.
* Removed duplicate code.
* Added note about Gitweb consumers receiving redacted logs.

Changes since v2:

Quoted 8 lines

* The feature can be set on a per-project basis. ('override' => 1)

Changes since v3:

* Renamed feature to "email-privacy" and improved documentation.
* Removed UI elements for git-format-patch since it won't be redacted.
* Simplified calls to the address redaction logic.
* Mail::Address is now used to reduce false-positive redactions.

Having these under the --- line like this is very helpful.

gitweb/gitweb.perl line 24

Quoted 9 lines
gitweb/gitweb.perl +1 -0
index 0959a782eccb..6630c76d92fd 100755
@@ -21,6 +21,7 @@
 use File::Basename qw(basename);
 use Time::HiRes qw(gettimeofday tv_interval);
 use Digest::MD5 qw(md5_hex);
+use Git::LoadCPAN::Mail::Address;

I'll defer to others who are more familiar with gitweb and Perl ecosystem if this is warranted, but I have a feeling that importing and using Mail::Address->parse() only because we want to see if a given "<string>" is an address is a bit overkill and it might be sufficient to do something as crude as m/^<[^@>]+@[a-z0-9-.]+>$/i

Quoted 27 lines
gitweb/gitweb.perl +20 -0
+	# Redact e-mail addresses.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'email-privacy'}{'default'} = [1];
+	'email-privacy' => {
+		'sub' => sub { feature_bool('email-privacy', @_) },
+		'override' => 1,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -3449,6 +3459,32 @@ sub parse_date {
 	return %date;
 }
 
+sub is_mailaddr {
+	my @addrs = Mail::Address->parse(shift);
+	if (!@addrs || !$addrs[0]->host || !$addrs[0]->user) {
+		return 0;
+	}
+	return 1;
+}
+
+sub hide_mailaddrs_if_private {
+	my $line = shift;
+	return $line unless gitweb_check_feature('email-privacy');
+	while ($line =~ m/(<[^>]+>)/g) {

gitweb/gitweb.perl sub parse_date { · line 3483

Quoted 10 lines
gitweb/gitweb.perl +10 -0
+		my $match = $1;
+		if (!is_mailaddr($match)) {
+			next;
+		}
+		my $offset = pos $line;
+		my $head = substr $line, 0, $offset - length($match);
+		my $redaction = "<redacted>";
+		my $tail = substr $line, $offset;
+		$line = $head . $redaction . $tail;
+		pos $line = length($head) + length($redaction);

Hmmmm, Perl suggestions from others? It looks quite strange to see that s/// operator is not used and replacement is done manually with byte position in a Perl script.

gitweb/gitweb.perl sub parse_tag { · line 3507

Quoted 12 lines
gitweb/gitweb.perl +1 -1
 sub parse_tag {
 	my $tag_id = shift;
 	my %tag;
@@ -3465,7 +3501,7 @@ sub parse_tag {
 		} elsif ($line =~ m/^tag (.+)$/) {
 			$tag{'name'} = $1;
 		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
-			$tag{'author'} = $1;
+			$tag{'author'} = hide_mailaddrs_if_private($1);
 			$tag{'author_epoch'} = $2;
 			$tag{'author_tz'} = $3;
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {

This (and the others that follow the same pattern) looks sensible.

Quoted 21 lines
gitweb/gitweb.perl +4 -2
@@ -7489,7 +7526,8 @@ sub git_log_generic {
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
 	my $patch_max = gitweb_get_feature('patches');
-	if ($patch_max && !defined $file_name) {
+	if ($patch_max && !defined $file_name &&
+		!gitweb_check_feature('email-privacy')) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {
 			$paging_nav .= " &sdot; " .
 				$cgi->a({-href => href(action=>"patches", -replay=>1)},
@@ -7550,7 +7588,8 @@ sub git_commit {
 			} @$parents ) .
 			')';
 	}
-	if (gitweb_check_feature('patches') && @$parents <= 1) {
+	if (gitweb_check_feature('patches') && @$parents <= 1 &&
+		!gitweb_check_feature('email-privacy')) {
 		$formats_nav .= " | " .
 			$cgi->a({-href => href(action=>"patch", -replay=>1)},
 				"patch");

gitweb/gitweb.perl sub git_commitdiff { · line 7909

Quoted 9 lines
gitweb/gitweb.perl +2 -1
@@ -7863,7 +7902,8 @@ sub git_commitdiff {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
 			        "raw");
-		if ($patch_max && @{$co{'parents'}} <= 1) {
+		if ($patch_max && @{$co{'parents'}} <= 1 &&
+			!gitweb_check_feature('email-privacy')) {
 			$formats_nav .= " | " .
 				$cgi->a({-href => href(action=>"patch", -replay=>1)},
 					"patch");

I wouldn't have expected to see the above three hunks in the "patch" codepath. Rather, I was hoping that you'd do something like this at startup when you find out that the privacy feature is enabled:

	$feature{'patches'}{'default'} = [0]
		if gitweb_get_feature('email-privacy');

so that nothing related to the 'patches' has to be modified. That way, even if there were fourth place that can leak an e-mail address in the 'patch' codepath that above three hunks in this patch missed, crawlers won't be able to get at it, no?

t/lib-gitweb.sh gitweb_run () { · line 72

Quoted 11 lines
t/lib-gitweb.sh +3 -0
index 1f32ca66ea51..77fc1298d4c6 100644
@@ -67,6 +67,9 @@ gitweb_run () {
 	GITWEB_CONFIG=$(pwd)/gitweb_config.perl
 	export GITWEB_CONFIG
 
+	PERL5LIB="$GIT_BUILD_DIR/perl:$GIT_BUILD_DIR/perl/FromCPAN"
+	export PERL5LIB
+

Why is this change suddenly become necessary? This addition is only about tests---do we need to do something similar in the runtime environment when the updated gitweb that requires Mail::Address gets deployed, or is that covered already somewhere else?

Thanks.

#18 lore raw
Quoted 8 lines

"Georgios Kontaxis via GitGitGadget" <[email protected]> writes:

[note to other reviewers. input from those who are more familiar with gitweb and Perl is very much appreciated on this patch].

Quoted 2 lines

From: Georgios Kontaxis <[email protected]>

Quoted 8 lines
Quoted 4 lines

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.

"... are exposed to web crawlers, which spammers may use." would be sufficient as a problem description.

Using robots.txt was suggested earlier in this review so I want to make sure people don't rely on it instead.

Quoted 4 lines

After giving a problem description, it is customery to describe the solution as if you are ordering the codebase to "be like so", so instead of this ...

Quoted 9 lines
Quoted 2 lines

This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

... we may say something like

Introduce an 'email-privacy' feature, which defaults to false, that redacts e-mail addresses that appear as author/committer info and in log messages from the generated HTML content.

Will rephrase.

Quoted 50 lines
Quoted 5 lines

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

And this is a good thing to add. Overall, nicely written.

Quoted 13 lines

---
gitweb: redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible
over
HTTP. As a result, e-mail addresses found in commits are exposed to
web
crawlers and they may not respect robots.txt. This may result in unsolicited messages. This is a feature for redacting e-mail
addresses
from the generated HTML, etc. content.

Quoted 6 lines

This feature does not prevent someone from downloading the
unredacted
commit log, e.g., by cloning the repository, and extracting
information
from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

You do not need to repeat the above, which is in the log message above.

Quoted 10 lines

Changes since v1:

* Turned off the feature by default.
* Removed duplicate code.
* Added note about Gitweb consumers receiving redacted logs.

Changes since v2:

* The feature can be set on a per-project basis. ('override' => 1)

Quoted 7 lines

Changes since v3:

* Renamed feature to "email-privacy" and improved documentation.
* Removed UI elements for git-format-patch since it won't be
redacted.
* Simplified calls to the address redaction logic.
* Mail::Address is now used to reduce false-positive redactions.

Having these under the --- line like this is very helpful.

Quoted 16 lines

gitweb/gitweb.perl line 24

Quoted 9 lines
gitweb/gitweb.perl +1 -0
index 0959a782eccb..6630c76d92fd 100755
@@ -21,6 +21,7 @@
 use File::Basename qw(basename);
 use Time::HiRes qw(gettimeofday tv_interval);
 use Digest::MD5 qw(md5_hex);
+use Git::LoadCPAN::Mail::Address;

I'll defer to others who are more familiar with gitweb and Perl ecosystem if this is warranted, but I have a feeling that importing and using Mail::Address->parse() only because we want to see if a given "<string>" is an address is a bit overkill and it might be sufficient to do something as crude as m/^<[^@>]+@[a-z0-9-.]+>$/i

I think Mail::Address->parse() pretty much does the same thing. It was suggested earlier in the review to use that function, similar to what git-send-mail does.
I'm assuming because the maintainers of the Mail package are already invested in the (accurate) parsing of e-mail addresses.

Quoted 33 lines
Quoted 33 lines
gitweb/gitweb.perl +26 -0
+	# Redact e-mail addresses.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'email-privacy'}{'default'} = [1];
+	'email-privacy' => {
+		'sub' => sub { feature_bool('email-privacy', @_) },
+		'override' => 1,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -3449,6 +3459,32 @@ sub parse_date {
 	return %date;
 }
 
+sub is_mailaddr {
+	my @addrs = Mail::Address->parse(shift);
+	if (!@addrs || !$addrs[0]->host || !$addrs[0]->user) {
+		return 0;
+	}
+	return 1;
+}
+
+sub hide_mailaddrs_if_private {
+	my $line = shift;
+	return $line unless gitweb_check_feature('email-privacy');
+	while ($line =~ m/(<[^>]+>)/g) {
+		my $match = $1;
+		if (!is_mailaddr($match)) {
+			next;
+		}
+		my $offset = pos $line;
+		my $head = substr $line, 0, $offset - length($match);
Quoted 9 lines

gitweb/gitweb.perl sub parse_date { · line 3483

Quoted 4 lines
gitweb/gitweb.perl +4 -0
+		my $redaction = "<redacted>";
+		my $tail = substr $line, $offset;
+		$line = $head . $redaction . $tail;
+		pos $line = length($head) + length($redaction);

Hmmmm, Perl suggestions from others? It looks quite strange to see that s/// operator is not used and replacement is done manually with byte position in a Perl script.

If there's a more elegant way to do the above we can certain do that instead.

Quoted 51 lines

gitweb/gitweb.perl sub parse_tag { · line 3507

Quoted 12 lines
gitweb/gitweb.perl +1 -1
 sub parse_tag {
 	my $tag_id = shift;
 	my %tag;
@@ -3465,7 +3501,7 @@ sub parse_tag {
 		} elsif ($line =~ m/^tag (.+)$/) {
 			$tag{'name'} = $1;
 		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
-			$tag{'author'} = $1;
+			$tag{'author'} = hide_mailaddrs_if_private($1);
 			$tag{'author_epoch'} = $2;
 			$tag{'author_tz'} = $3;
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {

This (and the others that follow the same pattern) looks sensible.

Quoted 21 lines
gitweb/gitweb.perl +4 -2
@@ -7489,7 +7526,8 @@ sub git_log_generic {
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
 	my $patch_max = gitweb_get_feature('patches');
-	if ($patch_max && !defined $file_name) {
+	if ($patch_max && !defined $file_name &&
+		!gitweb_check_feature('email-privacy')) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {
 			$paging_nav .= " &sdot; " .
 				$cgi->a({-href => href(action=>"patches", -replay=>1)},
@@ -7550,7 +7588,8 @@ sub git_commit {
 			} @$parents ) .
 			')';
 	}
-	if (gitweb_check_feature('patches') && @$parents <= 1) {
+	if (gitweb_check_feature('patches') && @$parents <= 1 &&
+		!gitweb_check_feature('email-privacy')) {
 		$formats_nav .= " | " .
 			$cgi->a({-href => href(action=>"patch", -replay=>1)},
 				"patch");

gitweb/gitweb.perl sub git_commitdiff { · line 7909

Quoted 9 lines
gitweb/gitweb.perl +2 -1
@@ -7863,7 +7902,8 @@ sub git_commitdiff {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
 			        "raw");
-		if ($patch_max && @{$co{'parents'}} <= 1) {
+		if ($patch_max && @{$co{'parents'}} <= 1 &&
+			!gitweb_check_feature('email-privacy')) {
 			$formats_nav .= " | " .
 				$cgi->a({-href => href(action=>"patch", -replay=>1)},
 					"patch");

I wouldn't have expected to see the above three hunks in the "patch" codepath. Rather, I was hoping that you'd do something like this at startup when you find out that the privacy feature is enabled:

Quoted 8 lines
	$feature{'patches'}{'default'} = [0]
		if gitweb_get_feature('email-privacy');

so that nothing related to the 'patches' has to be modified. That way, even if there were fourth place that can leak an e-mail address in the 'patch' codepath that above three hunks in this patch missed, crawlers won't be able to get at it, no?

Or there's multiple feature flags / triggers that lead to the "patch" codepath.
Either way is fine with me. (disabling the "patches" feature or preventing the
generation of UI elements)
Like you said, someone who regularly reviews Gitweb code may have an opinion on how do this.

Quoted 8 lines
Quoted 8 lines
t/lib-gitweb.sh +0 -0
index 1f32ca66ea51..77fc1298d4c6 100644
@@ -67,6 +67,9 @@ gitweb_run () {
 	GITWEB_CONFIG=$(pwd)/gitweb_config.perl
 	export GITWEB_CONFIG
 
Quoted 9 lines

t/lib-gitweb.sh gitweb_run () { · line 72

Quoted 3 lines
t/lib-gitweb.sh +3 -0
+	PERL5LIB="$GIT_BUILD_DIR/perl:$GIT_BUILD_DIR/perl/FromCPAN"
+	export PERL5LIB
+

Why is this change suddenly become necessary? This addition is only about tests---do we need to do something similar in the runtime environment when the updated gitweb that requires Mail::Address gets deployed, or is that covered already somewhere else?

This is necessary because osx-clang, osx-gcc tests were failing. If there's a better way to address this I'm open to suggestions.

I haven't been able to experience deployment on osx but on linux i didn't have to change anything.

Quoted 2 lines

Thanks.

#19 lore raw
Quoted 22 lines · Georgios Kontaxis
Quoted 6 lines

I'll defer to others who are more familiar with gitweb and Perl ecosystem if this is warranted, but I have a feeling that importing and using Mail::Address->parse() only because we want to see if a given "<string>" is an address is a bit overkill and it might be sufficient to do something as crude as m/^<[^@>]+@[a-z0-9-.]+>$/i
...

Quoted 16 lines

gitweb/gitweb.perl sub parse_date { · line 3483

Quoted 11 lines
gitweb/gitweb.perl +11 -0
+	while ($line =~ m/(<[^>]+>)/g) {
+		my $match = $1;
+		if (!is_mailaddr($match)) {
+			next;
+		}
+		my $offset = pos $line;
+		my $head = substr $line, 0, $offset - length($match);
+		my $redaction = "<redacted>";
+		my $tail = substr $line, $offset;
+		$line = $head . $redaction . $tail;
+		pos $line = length($head) + length($redaction);

Hmmmm, Perl suggestions from others? It looks quite strange to see that s/// operator is not used and replacement is done manually with byte position in a Perl script.

Quoted 1 line

If there's a more elegant way to do the above we can certain do that instead.

For example, if we do not insist on using overkill Mail::Address->parse(), we could do something silly like this:

	$line =~ s/<[^@>]+@[a-z0-9-.]+>/<redacted@address>/ig;

no?

#20 lore raw
Quoted 27 lines
Quoted 22 lines · Georgios Kontaxis
Quoted 6 lines

I'll defer to others who are more familiar with gitweb and Perl ecosystem if this is warranted, but I have a feeling that importing and using Mail::Address->parse() only because we want to see if a given "<string>" is an address is a bit overkill and it might be sufficient to do something as crude as m/^<[^@>]+@[a-z0-9-.]+>$/i
...

Quoted 16 lines

gitweb/gitweb.perl sub parse_date { · line 3483

Quoted 11 lines
gitweb/gitweb.perl +11 -0
+	while ($line =~ m/(<[^>]+>)/g) {
+		my $match = $1;
+		if (!is_mailaddr($match)) {
+			next;
+		}
+		my $offset = pos $line;
+		my $head = substr $line, 0, $offset - length($match);
+		my $redaction = "<redacted>";
+		my $tail = substr $line, $offset;
+		$line = $head . $redaction . $tail;
+		pos $line = length($head) + length($redaction);

Hmmmm, Perl suggestions from others? It looks quite strange to see that s/// operator is not used and replacement is done manually with byte position in a Perl script.

Quoted 2 lines

If there's a more elegant way to do the above we can certain do that instead.

Quoted 7 lines

For example, if we do not insist on using overkill Mail::Address->parse(), we could do something silly like this:

	$line =~ s/<[^@>]+@[a-z0-9-.]+>/<redacted@address>/ig;

no?

It's not clear if you think it's overkill because we have to depend on an external module or because we don't need accurate parsing.

The above expression seems to get the job done but it isn't the correct way to parse an e-mail address.
But if it's good enough for the reviewers I don't feel strongly about it.

If we prefer accurate parsing but don't like depending on Mail::Address, it's easy to write complete expressions ourselves. (In a separate, internal, Perl module)

Thoughts?

#21 lore raw
Quoted 2 lines · Georgios Kontaxis

It's not clear if you think it's overkill because we have to depend on an external module or because we don't need accurate parsing.

It is neither; what we need to parse is not exactly 'e-mail addresses' as known to Mail::Address.

The thing is, unlike send-email that needs to interact with the real-world MTAs and e-mail addresses, the codepaths we are talking about are mostly about author/committer ident, where the definition is quite narrower than the Mail::Address's "has to cover all the possible ways to spell e-mail addresses under the sun" requirement.

Having said all that ...

Quoted 3 lines

If we prefer accurate parsing but don't like depending on Mail::Address, it's easy to write complete expressions ourselves. (In a separate, internal, Perl module)

... I do not have strong opinions either way. I won't have the final say on the way things are done in Perl and what is done to Gitweb.

It was just that what I saw earlier with the offsets and manual parsing instead of s///g operator did not smell not like a Perl program to me, and the message you are responding to was my reaction to it.

Thanks.

#22 lore raw
In reply to Junio C Hamano
Quoted 188 lines

"Georgios Kontaxis via GitGitGadget" <[email protected]> writes:

[note to other reviewers. input from those who are more familiar with gitweb and Perl is very much appreciated on this patch].

Quoted 6 lines

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This may result in unsolicited messages.

"... are exposed to web crawlers, which spammers may use." would be sufficient as a problem description.

After giving a problem description, it is customery to describe the solution as if you are ordering the codebase to "be like so", so instead of this ...

Quoted 2 lines

This is a feature for redacting e-mail addresses
from the generated HTML, etc. content.

... we may say something like

Introduce an 'email-privacy' feature, which defaults to false, that redacts e-mail addresses that appear as author/committer info and in log messages from the generated HTML content.

Quoted 5 lines

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it.
It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

And this is a good thing to add. Overall, nicely written.

Quoted 13 lines

---
gitweb: redacted e-mail addresses feature.

Gitweb extracts content from the Git log and makes it accessible
over
HTTP. As a result, e-mail addresses found in commits are exposed to
web
crawlers and they may not respect robots.txt. This may result in unsolicited messages. This is a feature for redacting e-mail
addresses
from the generated HTML, etc. content.

Quoted 6 lines

This feature does not prevent someone from downloading the
unredacted
commit log, e.g., by cloning the repository, and extracting
information
from it. It aims to hinder the low-effort bulk collection of e-mail addresses by web crawlers.

You do not need to repeat the above, which is in the log message above.

Quoted 10 lines

Changes since v1:

* Turned off the feature by default.
* Removed duplicate code.
* Added note about Gitweb consumers receiving redacted logs.

Changes since v2:

* The feature can be set on a per-project basis. ('override' => 1)

Quoted 7 lines

Changes since v3:

* Renamed feature to "email-privacy" and improved documentation.
* Removed UI elements for git-format-patch since it won't be
redacted.
* Simplified calls to the address redaction logic.
* Mail::Address is now used to reduce false-positive redactions.

Having these under the --- line like this is very helpful.

gitweb/gitweb.perl line 24

Quoted 9 lines
gitweb/gitweb.perl +1 -0
index 0959a782eccb..6630c76d92fd 100755
@@ -21,6 +21,7 @@
 use File::Basename qw(basename);
 use Time::HiRes qw(gettimeofday tv_interval);
 use Digest::MD5 qw(md5_hex);
+use Git::LoadCPAN::Mail::Address;

I'll defer to others who are more familiar with gitweb and Perl ecosystem if this is warranted, but I have a feeling that importing and using Mail::Address->parse() only because we want to see if a given "<string>" is an address is a bit overkill and it might be sufficient to do something as crude as m/^<[^@>]+@[a-z0-9-.]+>$/i

Quoted 27 lines
gitweb/gitweb.perl +20 -0
+	# Redact e-mail addresses.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'email-privacy'}{'default'} = [1];
+	'email-privacy' => {
+		'sub' => sub { feature_bool('email-privacy', @_) },
+		'override' => 1,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -3449,6 +3459,32 @@ sub parse_date {
 	return %date;
 }
 
+sub is_mailaddr {
+	my @addrs = Mail::Address->parse(shift);
+	if (!@addrs || !$addrs[0]->host || !$addrs[0]->user) {
+		return 0;
+	}
+	return 1;
+}
+
+sub hide_mailaddrs_if_private {
+	my $line = shift;
+	return $line unless gitweb_check_feature('email-privacy');
+	while ($line =~ m/(<[^>]+>)/g) {

gitweb/gitweb.perl sub parse_date { · line 3483

Quoted 10 lines
gitweb/gitweb.perl +10 -0
+		my $match = $1;
+		if (!is_mailaddr($match)) {
+			next;
+		}
+		my $offset = pos $line;
+		my $head = substr $line, 0, $offset - length($match);
+		my $redaction = "<redacted>";
+		my $tail = substr $line, $offset;
+		$line = $head . $redaction . $tail;
+		pos $line = length($head) + length($redaction);

Hmmmm, Perl suggestions from others? It looks quite strange to see that s/// operator is not used and replacement is done manually with byte position in a Perl script.

gitweb/gitweb.perl sub parse_tag { · line 3507

Quoted 12 lines
gitweb/gitweb.perl +1 -1
 sub parse_tag {
 	my $tag_id = shift;
 	my %tag;
@@ -3465,7 +3501,7 @@ sub parse_tag {
 		} elsif ($line =~ m/^tag (.+)$/) {
 			$tag{'name'} = $1;
 		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
-			$tag{'author'} = $1;
+			$tag{'author'} = hide_mailaddrs_if_private($1);
 			$tag{'author_epoch'} = $2;
 			$tag{'author_tz'} = $3;
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {

This (and the others that follow the same pattern) looks sensible.

Quoted 21 lines
gitweb/gitweb.perl +4 -2
@@ -7489,7 +7526,8 @@ sub git_log_generic {
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
 	my $patch_max = gitweb_get_feature('patches');
-	if ($patch_max && !defined $file_name) {
+	if ($patch_max && !defined $file_name &&
+		!gitweb_check_feature('email-privacy')) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {
 			$paging_nav .= " &sdot; " .
 				$cgi->a({-href => href(action=>"patches", -replay=>1)},
@@ -7550,7 +7588,8 @@ sub git_commit {
 			} @$parents ) .
 			')';
 	}
-	if (gitweb_check_feature('patches') && @$parents <= 1) {
+	if (gitweb_check_feature('patches') && @$parents <= 1 &&
+		!gitweb_check_feature('email-privacy')) {
 		$formats_nav .= " | " .
 			$cgi->a({-href => href(action=>"patch", -replay=>1)},
 				"patch");

gitweb/gitweb.perl sub git_commitdiff { · line 7909

Quoted 9 lines
gitweb/gitweb.perl +2 -1
@@ -7863,7 +7902,8 @@ sub git_commitdiff {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
 			        "raw");
-		if ($patch_max && @{$co{'parents'}} <= 1) {
+		if ($patch_max && @{$co{'parents'}} <= 1 &&
+			!gitweb_check_feature('email-privacy')) {
 			$formats_nav .= " | " .
 				$cgi->a({-href => href(action=>"patch", -replay=>1)},
 					"patch");

I wouldn't have expected to see the above three hunks in the "patch" codepath. Rather, I was hoping that you'd do something like this at startup when you find out that the privacy feature is enabled:

Quoted 8 lines
	$feature{'patches'}{'default'} = [0]
		if gitweb_get_feature('email-privacy');

so that nothing related to the 'patches' has to be modified. That way, even if there were fourth place that can leak an e-mail address in the 'patch' codepath that above three hunks in this patch missed, crawlers won't be able to get at it, no?

I forgot to mention one of the reasons for doing it this way. If someone knows this URL exists and is relying on it (e.g., in a script) they can still call it with "email-privacy" enabled.
The only thing that's changing is that the UI element (link) that a web crawler would follow just because it's there is now gone.

But, as mentioned before, I don't feel strongly about either approach. Let me know.

Quoted 19 lines

t/lib-gitweb.sh gitweb_run () { · line 72

Quoted 11 lines
t/lib-gitweb.sh +3 -0
index 1f32ca66ea51..77fc1298d4c6 100644
@@ -67,6 +67,9 @@ gitweb_run () {
 	GITWEB_CONFIG=$(pwd)/gitweb_config.perl
 	export GITWEB_CONFIG
 
+	PERL5LIB="$GIT_BUILD_DIR/perl:$GIT_BUILD_DIR/perl/FromCPAN"
+	export PERL5LIB
+

Why is this change suddenly become necessary? This addition is only about tests---do we need to do something similar in the runtime environment when the updated gitweb that requires Mail::Address gets deployed, or is that covered already somewhere else?

Thanks.

#23 lore raw

PATCH v5 1/1 gitweb: redacted e-mail addresses feature.

In reply to Georgios Kontaxis via GitGitGadget

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This can result in unsolicited messages.

Introduce an 'email-privacy' feature which redacts e-mail addresses from the generated HTML content. Specifically, obscure addresses retrieved from the the author/committer and comment sections of the Git log. The feature is off by default.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it. It aims to hinder the low- effort, bulk collection of e-mail addresses by web crawlers.

---
gitweb: redacted e-mail addresses feature.

Changes since v1:

* Turned off the feature by default.
* Removed duplicate code.
* Added note about Gitweb consumers receiving redacted logs.

Changes since v2:

* The feature can be set on a per-project basis. ('override' => 1)

Changes since v3:

* Renamed feature to "email-privacy" and improved documentation.
* Removed UI elements for git-format-patch since it won't be redacted.
* Simplified calls to the address redaction logic.
* Mail::Address is now used to reduce false-positive redactions.

Changes since v4:

* Rephrased the commit comment.
* hide_mailaddrs_if_private is slighly more compact.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v4:

1: 03a3f41c37ef ! 1: 1427231f9db5 gitweb: redacted e-mail addresses feature.
@@ Commit message
Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt.
- This may result in unsolicited messages.
- This is a feature for redacting e-mail addresses
- from the generated HTML, etc. content.
+ This can result in unsolicited messages.
+
+ Introduce an 'email-privacy' feature which redacts e-mail addresses
+ from the generated HTML content. Specifically, obscure addresses
+ retrieved from the the author/committer and comment sections of the
+ Git log. The feature is off by default.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and
- extracting information from it.
- It aims to hinder the low-effort bulk collection of e-mail
- addresses by web crawlers.
+ extracting information from it. It aims to hinder the low-
+ effort, bulk collection of e-mail addresses by web crawlers.

@@ Documentation/gitweb.conf.txt: default font sizes or lineheights are changed (e.

+email-privacy::
+ Redact e-mail addresses from the generated HTML, etc. content.
-+ This hides e-mail addresses found in the commit log from HTTP clients.
++ This obscures e-mail addresses retrieved from the author/committer
++ and comment sections of the Git log.
+ It is meant to hinder web crawlers that harvest and abuse addresses.
+ Such crawlers may not respect robots.txt.
-+ Note that users and user tools also see the addresses redacted.
++ Note that users and user tools also see the addresses as redacted.
+ If Gitweb is not the final step in a workflow then subsequent steps
+ may misbehave because of the redacted information they receive.
+ Disabled by default.
@@ gitweb/gitweb.perl: sub parse_date {
+ if (!is_mailaddr($match)) {
+ next;
+ }
-+ my $offset = pos $line;
-+ my $head = substr $line, 0, $offset - length($match);
++ my $match_offset = pos($line) - length($match);
++ pos $line = $match_offset;
++
+ my $redaction = "<redacted>";
-+ my $tail = substr $line, $offset;
-+ $line = $head . $redaction . $tail;
-+ pos $line = length($head) + length($redaction);
++ $line =~ s/\G(<[^>]+>)/$redaction/;
++
++ pos $line = $match_offset + length($redaction);
+ }
+ return $line;
+}

 Documentation/gitweb.conf.txt | 11 +++++++
 gitweb/gitweb.perl            | 55 ++++++++++++++++++++++++++++++-----
 t/lib-gitweb.sh               |  3 ++
 3 files changed, 62 insertions(+), 7 deletions(-)
Documentation/gitweb.conf.txt +11 -0
index 7963a79ba98b..34b1d6e22435 100644
@@ -751,6 +751,17 @@ default font sizes or lineheights are changed (e.g. via adding extra
 CSS stylesheet in `@stylesheets`), it may be appropriate to change
 these values.
 
+email-privacy::
+	Redact e-mail addresses from the generated HTML, etc. content.
+	This obscures e-mail addresses retrieved from the author/committer
+	and comment sections of the Git log.
+	It is meant to hinder web crawlers that harvest and abuse addresses.
+	Such crawlers may not respect robots.txt.
+	Note that users and user tools also see the addresses as redacted.
+	If Gitweb is not the final step in a workflow then subsequent steps
+	may misbehave because of the redacted information they receive.
+	Disabled by default.
+
 highlight::
 	Server-side syntax highlight support in "blob" view.  It requires
 	`$highlight_bin` program to be available (see the description of
gitweb/gitweb.perl +48 -7

117 lines not on this page · raw message

t/lib-gitweb.sh +3 -0
index 1f32ca66ea51..77fc1298d4c6 100644
@@ -67,6 +67,9 @@ gitweb_run () {
 	GITWEB_CONFIG=$(pwd)/gitweb_config.perl
 	export GITWEB_CONFIG
 
+	PERL5LIB="$GIT_BUILD_DIR/perl:$GIT_BUILD_DIR/perl/FromCPAN"
+	export PERL5LIB
+
 	# some of git commands write to STDERR on error, but this is not
 	# written to web server logs, so we are not interested in that:
 	# we are interested only in properly formatted errors/warnings

base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f

Signature · 1 line
gitgitgadget
#24 lore raw

PATCH v6 1/1 gitweb: redacted e-mail addresses feature.

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This can result in unsolicited messages.

Introduce an 'email-privacy' feature which redacts e-mail addresses from the generated HTML content. Specifically, obscure addresses retrieved from the the author/committer and comment sections of the Git log. The feature is off by default.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it. It aims to hinder the low- effort, bulk collection of e-mail addresses by web crawlers.

---
gitweb: redacted e-mail addresses feature.

Changes since v1:

* Turned off the feature by default.
* Removed duplicate code.
* Added note about Gitweb consumers receiving redacted logs.

Changes since v2:

* The feature can be set on a per-project basis. ('override' => 1)

Changes since v3:

* Renamed feature to "email-privacy" and improved documentation.
* Removed UI elements for git-format-patch since it won't be redacted.
* Simplified calls to the address redaction logic.
* Mail::Address is now used to reduce false-positive redactions.

Changes since v4:

* Rephrased the commit comment.
* hide_mailaddrs_if_private is slighly more compact.

Changes since v5:

* A simple <local@domain> filter is used instead of Mail::Address to identify addresses.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-910%2Fkontaxis%2Fkontaxis%2Femail_privacy-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-910/kontaxis/kontaxis/email_privacy-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/910

Range-diff vs v5:

1: 1427231f9db5 ! 1: 245cfed8ad58 gitweb: redacted e-mail addresses feature.

     @@ Documentation/gitweb.conf.txt: default font sizes or lineheights are changed (e.
       	`$highlight_bin` program to be available (see the description of
      
       ## gitweb/gitweb.perl ##
     -@@
     - use File::Basename qw(basename);
     - use Time::HiRes qw(gettimeofday tv_interval);
     - use Digest::MD5 qw(md5_hex);
     -+use Git::LoadCPAN::Mail::Address;
     - 
     - binmode STDOUT, ':utf8';
     - 
      @@ gitweb/gitweb.perl: sub evaluate_uri {
       		'sub' => \&feature_extra_branch_refs,
       		'override' => 0,
     @@ gitweb/gitweb.perl: sub parse_date {
       	return %date;
       }
       
     -+sub is_mailaddr {
     -+	my @addrs = Mail::Address->parse(shift);
     -+	if (!@addrs || !$addrs[0]->host || !$addrs[0]->user) {
     -+		return 0;
     -+	}
     -+	return 1;
     -+}
     -+
      +sub hide_mailaddrs_if_private {
      +	my $line = shift;
      +	return $line unless gitweb_check_feature('email-privacy');
     -+	while ($line =~ m/(<[^>]+>)/g) {
     -+		my $match = $1;
     -+		if (!is_mailaddr($match)) {
     -+			next;
     -+		}
     -+		my $match_offset = pos($line) - length($match);
     -+		pos $line = $match_offset;
     -+
     -+		my $redaction = "<redacted>";
     -+		$line =~ s/\G(<[^>]+>)/$redaction/;
     -+
     -+		pos $line = $match_offset + length($redaction);
     -+	}
     ++	$line =~ s/<[^@>]+@[^>]+>/<redacted>/ig;
      +	return $line;
      +}
      +
     @@ gitweb/gitweb.perl: sub git_commitdiff {
       			$formats_nav .= " | " .
       				$cgi->a({-href => href(action=>"patch", -replay=>1)},
       					"patch");
     -
     - ## t/lib-gitweb.sh ##
     -@@ t/lib-gitweb.sh: gitweb_run () {
     - 	GITWEB_CONFIG=$(pwd)/gitweb_config.perl
     - 	export GITWEB_CONFIG
     - 
     -+	PERL5LIB="$GIT_BUILD_DIR/perl:$GIT_BUILD_DIR/perl/FromCPAN"
     -+	export PERL5LIB
     -+
     - 	# some of git commands write to STDERR on error, but this is not
     - 	# written to web server logs, so we are not interested in that:
     - 	# we are interested only in properly formatted errors/warnings
 Documentation/gitweb.conf.txt | 11 +++++++++++
 gitweb/gitweb.perl            | 34 +++++++++++++++++++++++++++-------
 2 files changed, 38 insertions(+), 7 deletions(-)
Documentation/gitweb.conf.txt +11 -0
index 7963a79ba98b..34b1d6e22435 100644
@@ -751,6 +751,17 @@ default font sizes or lineheights are changed (e.g. via adding extra
 CSS stylesheet in `@stylesheets`), it may be appropriate to change
 these values.
 
+email-privacy::
+	Redact e-mail addresses from the generated HTML, etc. content.
+	This obscures e-mail addresses retrieved from the author/committer
+	and comment sections of the Git log.
+	It is meant to hinder web crawlers that harvest and abuse addresses.
+	Such crawlers may not respect robots.txt.
+	Note that users and user tools also see the addresses as redacted.
+	If Gitweb is not the final step in a workflow then subsequent steps
+	may misbehave because of the redacted information they receive.
+	Disabled by default.
+
 highlight::
 	Server-side syntax highlight support in "blob" view.  It requires
 	`$highlight_bin` program to be available (see the description of
gitweb/gitweb.perl +27 -7
index 0959a782eccb..01c6faf88006 100755
@@ -569,6 +569,15 @@ sub evaluate_uri {
 		'sub' => \&feature_extra_branch_refs,
 		'override' => 0,
 		'default' => []},
+
+	# Redact e-mail addresses.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'email-privacy'}{'default'} = [1];
+	'email-privacy' => {
+		'sub' => sub { feature_bool('email-privacy', @_) },
+		'override' => 1,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {
@@ -3449,6 +3458,13 @@ sub parse_date {
 	return %date;
 }
 
+sub hide_mailaddrs_if_private {
+	my $line = shift;
+	return $line unless gitweb_check_feature('email-privacy');
+	$line =~ s/<[^@>]+@[^>]+>/<redacted>/ig;
+	return $line;
+}
+
 sub parse_tag {
 	my $tag_id = shift;
 	my %tag;
@@ -3465,7 +3481,7 @@ sub parse_tag {
 		} elsif ($line =~ m/^tag (.+)$/) {
 			$tag{'name'} = $1;
 		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
-			$tag{'author'} = $1;
+			$tag{'author'} = hide_mailaddrs_if_private($1);
 			$tag{'author_epoch'} = $2;
 			$tag{'author_tz'} = $3;
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
@@ -3513,7 +3529,7 @@ sub parse_commit_text {
 		} elsif ((!defined $withparents) && ($line =~ m/^parent ($oid_regex)$/)) {
 			push @parents, $1;
 		} elsif ($line =~ m/^author (.*) ([0-9]+) (.*)$/) {
-			$co{'author'} = to_utf8($1);
+			$co{'author'} = hide_mailaddrs_if_private(to_utf8($1));
 			$co{'author_epoch'} = $2;
 			$co{'author_tz'} = $3;
 			if ($co{'author'} =~ m/^([^<]+) <([^>]*)>/) {
@@ -3523,7 +3539,7 @@ sub parse_commit_text {
 				$co{'author_name'} = $co{'author'};
 			}
 		} elsif ($line =~ m/^committer (.*) ([0-9]+) (.*)$/) {
-			$co{'committer'} = to_utf8($1);
+			$co{'committer'} = hide_mailaddrs_if_private(to_utf8($1));
 			$co{'committer_epoch'} = $2;
 			$co{'committer_tz'} = $3;
 			if ($co{'committer'} =~ m/^([^<]+) <([^>]*)>/) {
@@ -3568,9 +3584,10 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		$line = hide_mailaddrs_if_private($line);
 	}
 	$co{'comment'} = \@commit_lines;
 
@@ -7489,7 +7506,8 @@ sub git_log_generic {
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
 	my $patch_max = gitweb_get_feature('patches');
-	if ($patch_max && !defined $file_name) {
+	if ($patch_max && !defined $file_name &&
+		!gitweb_check_feature('email-privacy')) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {
 			$paging_nav .= " &sdot; " .
 				$cgi->a({-href => href(action=>"patches", -replay=>1)},
@@ -7550,7 +7568,8 @@ sub git_commit {
 			} @$parents ) .
 			')';
 	}
-	if (gitweb_check_feature('patches') && @$parents <= 1) {
+	if (gitweb_check_feature('patches') && @$parents <= 1 &&
+		!gitweb_check_feature('email-privacy')) {
 		$formats_nav .= " | " .
 			$cgi->a({-href => href(action=>"patch", -replay=>1)},
 				"patch");
@@ -7863,7 +7882,8 @@ sub git_commitdiff {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
 			        "raw");
-		if ($patch_max && @{$co{'parents'}} <= 1) {
+		if ($patch_max && @{$co{'parents'}} <= 1 &&
+			!gitweb_check_feature('email-privacy')) {
 			$formats_nav .= " | " .
 				$cgi->a({-href => href(action=>"patch", -replay=>1)},
 					"patch");

base-commit: a5828ae6b52137b913b978e16cd2334482eb4c1f

Signature · 1 line
gitgitgadget
#25 lore raw

"Georgios Kontaxis via GitGitGadget" <[email protected]>

Quoted 24 lines · writes:

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This can result in unsolicited messages.

Introduce an 'email-privacy' feature which redacts e-mail addresses from the generated HTML content. Specifically, obscure addresses retrieved from the the author/committer and comment sections of the Git log. The feature is off by default.

This feature does not prevent someone from downloading the unredacted commit log, e.g., by cloning the repository, and extracting information from it. It aims to hinder the low- effort, bulk collection of e-mail addresses by web crawlers.

---

Documentation/gitweb.conf.txt +0 -0
@@ -751,6 +751,17 @@ default font sizes or lineheights are changed (e.g. via adding extra
 CSS stylesheet in `@stylesheets`), it may be appropriate to change
 these values.
 

Documentation/gitweb.conf.txt default font sizes or lineheights are changed (e.g. via adding extra · line 763

Quoted 10 lines
Documentation/gitweb.conf.txt +10 -0
+email-privacy::
+	Redact e-mail addresses from the generated HTML, etc. content.
+	This obscures e-mail addresses retrieved from the author/committer
+	and comment sections of the Git log.
+	It is meant to hinder web crawlers that harvest and abuse addresses.
+	Such crawlers may not respect robots.txt.
+	Note that users and user tools also see the addresses as redacted.
+	If Gitweb is not the final step in a workflow then subsequent steps
+	may misbehave because of the redacted information they receive.
+	Disabled by default.

OK. I still think everything after "Note that" is a bit redandant (as any intelligent reader can read what the feature does and reach its natural consequence themselves), but I do not strongly oppose leaving it in.

Quoted 8 lines
gitweb/gitweb.perl +0 -0
index 0959a782eccb..01c6faf88006 100755
@@ -569,6 +569,15 @@ sub evaluate_uri {
 		'sub' => \&feature_extra_branch_refs,
 		'override' => 0,
 		'default' => []},

gitweb/gitweb.perl sub evaluate_uri { · line 581

Quoted 10 lines
gitweb/gitweb.perl +9 -0
+
+	# Redact e-mail addresses.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'email-privacy'}{'default'} = [1];
+	'email-privacy' => {
+		'sub' => sub { feature_bool('email-privacy', @_) },
+		'override' => 1,
+		'default' => [0]},
 );

Sensible.

While reviewing this part, I noticed a few things

- A few other features explain how to toggle it system wide and nothing else, like this one does.

- Others explain, in addition, talk about how to toggle per repository (which may necessitate explaining how to toggle the override bit if the default is false).

It might be a good idea to standardise the explanation somehow.

This is just an observation (in other words, nothing needs to be done in this patch).

gitweb/gitweb.perl sub parse_date { · line 3466

Quoted 10 lines
gitweb/gitweb.perl +6 -0
@@ -3449,6 +3458,13 @@ sub parse_date {
 	return %date;
 }
 
+sub hide_mailaddrs_if_private {
+	my $line = shift;
+	return $line unless gitweb_check_feature('email-privacy');
+	$line =~ s/<[^@>]+@[^>]+>/<redacted>/ig;
+	return $line;
+}

OK. The "this catches AUTHOR_EMAIL" pattern is a bit embarrassing (my fault); "s/<[^@>]+@[-a-z0-9.]+>/<redacted>/ig" might be less embarrassing but I do not care deeply enough either way. The version in this patch is probably closer to what ident.c uses anyway ;-)

gitweb/gitweb.perl sub git_log_generic { · line 7511

Quoted 8 lines
gitweb/gitweb.perl +2 -1
@@ -7489,7 +7506,8 @@ sub git_log_generic {
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
 	my $patch_max = gitweb_get_feature('patches');
-	if ($patch_max && !defined $file_name) {
+	if ($patch_max && !defined $file_name &&
+		!gitweb_check_feature('email-privacy')) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {

An observation unrelated to this change. I think checking for negative patch_max is a bug in the original code. Everywhere else, the way to disable the 'patch' view is to set it to 0, not negative, and this block is already protected with "if ($patch_max".

Quoted 11 lines
gitweb/gitweb.perl +2 -1
@@ -7550,7 +7568,8 @@ sub git_commit {
 			} @$parents ) .
 			')';
 	}
-	if (gitweb_check_feature('patches') && @$parents <= 1) {
+	if (gitweb_check_feature('patches') && @$parents <= 1 &&
+		!gitweb_check_feature('email-privacy')) {
 		$formats_nav .= " | " .
 			$cgi->a({-href => href(action=>"patch", -replay=>1)},
 				"patch");

gitweb/gitweb.perl sub git_commitdiff { · line 7889

Quoted 9 lines
gitweb/gitweb.perl +2 -1
@@ -7863,7 +7882,8 @@ sub git_commitdiff {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
 			        "raw");
-		if ($patch_max && @{$co{'parents'}} <= 1) {
+		if ($patch_max && @{$co{'parents'}} <= 1 &&
+			!gitweb_check_feature('email-privacy')) {
 			$formats_nav .= " | " .
 				$cgi->a({-href => href(action=>"patch", -replay=>1)},
 					"patch");

I see your wish is "we do not show any link to a patch page, but anybody who knows what a link to the patch page looks like can craft and ask", but I am not sure if that is worth the above three hunks. It still feels more robust to just disable the 'patches' feature when the email-privacy feature is enabled, but that may be just me.

Will queue as-is. Input from those who are more adept at Perl and/or interested in helping polish gitweb are still welcome, but at my level of interest on the topic, this version looks as good as it gets ;-)

Thanks.

#26 lore raw
Quoted 4 lines · Junio C Hamano

Will queue as-is. Input from those who are more adept at Perl and/or interested in helping polish gitweb are still welcome, but at my level of interest on the topic, this version looks as good as it gets ;-)

Oh by the way, as nobody commented on the title, I'd have to X-<.

I'll retitle it to

gitweb: add "e-mail privacy" feature to redact e-mail addresses

while queuing.

Thanks.

#27 lore raw
In reply to Junio C Hamano
Quoted 10 lines · Junio C Hamano

"Georgios Kontaxis via GitGitGadget" <[email protected]>

Quoted 7 lines · writes:

From: Georgios Kontaxis <[email protected]>

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This can result in unsolicited messages.
...

Quoted 4 lines

Will queue as-is. Input from those who are more adept at Perl and/or interested in helping polish gitweb are still welcome, but at my level of interest on the topic, this version looks as good as it gets ;-)

Comments by anybody who is more adept at Perl and/or more well versed in gitweb and e-mail privacy issues?

Otherwise I am inclined to merge it down to 'next' as-is in a couple of days.

Thanks.

#28 lore raw
In reply to Georgios Kontaxis via GitGitGadget
Quoted 36 lines · Georgios Kontaxis via GitGitGadget

[...]

Documentation/gitweb.conf.txt +11 -0
+email-privacy::
+	Redact e-mail addresses from the generated HTML, etc. content.
+	This obscures e-mail addresses retrieved from the author/committer
+	and comment sections of the Git log.
+	It is meant to hinder web crawlers that harvest and abuse addresses.
+	Such crawlers may not respect robots.txt.
+	Note that users and user tools also see the addresses as redacted.
+	If Gitweb is not the final step in a workflow then subsequent steps
+	may misbehave because of the redacted information they receive.
+	Disabled by default.
+
 highlight::
 	Server-side syntax highlight support in "blob" view.  It requires
 	`$highlight_bin` program to be available (see the description of
gitweb/gitweb.perl +9 -0
index 0959a782eccb..01c6faf88006 100755
@@ -569,6 +569,15 @@ sub evaluate_uri {
 		'sub' => \&feature_extra_branch_refs,
 		'override' => 0,
 		'default' => []},
+
+	# Redact e-mail addresses.
+
+	# To enable system wide have in $GITWEB_CONFIG
+	# $feature{'email-privacy'}{'default'} = [1];
+	'email-privacy' => {
+		'sub' => sub { feature_bool('email-privacy', @_) },
+		'override' => 1,
+		'default' => [0]},
 );
 
 sub gitweb_get_feature {

gitweb/gitweb.perl sub parse_date { · line 3464

Quoted 7 lines
gitweb/gitweb.perl +4 -0
@@ -3449,6 +3458,13 @@ sub parse_date {
 	return %date;
 }
 
+sub hide_mailaddrs_if_private {
+	my $line = shift;
+	return $line unless gitweb_check_feature('email-privacy');
+	$line =~ s/<[^@>]+@[^>]+>/<redacted>/ig;

The /i here is redundant, since you have nothing that'll case-fold on the LHS of the s///, doesn't harm anything either. Just a small note since it's new in v6...

Quoted 66 lines
gitweb/gitweb.perl +12 -6
+	return $line;
+}
+
 sub parse_tag {
 	my $tag_id = shift;
 	my %tag;
@@ -3465,7 +3481,7 @@ sub parse_tag {
 		} elsif ($line =~ m/^tag (.+)$/) {
 			$tag{'name'} = $1;
 		} elsif ($line =~ m/^tagger (.*) ([0-9]+) (.*)$/) {
-			$tag{'author'} = $1;
+			$tag{'author'} = hide_mailaddrs_if_private($1);
 			$tag{'author_epoch'} = $2;
 			$tag{'author_tz'} = $3;
 			if ($tag{'author'} =~ m/^([^<]+) <([^>]*)>/) {
@@ -3513,7 +3529,7 @@ sub parse_commit_text {
 		} elsif ((!defined $withparents) && ($line =~ m/^parent ($oid_regex)$/)) {
 			push @parents, $1;
 		} elsif ($line =~ m/^author (.*) ([0-9]+) (.*)$/) {
-			$co{'author'} = to_utf8($1);
+			$co{'author'} = hide_mailaddrs_if_private(to_utf8($1));
 			$co{'author_epoch'} = $2;
 			$co{'author_tz'} = $3;
 			if ($co{'author'} =~ m/^([^<]+) <([^>]*)>/) {
@@ -3523,7 +3539,7 @@ sub parse_commit_text {
 				$co{'author_name'} = $co{'author'};
 			}
 		} elsif ($line =~ m/^committer (.*) ([0-9]+) (.*)$/) {
-			$co{'committer'} = to_utf8($1);
+			$co{'committer'} = hide_mailaddrs_if_private(to_utf8($1));
 			$co{'committer_epoch'} = $2;
 			$co{'committer_tz'} = $3;
 			if ($co{'committer'} =~ m/^([^<]+) <([^>]*)>/) {
@@ -3568,9 +3584,10 @@ sub parse_commit_text {
 	if (! defined $co{'title'} || $co{'title'} eq "") {
 		$co{'title'} = $co{'title_short'} = '(no commit message)';
 	}
-	# remove added spaces
+	# remove added spaces, redact e-mail addresses if applicable.
 	foreach my $line (@commit_lines) {
 		$line =~ s/^    //;
+		$line = hide_mailaddrs_if_private($line);
 	}
 	$co{'comment'} = \@commit_lines;
 
@@ -7489,7 +7506,8 @@ sub git_log_generic {
 			         -accesskey => "n", -title => "Alt-n"}, "next");
 	}
 	my $patch_max = gitweb_get_feature('patches');
-	if ($patch_max && !defined $file_name) {
+	if ($patch_max && !defined $file_name &&
+		!gitweb_check_feature('email-privacy')) {
 		if ($patch_max < 0 || @commitlist <= $patch_max) {
 			$paging_nav .= " &sdot; " .
 				$cgi->a({-href => href(action=>"patches", -replay=>1)},
@@ -7550,7 +7568,8 @@ sub git_commit {
 			} @$parents ) .
 			')';
 	}
-	if (gitweb_check_feature('patches') && @$parents <= 1) {
+	if (gitweb_check_feature('patches') && @$parents <= 1 &&
+		!gitweb_check_feature('email-privacy')) {
 		$formats_nav .= " | " .
 			$cgi->a({-href => href(action=>"patch", -replay=>1)},
 				"patch");

gitweb/gitweb.perl sub git_commitdiff { · line 7889

Quoted 9 lines
gitweb/gitweb.perl +2 -1
@@ -7863,7 +7882,8 @@ sub git_commitdiff {
 		$formats_nav =
 			$cgi->a({-href => href(action=>"commitdiff_plain", -replay=>1)},
 			        "raw");
-		if ($patch_max && @{$co{'parents'}} <= 1) {
+		if ($patch_max && @{$co{'parents'}} <= 1 &&
+			!gitweb_check_feature('email-privacy')) {
 			$formats_nav .= " | " .
 				$cgi->a({-href => href(action=>"patch", -replay=>1)},
 					"patch");

I didn't run this, and hadn't kept up for a few rounds. I'm happy to see the pos/while etc. looping gone, this LGTM.

#29 lore raw
Quoted 8 lines · Ævar Arnfjörð Bjarmason

gitweb/gitweb.perl sub parse_date { · line 3464

Quoted 4 lines
gitweb/gitweb.perl +4 -0
+sub hide_mailaddrs_if_private {
+	my $line = shift;
+	return $line unless gitweb_check_feature('email-privacy');
+	$line =~ s/<[^@>]+@[^>]+>/<redacted>/ig;

The /i here is redundant, since you have nothing that'll case-fold on the LHS of the s///, doesn't harm anything either. Just a small note since it's new in v6...

True. If it were left original version that was suggested during the review, e.g.

	s/<[^@]+@[-.a-z0-9]+>/<red@cted>/ig;

the /i/ would have been needed, but without the "after the @-sign should be a run of DNS-valid characters", I agree that there is no need.

Thanks. Will locally tweak.

#30 lore raw
In reply to Georgios Kontaxis via GitGitGadget
Quoted 7 lines · Georgios Kontaxis via GitGitGadget

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This can result in unsolicited messages.

Introduce an 'email-privacy' feature which redacts e-mail addresses from the generated HTML content

A general reply to the topic: have you considered munging addresses in a way that is still human readable, but obviously obfuscated?

On some other project, I settled on HTML "&#8226;" as a replacement for '.' for admins who enable that option. The $USER@$NO_DOT remains as-is for easy identification+recognition of hosts.

I also considered Unicode homographs which can look identical to replacement characters, too; but rejected that idea since it would cause grief for legitimate users who would not notice the homograph when pasting into their mail client.

Anyways, here's the list of candidates I tried:

homograph∂80x24.org
homograph@80x24ͺorg
homograph@80x24·org
homograph@80x24•org
homograph@80x24.org
homograph﹫80x24.org

https://en.wikipedia.org/wiki/Ano_Teleia#Similar_symbols
https://en.wikipedia.org/wiki/Enclosed_A

homographⒶ80x24.org
homograph@80x24 org
homograph@80x24․org
homograph@80x24ꓸorg

#31 lore raw
Quoted 9 lines
Quoted 7 lines · Georgios Kontaxis via GitGitGadget

Gitweb extracts content from the Git log and makes it accessible over HTTP. As a result, e-mail addresses found in commits are exposed to web crawlers and they may not respect robots.txt. This can result in unsolicited messages.

Introduce an 'email-privacy' feature which redacts e-mail addresses from the generated HTML content

Quoted 8 lines

A general reply to the topic: have you considered munging addresses in a way that is still human readable, but obviously obfuscated?

On some other project, I settled on HTML "&#8226;" as a replacement for '.' for admins who enable that option. The $USER@$NO_DOT remains as-is for easy identification+recognition of hosts.

Thanks for the suggestion.

People have been trying to hinder address harvesting for a while now. Replacing '@' with "at", the dot with "dot", adding spaces, etc. was pretty common at some point. May still be.
I would expect crawlers to have caught up and this includes all sorts of character encodings and unicode look-alike substitutions.

At the end of the day we are looking for something that's easy for humans to read but hard for scripts to parse as an e-mail address. (And that scripts cannot learn through an additional regex) I'm not aware of anything like that. (I know CAPTCHAs, etc.)

Quoted 22 lines

I also considered Unicode homographs which can look identical to replacement characters, too; but rejected that idea since it would cause grief for legitimate users who would not notice the homograph when pasting into their mail client.

Anyways, here's the list of candidates I tried:

homograph∂80x24.org
homograph@80x24ͺorg
homograph@80x24·org
homograph@80x24•org
homograph@80x24.org
homograph﹫80x24.org

https://en.wikipedia.org/wiki/Ano_Teleia#Similar_symbols
https://en.wikipedia.org/wiki/Enclosed_A

homographⒶ80x24.org
homograph@80x24 org
homograph@80x24․org
homograph@80x24ꓸorg

#32 lore raw
Quoted 13 lines · Georgios Kontaxis
Quoted 4 lines
Quoted 2 lines · Georgios Kontaxis via GitGitGadget

Introduce an 'email-privacy' feature which redacts e-mail addresses from the generated HTML content

Quoted 8 lines · Eric Wong

A general reply to the topic: have you considered munging addresses in a way that is still human readable, but obviously obfuscated?

On some other project, I settled on HTML "&#8226;" as a replacement for '.' for admins who enable that option. The $USER@$NO_DOT remains as-is for easy identification+recognition of hosts.

Quoted 7 lines

Thanks for the suggestion.

People have been trying to hinder address harvesting for a while now. Replacing '@' with "at", the dot with "dot", adding spaces, etc. was pretty common at some point. May still be.
I would expect crawlers to have caught up and this includes all sorts of character encodings and unicode look-alike substitutions.

I figure the crawlers hit a combinatorial explosion and give up since they'd be wasting time with false-positives.

Quoted 4 lines
Quoted 4 lines

I also considered Unicode homographs which can look identical to replacement characters, too; but rejected that idea since it would cause grief for legitimate users who would not notice the homograph when pasting into their mail client.

As a data point, none of the homograph@ candidates I posted here on Mar 29 have attracted any attempts on my mail server.

#33 lore raw
Quoted 9 lines · Eric Wong
Quoted 8 lines · Georgios Kontaxis
Quoted 4 lines
Quoted 2 lines · Georgios Kontaxis via GitGitGadget

Introduce an 'email-privacy' feature which redacts e-mail addresses from the generated HTML content

Quoted 3 lines · Eric Wong

A general reply to the topic: have you considered munging addresses in a way that is still human readable, but obviously obfuscated?

Quoted 8 lines
Quoted 5 lines

...

Quoted 4 lines

I also considered Unicode homographs which can look identical to replacement characters, too; but rejected that idea since it would cause grief for legitimate users who would not notice the homograph when pasting into their mail client.

As a data point, none of the homograph@ candidates I posted here on Mar 29 have attracted any attempts on my mail server.

That is an interesting observation. All homograph@ non-addresses, if a human corrected the funnies in their spelling, would have hit whoever handles @80x24.org mailboxes.

I take it to mean that as a future direction, replacing <redacted> with the obfuscated-but-readable-by-humans homographs is a likely improvement that would help human users while still inconveniencing the crawlers. It may however need some provision to prevent casual end-users from cutting-and-pasting these homographs, as you said in your original mention of the homograph approach.

But other than that, does the patch look reasonable?

Thanks.

#34 lore raw
Quoted 8 lines · Junio C Hamano
Quoted 2 lines · Eric Wong

As a data point, none of the homograph@ candidates I posted here on Mar 29 have attracted any attempts on my mail server.

That is an interesting observation. All homograph@ non-addresses, if a human corrected the funnies in their spelling, would have hit whoever handles @80x24.org mailboxes.

Quoted 6 lines

I take it to mean that as a future direction, replacing <redacted> with the obfuscated-but-readable-by-humans homographs is a likely improvement that would help human users while still inconveniencing the crawlers. It may however need some provision to prevent casual end-users from cutting-and-pasting these homographs, as you said in your original mention of the homograph approach.

Yes, exactly.

Quoted 1 line

But other than that, does the patch look reasonable?

I only took a cursory glance at it, but v6 seemed fine.

#35 lore raw
Quoted 16 lines · Eric Wong
Quoted 8 lines · Junio C Hamano
Quoted 2 lines · Eric Wong

As a data point, none of the homograph@ candidates I posted here on Mar 29 have attracted any attempts on my mail server.

That is an interesting observation. All homograph@ non-addresses, if a human corrected the funnies in their spelling, would have hit whoever handles @80x24.org mailboxes.

Quoted 6 lines

I take it to mean that as a future direction, replacing <redacted> with the obfuscated-but-readable-by-humans homographs is a likely improvement that would help human users while still inconveniencing the crawlers. It may however need some provision to prevent casual end-users from cutting-and-pasting these homographs, as you said in your original mention of the homograph approach.

Quoted 5 lines

Yes, exactly.

Quoted 1 line

But other than that, does the patch look reasonable?

I only took a cursory glance at it, but v6 seemed fine.

Ditto, I left a small nit comment about a needless /i in a regex, but I don't think that needs a re-roll.

#36 lore raw
Quoted 18 lines · Ævar Arnfjörð Bjarmason
Quoted 16 lines · Eric Wong
Quoted 14 lines · Junio C Hamano
Quoted 2 lines · Eric Wong

As a data point, none of the homograph@ candidates I posted here on Mar 29 have attracted any attempts on my mail server.

That is an interesting observation. All homograph@ non-addresses, if a human corrected the funnies in their spelling, would have hit whoever handles @80x24.org mailboxes.

I take it to mean that as a future direction, replacing <redacted> with the obfuscated-but-readable-by-humans homographs is a likely improvement that would help human users while still inconveniencing the crawlers. It may however need some provision to prevent casual end-users from cutting-and-pasting these homographs, as you said in your original mention of the homograph approach.

Quoted 8 lines
Quoted 5 lines

Yes, exactly.

Quoted 1 line

But other than that, does the patch look reasonable?

I only took a cursory glance at it, but v6 seemed fine.

Ditto, I left a small nit comment about a needless /i in a regex, but I don't think that needs a re-roll.

Thanks, both.

Will tweak the /i out, and re-queue with acked-by from you two.

#37 lore raw
In reply to Georgios Kontaxis via GitGitGadget

"Georgios Kontaxis via GitGitGadget" <[email protected]>

gitweb/gitweb.perl sub evaluate_uri { · line 581

Quoted 7 lines · writes:
gitweb/gitweb.perl +6 -0
+    # To disable system wide have in $GITWEB_CONFIG
+    # $feature{'email_privacy'}{'default'} = [0];
+	'email_privacy' => {
+		'sub' => sub { feature_bool('email_privacy', @_) },
+		'override' => 0,
+		'default' => [1]},
 );

I do not see why this should default to true.

It would break existing installations, who have been perfectly happy with the convenience of supplying a ready access to potential new contributors who/which addresses to contact plausible mentors in the projects they are interested in.

And more importantly, I do not see why it should be made impossible to override per repository/project in a multi-tenant installation. Some projects may be more "privacy" sensitive than others. Those who want to use tighter setting should be able to enable it even when the side-wide default is set to false, no?

Thanks.

#38 lore raw
Quoted 5 lines · Junio C Hamano

And more importantly, I do not see why it should be made impossible to override per repository/project in a multi-tenant installation. Some projects may be more "privacy" sensitive than others. Those who want to use tighter setting should be able to enable it even when the side-wide default is set to false, no?

To answer an inevitable and natural follow-up question preemptively, the primary reason why we have the override => 0 mechanism is so that site administrators can disable certain expensive features (like blame, snapshot, etc.) no matter what each project hosted by them wish.

And hiding contributor identity would not be a choice that is based on how expensive the feature is to run.

#39 lore raw
In reply to Junio C Hamano
Quoted 13 lines

"Georgios Kontaxis via GitGitGadget" <[email protected]>

gitweb/gitweb.perl sub evaluate_uri { · line 581

Quoted 7 lines · writes:
gitweb/gitweb.perl +6 -0
+    # To disable system wide have in $GITWEB_CONFIG
+    # $feature{'email_privacy'}{'default'} = [0];
+	'email_privacy' => {
+		'sub' => sub { feature_bool('email_privacy', @_) },
+		'override' => 0,
+		'default' => [1]},
 );

I do not see why this should default to true.

I've changed the default to "false". V2 should reflect the change.

Quoted 5 lines

It would break existing installations, who have been perfectly happy with the convenience of supplying a ready access to potential new contributors who/which addresses to contact plausible mentors in the projects they are interested in.

Quoted 8 lines

And more importantly, I do not see why it should be made impossible to override per repository/project in a multi-tenant installation. Some projects may be more "privacy" sensitive than others. Those who want to use tighter setting should be able to enable it even when the side-wide default is set to false, no?

Thanks.

I was actually thinking about the other way around; preventing projects from disabling this feature.

Sounds like the "override" flag is for other types of use cases though. I'll change it to "true".

Thanks for the feedback.

#40 lore raw
Quoted 10 lines · Georgios Kontaxis
Quoted 8 lines

And more importantly, I do not see why it should be made impossible to override per repository/project in a multi-tenant installation. Some projects may be more "privacy" sensitive than others. Those who want to use tighter setting should be able to enable it even when the side-wide default is set to false, no?

Thanks.

I was actually thinking about the other way around; preventing projects from disabling this feature.

Yes, it cuts both ways, and override should be allowed for most cases (unless absolutely necessary for healthy operation of the system) for a simple reason that anybody who sets site-wide default is not in a better position than those who set per-repository or per-project setting to judge what is good for them.

Thanks.