[PATCH] Do not load user or system gitconfig and gitattributes

[PATCH] Do not load user or system gitconfig and gitattributes

[Jason]

From: "Jason A. Donenfeld" <Jason at zx2c4.com>

While doing any kind of git loading, unset HOME variables and set
NOSYSTEM variables so that cgit does not load any settings that a user
may have set for his own /usr/bin/git usage.

This fixes a fatal error introduced with git 1.8, whereupon git would
fatally exit when failing to access particular files.

Reported-by: Ferry Huberts <ferry.huberts at pelagic.nl>
Tested-by: Jason A. Donenfeld <Jason at zx2c4.com>
Tested-by: Ferry Huberts <ferry.huberts at pelagic.nl>
Signed-off-by: Jason A. Donenfeld <Jason at zx2c4.com>
---
 cgit.c      | 24 ++++++++++++++++++++++++
 ui-commit.c |  1 -
 ui-log.c    |  1 -
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/cgit.c b/cgit.c
index f73c7b0..0bf8972 100644
--- a/cgit.c
+++ b/cgit.c
@@ -459,12 +459,36 @@ static char *guess_defbranch(void)
 
 static int prepare_repo_cmd(struct cgit_context *ctx)
 {
+	char *user_home;
+	char *xdg_home;
 	unsigned char sha1[20];
 	int nongit = 0;
 	int rc;
 
+	/* The path to the git repository. */
 	setenv("GIT_DIR", ctx->repo->path, 1);
+
+	/* Do not look in /etc/ for gitconfig and gitattributes. */
+	setenv("GIT_CONFIG_NOSYSTEM", "1", 1);
+	setenv("GIT_ATTR_NOSYSTEM", "1", 1);
+
+	/* We unset HOME and XDG_CONFIG_HOME before calling the git setup function
+	 * so that we don't make unneccessary filesystem accesses. */
+	user_home = getenv("HOME");
+	xdg_home = getenv("XDG_CONFIG_HOME");
+	unsetenv("HOME");
+	unsetenv("XDG_CONFIG_HOME");
+
+	/* Setup the git directory and initialize the notes system. Both of these
+	 * load local configuration from the git repository, so we do them both while
+	 * the HOME variables are unset. */
 	setup_git_directory_gently(&nongit);
+	init_display_notes(NULL);
+
+	/* We restore the unset variables afterward. */
+	setenv("HOME", user_home, 1);
+	setenv("XDG_CONFIG_HOME", xdg_home, 1);
+
 	if (nongit) {
 		const char *name = ctx->repo->name;
 		rc = errno;
diff --git a/ui-commit.c b/ui-commit.c
index 6b41017..a5a6ea8 100644
--- a/ui-commit.c
+++ b/ui-commit.c
@@ -37,7 +37,6 @@ void cgit_print_commit(char *hex, const char *prefix)
 	}
 	info = cgit_parse_commit(commit);
 
-	init_display_notes(NULL);
 	format_display_notes(sha1, &notes, PAGE_ENCODING, 0);
 
 	load_ref_decorations(DECORATE_FULL_REFS);
diff --git a/ui-log.c b/ui-log.c
index 93af0ce..2aa12c3 100644
--- a/ui-log.c
+++ b/ui-log.c
@@ -403,7 +403,6 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
 		commit->parents = NULL;
 	}
 
