All of lore.kernel.org
 help / color / mirror / Atom feed
From: Simon Oosthoek <s.oosthoek@xs4all.nl>
To: G?bor Szeder <szeder@ira.uka.de>
Cc: Junio C Hamano <gitster@pobox.com>,
	sitaramc@gmail.com, Richard Hansen <rhansen@bbn.com>,
	git@vger.kernel.org
Subject: Re: [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1
Date: Fri, 25 Apr 2014 09:37:24 +0200	[thread overview]
Message-ID: <20140425073724.GA9384@xs4all.nl> (raw)
In-Reply-To: <E1WdOZY-0006ck-F9@iramx2.ira.uni-karlsruhe.de>

* G?bor Szeder <szeder@ira.uka.de> [2014-04-24 23:10:10 +0430]:

> > I'd like to see this patch eyeballed by those who have been involved 
> > in the script (shortlog and blame tells me they are SZEDER and 
> > Simon, CC'ed), so that we can hopefully merge it by the time -rc1 is 
> > tagged.
> 
> I think this is a sensible thing to do.  However, for now I can only check the patch on my phone, hence I can't say any more (e.g. acked or reviewed by) than that, unfortunately.

Ditto for me, though I've gone so far as to try it (it works for me). At the time I wrote the patch I honestly forgot to think about the security implications and from the description, this is closing a hole. There are situations where you're not in control of a branch name (though tbh, I think you'd have to be in an automated situation to check out a branch that is basically a command to hack your system, a human would probably figure it too cumbersome, or too fishy)

> 
> > > + # not needed anymore; keep user's 
> > > + # environment clean 
> > > + unset __git_ps1_upstream_name 
> > > + fi
> 
> We already have a lot of stuff in the user's environment beginning with __git, so I don't think the unset is necessary.

If people rely on the string being set in their scripts, it can be bad to remove it. But if it's new in this patch, I don't see the need to keep it. Cruft is bad IMO.

Cheers

Simon

  reply	other threads:[~2014-04-25  7:37 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-24 18:40 [SECURITY PATCH] git-prompt.sh: don't put unsanitized branch names in $PS1 Gábor Szeder
2014-04-25  7:37 ` Simon Oosthoek [this message]
2014-04-25 16:39   ` Richard Hansen
  -- strict thread matches above, loose matches on Subject: below --
2014-04-21 19:07 Richard Hansen
2014-04-21 20:24 ` Jeff King
2014-04-21 21:07   ` Richard Hansen
2014-04-22  8:38   ` Michael Haggerty
2014-04-22 17:38     ` Junio C Hamano
2014-04-22 18:38       ` Richard Hansen
2014-04-22 19:47         ` Junio C Hamano
2014-04-21 22:23 ` Junio C Hamano
2014-04-21 22:33   ` Junio C Hamano
2014-04-21 22:58     ` Richard Hansen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140425073724.GA9384@xs4all.nl \
    --to=s.oosthoek@xs4all.nl \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=rhansen@bbn.com \
    --cc=sitaramc@gmail.com \
    --cc=szeder@ira.uka.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.