-	init_display_notes(NULL);
 	for (i = 0; i < cnt && (commit = get_revision(&rev)) != NULL; i++) {
 		print_commit(commit, &rev);
 		free(commit->buffer);
-- 
1.8.1.5

[PATCH] Do not load user or system gitconfig and gitattributes

[mailings]

On 08/04/13 21:33, Jason A. Donenfeld wrote:
> From: "Jason A. Donenfeld" <Jason at zx2c4.com>
> 
> While doing any kind of git loading, unset HOME variables and set
> NOSYSTEM variables so that cgit does not load any settings that a user
> may have set for his own /usr/bin/git usage.
> 
> This fixes a fatal error introduced with git 1.8, whereupon git would
> fatally exit when failing to access particular files.
> 
> Reported-by: Ferry Huberts <ferry.huberts at pelagic.nl>
> Tested-by: Jason A. Donenfeld <Jason at zx2c4.com>
> Tested-by: Ferry Huberts <ferry.huberts at pelagic.nl>
> Signed-off-by: Jason A. Donenfeld <Jason at zx2c4.com>
> ---
>  cgit.c      | 24 ++++++++++++++++++++++++
>  ui-commit.c |  1 -
>  ui-log.c    |  1 -
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/cgit.c b/cgit.c
> index f73c7b0..0bf8972 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -459,12 +459,36 @@ static char *guess_defbranch(void)
>  
>  static int prepare_repo_cmd(struct cgit_context *ctx)
>  {
> +	char *user_home;
> +	char *xdg_home;
>  	unsigned char sha1[20];
>  	int nongit = 0;
>  	int rc;
>  
> +	/* The path to the git repository. */
>  	setenv("GIT_DIR", ctx->repo->path, 1);
> +
> +	/* Do not look in /etc/ for gitconfig and gitattributes. */
> +	setenv("GIT_CONFIG_NOSYSTEM", "1", 1);
> +	setenv("GIT_ATTR_NOSYSTEM", "1", 1);

John's patch has a 'no override' here, which I think is better.
Also I like the place where John sets these up better, at the start of
execution.


> +
> +	/* We unset HOME and XDG_CONFIG_HOME before calling the git setup function
> +	 * so that we don't make unneccessary filesystem accesses. */
> +	user_home = getenv("HOME");
> +	xdg_home = getenv("XDG_CONFIG_HOME");
> +	unsetenv("HOME");
> +	unsetenv("XDG_CONFIG_HOME");
> +
> +	/* Setup the git directory and initialize the notes system. Both of these
> +	 * load local configuration from the git repository, so we do them both while
> +	 * the HOME variables are unset. */
>  	setup_git_directory_gently(&nongit);
> +	init_display_notes(NULL);
> +
> +	/* We restore the unset variables afterward. */
> +	setenv("HOME", user_home, 1);
> +	setenv("XDG_CONFIG_HOME", xdg_home, 1);
> +
>  	if (nongit) {
>  		const char *name = ctx->repo->name;
>  		rc = errno;
> diff --git a/ui-commit.c b/ui-commit.c
> index 6b41017..a5a6ea8 100644
> --- a/ui-commit.c
> +++ b/ui-commit.c
> @@ -37,7 +37,6 @@ void cgit_print_commit(char *hex, const char *prefix)
>  	}
>  	info = cgit_parse_commit(commit);
>  
> -	init_display_notes(NULL);
>  	format_display_notes(sha1, &notes, PAGE_ENCODING, 0);
>  
>  	load_ref_decorations(DECORATE_FULL_REFS);
> diff --git a/ui-log.c b/ui-log.c
> index 93af0ce..2aa12c3 100644
> --- a/ui-log.c
> +++ b/ui-log.c
> @@ -403,7 +403,6 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
>  		commit->parents = NULL;
>  	}
>  
> -	init_display_notes(NULL);
>  	for (i = 0; i < cnt && (commit = get_revision(&rev)) != NULL; i++) {
>  		print_commit(commit, &rev);
>  		free(commit->buffer);
> 

-- 
Ferry Huberts

[PATCH] Do not load user or system gitconfig and gitattributes

[Jason]

On Mon, Apr 8, 2013 at 9:37 PM, Ferry Huberts <mailings at hupie.com> wrote:
> John's patch has a 'no override' here, which I think is better.

IMHO, cgit should _not_ process system wide or environmental
configuration data, both to eliminate potential security holes and to
ensure that git will behave deterministically.

> Also I like the place where John sets these up better, at the start of
> execution.

I'd like to restore HOME in case it's useful later on in cgit
development, which means unsetting it in main() isn't so great. As
well, prefer to do the git initialization in one isolated place, in
which we can have a careful idea of what the state of the program is,
rather than saying "oh, somewhere it was setup, hopefully things are
okay, I'll initialize something now here." In my patch, the relevant
git environment variables are set/unset in the same place as GIT_DIR.

[PATCH] Do not load user or system gitconfig and gitattributes

[john]

On Mon, Apr 08, 2013 at 09:33:31PM +0200, Jason A. Donenfeld wrote:
> From: "Jason A. Donenfeld" <Jason at zx2c4.com>
> 
> While doing any kind of git loading, unset HOME variables and set
> NOSYSTEM variables so that cgit does not load any settings that a user
> may have set for his own /usr/bin/git usage.
> 
> This fixes a fatal error introduced with git 1.8, whereupon git would
> fatally exit when failing to access particular files.
> 
> Reported-by: Ferry Huberts <ferry.huberts at pelagic.nl>
> Tested-by: Jason A. Donenfeld <Jason at zx2c4.com>
> Tested-by: Ferry Huberts <ferry.huberts at pelagic.nl>
> Signed-off-by: Jason A. Donenfeld <Jason at zx2c4.com>
> ---

Looks sensible, but I don't think it's quite guaranteed to do what you
want, since Git can call git_config at any time and it re-reads the
files whenever it does.  We may not be doing anything that causes it to
be reparsed now, but will we notice if that changes?

Is there any reason not to leave $HOME and $XDG_CONFIG_HOME unset?

Another (very) minor point:

    /*
     * Git prefers to format multi-line comments
     * like this.
     */

I don't know how closely you think CGit should stick to that style
though.

[patch left unsnipped for reference]

> diff --git a/cgit.c b/cgit.c
> index f73c7b0..0bf8972 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -459,12 +459,36 @@ static char *guess_defbranch(void)
>  
>  static int prepare_repo_cmd(struct cgit_context *ctx)
>  {
> +	char *user_home;
> +	char *xdg_home;
>  	unsigned char sha1[20];
>  	int nongit = 0;
>  	int rc;
>  
> +	/* The path to the git repository. */
>  	setenv("GIT_DIR", ctx->repo->path, 1);
> +
> +	/* Do not look in /etc/ for gitconfig and gitattributes. */
> +	setenv("GIT_CONFIG_NOSYSTEM", "1", 1);
> +	setenv("GIT_ATTR_NOSYSTEM", "1", 1);
> +
> +	/* We unset HOME and XDG_CONFIG_HOME before calling the git setup function
> +	 * so that we don't make unneccessary filesystem accesses. */
> +	user_home = getenv("HOME");
> +	xdg_home = getenv("XDG_CONFIG_HOME");
> +	unsetenv("HOME");
> +	unsetenv("XDG_CONFIG_HOME");
> +
> +	/* Setup the git directory and initialize the notes system. Both of these
> +	 * load local configuration from the git repository, so we do them both while
> +	 * the HOME variables are unset. */
>  	setup_git_directory_gently(&nongit);
> +	init_display_notes(NULL);
> +
> +	/* We restore the unset variables afterward. */
> +	setenv("HOME", user_home, 1);
> +	setenv("XDG_CONFIG_HOME", xdg_home, 1);
> +
>  	if (nongit) {
>  		const char *name = ctx->repo->name;
>  		rc = errno;
> diff --git a/ui-commit.c b/ui-commit.c
> index 6b41017..a5a6ea8 100644
> --- a/ui-commit.c
> +++ b/ui-commit.c
> @@ -37,7 +37,6 @@ void cgit_print_commit(char *hex, const char *prefix)
>  	}
>  	info = cgit_parse_commit(commit);
>  
> -	init_display_notes(NULL);
>  	format_display_notes(sha1, &notes, PAGE_ENCODING, 0);
>  
>  	load_ref_decorations(DECORATE_FULL_REFS);
> diff --git a/ui-log.c b/ui-log.c
> index 93af0ce..2aa12c3 100644
> --- a/ui-log.c
> +++ b/ui-log.c
> @@ -403,7 +403,6 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
>  		commit->parents = NULL;
>  	}
>  
> -	init_display_notes(NULL);
>  	for (i = 0; i < cnt && (commit = get_revision(&rev)) != NULL; i++) {
>  		print_commit(commit, &rev);
>  		free(commit->buffer);
> -- 
> 1.8.1.5
> 
> 
> _______________________________________________
> cgit mailing list
> cgit at hjemli.net
> http://hjemli.net/mailman/listinfo/cgit

[PATCH] Do not load user or system gitconfig and gitattributes

[Jason]

On Mon, Apr 8, 2013 at 9:44 PM, John Keeping <john at keeping.me.uk> wrote:
>
> Looks sensible, but I don't think it's quite guaranteed to do what you
> want, since Git can call git_config at any time and it re-reads the
> files whenever it does.  We may not be doing anything that causes it to
> be reparsed now, but will we notice if that changes?

Test case time!

>
> Is there any reason not to leave $HOME and $XDG_CONFIG_HOME unset?

Yes. Various parts of cgit shell out to scripts -- filters etc -- and
these scripts may very much benefit from having a $HOME.



> I don't know how closely you think CGit should stick to that style
> though.

Actually, to date, cgit has been using

/* Comments that
 *  are like this.
 */

as well as

/*
  * Comments that
 *  are like this.
 */

as well as

// comments that
// are like this.

the last of which is pretty gross.

At some point I'll have to clean up and normalize all of these.