* git log -p unexpected behaviour - security risk?
@ 2013-04-11 10:36 John Tapsell
2013-04-11 15:19 ` Tay Ray Chuan
2013-04-20 14:00 ` Simon Ruderich
0 siblings, 2 replies; 22+ messages in thread
From: John Tapsell @ 2013-04-11 10:36 UTC (permalink / raw)
To: Git List
Hi,
I noticed that code that you put in merge will not be visible by
default. This seems like a pretty horrible security problem, no?
I made the following test tree, with just 3 commits:
https://github.com/johnflux/ExampleEvilness.git
Doing "git log -p" shows all very innocent commits. Completely
hidden is the change to add "EVIL CODE MUWHAHAHA".
This seems really dangerous!
The evil code only shows up with the non-default --cc or -m option.
Is there a way to make --cc default?
John
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-11 10:36 git log -p unexpected behaviour - security risk? John Tapsell
@ 2013-04-11 15:19 ` Tay Ray Chuan
2013-04-20 14:00 ` Simon Ruderich
1 sibling, 0 replies; 22+ messages in thread
From: Tay Ray Chuan @ 2013-04-11 15:19 UTC (permalink / raw)
To: John Tapsell; +Cc: Git List
On Thu, Apr 11, 2013 at 6:36 PM, John Tapsell <johnflux@gmail.com> wrote:
> I noticed that code that you put in merge will not be visible by
> default. This seems like a pretty horrible security problem, no?
>
> I made the following test tree, with just 3 commits:
>
> https://github.com/johnflux/ExampleEvilness.git
>
> Doing "git log -p" shows all very innocent commits. Completely
> hidden is the change to add "EVIL CODE MUWHAHAHA".
>
> This seems really dangerous!
>
> The evil code only shows up with the non-default --cc or -m option.
For email-based patch workflows (eg. git, linux kernel), then this is
not a problem - the diff doesn't even show up, so nothing is applied
when git-am is run.
For github with pull-requests, a diff is shown between trees, so this
will show up.
--
Cheers,
Ray Chuan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-11 10:36 git log -p unexpected behaviour - security risk? John Tapsell
2013-04-11 15:19 ` Tay Ray Chuan
@ 2013-04-20 14:00 ` Simon Ruderich
2013-04-21 7:26 ` Junio C Hamano
1 sibling, 1 reply; 22+ messages in thread
From: Simon Ruderich @ 2013-04-20 14:00 UTC (permalink / raw)
To: John Tapsell; +Cc: Git List, Tay Ray Chuan, Junio C Hamano
On Thu, Apr 11, 2013 at 11:36:26AM +0100, John Tapsell wrote:
> Is there a way to make --cc default?
If you use aliases, something like this is easy:
git config --global --add alias.lp 'log --patch --cc'
I use aliases heavily, so that's my fix for now.
But I think the current behaviour is unexpected for most (new?)
users (including me). I thought -p would display all changes in
all commits, including merges.
I guess changing -p's default behaviour to imply --cc is
problematic, so I think we should document that -p doesn't
generate patches for merges. Maybe something like this:
-- 8< --
Subject: [PATCH] Documentation/diff-options.txt: -p doesn't display merge changes
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---
Documentation/diff-options.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 104579d..cd35ec7 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -24,6 +24,10 @@ ifndef::git-format-patch[]
--patch::
Generate patch (see section on generating patches).
{git-diff? This is the default.}
+ifdef::git-log[]
+ Changes introduced in merge commits are not displayed. Use `-c`,
+ `--cc` or `-m` to include them.
+endif::git-log[]
endif::git-format-patch[]
-U<n>::
--
1.8.2.1.513.gdedbb69.dirty
-- 8< --
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-20 14:00 ` Simon Ruderich
@ 2013-04-21 7:26 ` Junio C Hamano
2013-04-21 8:56 ` John Tapsell
0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2013-04-21 7:26 UTC (permalink / raw)
To: Simon Ruderich; +Cc: John Tapsell, Git List, Tay Ray Chuan
Simon Ruderich <simon@ruderich.org> writes:
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 104579d..cd35ec7 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -24,6 +24,10 @@ ifndef::git-format-patch[]
> --patch::
> Generate patch (see section on generating patches).
> {git-diff? This is the default.}
> +ifdef::git-log[]
> + Changes introduced in merge commits are not displayed. Use `-c`,
> + `--cc` or `-m` to include them.
> +endif::git-log[]
It probably is a better change to drop "Use `-c`..." and refer to
the "Diff formatting" section.
And then add '-p' and the fact that by default it will not show
pairwise diff for merge commits to the "Diff Formatting" section.
That is where -c/--cc/-m are already described.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 7:26 ` Junio C Hamano
@ 2013-04-21 8:56 ` John Tapsell
2013-04-21 10:21 ` Jonathan Nieder
0 siblings, 1 reply; 22+ messages in thread
From: John Tapsell @ 2013-04-21 8:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Simon Ruderich, Git List, Tay Ray Chuan
On 21 April 2013 08:26, Junio C Hamano <gitster@pobox.com> wrote:
> Simon Ruderich <simon@ruderich.org> writes:
>
>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index 104579d..cd35ec7 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -24,6 +24,10 @@ ifndef::git-format-patch[]
>> --patch::
>> Generate patch (see section on generating patches).
>> {git-diff? This is the default.}
>> +ifdef::git-log[]
>> + Changes introduced in merge commits are not displayed. Use `-c`,
>> + `--cc` or `-m` to include them.
>> +endif::git-log[]
>
> It probably is a better change to drop "Use `-c`..." and refer to
> the "Diff formatting" section.
>
> And then add '-p' and the fact that by default it will not show
> pairwise diff for merge commits to the "Diff Formatting" section.
> That is where -c/--cc/-m are already described.
Why not have it in both places? This is really important.
I'm concerned that noone is taking this security risk seriously. Just
because it doesn't show up in certain workflows doesn't make the risk
go away.
What about all the people who use git internally? They aren't using
github and almost certainly aren't using a mail based system.
It's bad that we can't even set the right behaviour as a default.
John
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 8:56 ` John Tapsell
@ 2013-04-21 10:21 ` Jonathan Nieder
2013-04-21 13:46 ` John Tapsell
2013-04-21 18:25 ` Junio C Hamano
0 siblings, 2 replies; 22+ messages in thread
From: Jonathan Nieder @ 2013-04-21 10:21 UTC (permalink / raw)
To: John Tapsell; +Cc: Junio C Hamano, Simon Ruderich, Git List, Tay Ray Chuan
John Tapsell wrote:
> I'm concerned that noone is taking this security risk seriously.
If anyone relies on "git log -p" or "git log -p --cc" output to make
sure that the untrusted code they use doesn't introduce unwanted
behavior, they are making a serious mistake. A merge can completely
undo important changes made in a side branch and "-c" and "--cc" will
not show it. The lack of "-c" cannot be a security issue here,
because in normal life adding "-c" isn't a secure deployment strategy.
That's why if you want to review the code you are pulling in as a
whole, it is worthwhile to do
git diff HEAD...FETCH_HEAD
That is how you ask "What code changes does FETCH_HEAD introduce?"
before putting your stamp of approval on them by merging and pushing
out the result. Unfortunately that doesn't protect you from
maliciously written commits that will be encountered when bisecting.
At some point you have to be able to trust people.
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 10:21 ` Jonathan Nieder
@ 2013-04-21 13:46 ` John Tapsell
2013-04-21 15:56 ` Thomas Rast
2013-04-21 16:09 ` Jonathan Nieder
2013-04-21 18:25 ` Junio C Hamano
1 sibling, 2 replies; 22+ messages in thread
From: John Tapsell @ 2013-04-21 13:46 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Simon Ruderich, Git List, Tay Ray Chuan
On 21 April 2013 11:21, Jonathan Nieder <jrnieder@gmail.com> wrote:
> John Tapsell wrote:
>
>> I'm concerned that noone is taking this security risk seriously.
>
> If anyone relies on "git log -p" or "git log -p --cc" output to make
> sure that the untrusted code they use doesn't introduce unwanted
> behavior, they are making a serious mistake.
Which is exactly my problem.
Go and ask the average person using git this very question, and I bet
you the vast majority will not know about -cc etc.
You can't just push all the blame on the user for bad defaults.
Hiding code changes is a bad default.
> A merge can completely
> undo important changes made in a side branch and "-c" and "--cc" will
> not show it.
Wait, what? This is getting even worse then! Can you expand on this please?
And then how do I show all of these important changes with a git log -p ?
Or is it impossible to get a sane output?
> The lack of "-c" cannot be a security issue here,
> because in normal life adding "-c" isn't a secure deployment strategy.
So, is it impossible to make git log -p a "secure deployment strategy" ?
> That's why if you want to review the code you are pulling in as a
> whole, it is worthwhile to do
>
> git diff HEAD...FETCH_HEAD
Which basically means that you're asking the review the same code
twice. Once that way, and once using git log -p (to check for the
exact reason that you said).
> Unfortunately that doesn't protect you from
> maliciously written commits that will be encountered when bisecting.
> At some point you have to be able to trust people.
Seriously? Your reasoning for awful defaults is that you should just
trust people?
This is getting worse and worse!
John
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 13:46 ` John Tapsell
@ 2013-04-21 15:56 ` Thomas Rast
2013-04-21 16:09 ` Jonathan Nieder
1 sibling, 0 replies; 22+ messages in thread
From: Thomas Rast @ 2013-04-21 15:56 UTC (permalink / raw)
To: John Tapsell
Cc: Jonathan Nieder, Junio C Hamano, Simon Ruderich, Git List,
Tay Ray Chuan
John Tapsell <johnflux@gmail.com> writes:
> On 21 April 2013 11:21, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>> A merge can completely
>> undo important changes made in a side branch and "-c" and "--cc" will
>> not show it.
>
> Wait, what? This is getting even worse then! Can you expand on this please?
>
> And then how do I show all of these important changes with a git log -p ?
> Or is it impossible to get a sane output?
It pretty much by definition does not show changes if the merge picks
one side unchanged:
-c
[...] lists only files which were modified from all parents.
--cc
This flag implies the -c option and further compresses the patch
output [...]
On top of that, the default history simplification when you specify a
pathspec will only walk the (or any one) unchanged side of such a merge,
so if you filter for a file you wouldn't even find the offending commit
further back in history.
I don't think this can be improved easily with the current one-pass[1]
history/diff generation. To know what the merge *should* have done,
you'd need to somehow get an idea what parts of the resulting files
should be affected, which AFAICS boils down to redoing the merge. And
to do that, you need to scan history so far that you can compute the
merge-bases. Not to mention that redoing all merges while walking
history is somewhat expensive.
You could hack up a script that does the verification manually, by
actually running a merge and comparing the result with what the merge
gave you. But it's not something that you would want to run by default.
[1] some things like --simplify-merges are actually another pass, but
the default is to generate everything -- including diffs -- as we go.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 13:46 ` John Tapsell
2013-04-21 15:56 ` Thomas Rast
@ 2013-04-21 16:09 ` Jonathan Nieder
2013-04-21 18:42 ` Junio C Hamano
2013-04-30 11:48 ` git log -p unexpected behaviour - security risk? shawn wilson
1 sibling, 2 replies; 22+ messages in thread
From: Jonathan Nieder @ 2013-04-21 16:09 UTC (permalink / raw)
To: John Tapsell; +Cc: Junio C Hamano, Simon Ruderich, Git List, Tay Ray Chuan
John Tapsell wrote:
> Jonathan Nieder wrote:
>> If anyone relies on "git log -p" or "git log -p --cc" output to make
>> sure that the untrusted code they use doesn't introduce unwanted
>> behavior, they are making a serious mistake.
[...]
> You can't just push all the blame on the user for bad defaults.
The thing is, I'm not convinced this is a bad default. "Shows no diff
at all for merges" is easy for a person to understand. It is much
easier to understand its limitations than -c and --cc. For that
reason, it is a much *better* default for security than --cc or -c
(even though I believe one of the latter would be a better default for
convenience).
I agree that this is an important documentation bug, since
introductory documentation does not explain clearly enough how
"git log -p" will act for merges.
>> A merge can completely
>> undo important changes made in a side branch and "-c" and "--cc" will
>> not show it.
>
> Wait, what? This is getting even worse then! Can you expand on this please?
If a given file matches one of its parents, there is nothing to show
in the combined diff format. Otherwise every merge would have a very
long diff.
If what you really want is the diff against the first parent, you
can use -m --first-parent with -p. If you want the diffs against each
parent, you can use -m -p.
[...]
>> Unfortunately that doesn't protect you from
>> maliciously written commits that will be encountered when bisecting.
>> At some point you have to be able to trust people.
>
> Seriously? Your reasoning for awful defaults is that you should just
> trust people?
I didn't set the defaults. I'm explaining how the tool currently
behaves in response to your question. A person can do many
unfortunate things if you blindly trust them and merge from them.
For example, whenever git adds (or plans) support for a new header
line in commit objects, before you've upgraded, a prankster can
provide a bad value for that header line in objects they hand-craft.
"git fsck" in your older version of git will accept the resulting
objects on the assumption that they came from a newer version of git,
so you won't notice. Later you upgrade Git and "git fsck" considers
the objects malformed. Clients with "[transfer] fsckobjects" enabled
start to reject your history. That is, this person has made your
repository corrupt in the eyes of "git fsck".
The usual excellent integrity checking will let you pinpoint the
problem to the merge from that untrusted person so you can avoid
trusting them again, and all the data will be there to recover without
them. So it is auditable later. But this does mean that with the
current design, there is some level of trust required to let someone
commit into your history unless you inspect their work with a
fine-toothed comb.
All that said, if someone has ideas for improving git's support for
such inspection, that would be great. "-c" just isn't it. "-c" can
be a good tool for finding honest mistakes, but it doesn't protect
well against an adversary.
In the meantime, if you didn't intend to trust those people this much,
this might mean your procedures (and git's documentation, for the sake
of others in the same boat) need some changes. Sorry to be the bearer
of bad news.
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 10:21 ` Jonathan Nieder
2013-04-21 13:46 ` John Tapsell
@ 2013-04-21 18:25 ` Junio C Hamano
1 sibling, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2013-04-21 18:25 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan
Jonathan Nieder <jrnieder@gmail.com> writes:
> That's why if you want to review the code you are pulling in as a
> whole, it is worthwhile to do
>
> git diff HEAD...FETCH_HEAD
>
> That is how you ask "What code changes does FETCH_HEAD introduce?"
> before putting your stamp of approval on them by merging and pushing
> out the result.
And the only way to retroactively review that a merge C did not do
anything funly is to check "git diff C^1 C", assuming that you
already trust C^1, the state before you performed the merge.
Incidentally, this works for non-merge commits just as well.
"git log -m -p" is not the default because most of the time people
are not interested in seeing what it shows over "--cc" or "-c". It
is a repetition of what you would get from individual patches on the
side branch merged that you will later see in the traversal of that
command. "--cc/-c" gives a representation for tricky merge cases
where people could likely have made a mistake, or had correctly
resolved semantic conflicts (e.g. one side renames a function, the
other side adds a callsite, the merge result renames the function
new caller calls).
For the purpose of a "merge audit" John seems to want, the only way
is to wade through "log -m -p" output.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 16:09 ` Jonathan Nieder
@ 2013-04-21 18:42 ` Junio C Hamano
2013-04-30 10:09 ` John Szakmeister
2013-04-30 11:48 ` git log -p unexpected behaviour - security risk? shawn wilson
1 sibling, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2013-04-21 18:42 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: John Tapsell, Simon Ruderich, Git List, Tay Ray Chuan
Jonathan Nieder <jrnieder@gmail.com> writes:
> The thing is, I'm not convinced this is a bad default. "Shows no diff
> at all for merges" is easy for a person to understand. It is much
> easier to understand its limitations than -c and --cc.
Making "log -p -m" a default before -c/--cc was introduced would
have been the stupidest thing to do, as it would make the command
mostly useless. Nobody would want to see repetitious output from a
merge that he would eventually get when the traversal drills down to
individual commits on the merged side branch.
When I added -c/--cc, I contemplated making -p imply --cc, but
decided against it primarily because it is a change in traditional
behaviour, and it is easy for users to say --cc instead of -p from
the command line.
On the other hand, "show" was a newer command and it was easy to
turn its default to --cc without having to worry too much about
existing users.
> For that
> reason, it is a much *better* default for security than --cc or -c
> (even though I believe one of the latter would be a better default for
> convenience).
Yes. I do not fundamentally oppose to the idea of "log -p" to imply
"log --cc" when "-m" is not given ("log -p -m" is specifically
declining the combined diff simplification). It may be a usability
improvement.
But "--cc/-c" does not have anything to do with Tapsell's "security
worries". The only real audit he can do is with "log -m -p",
possibly with --first-parent (only if he trusts his first-parent
history).
The "recreate mechanical merge and compare recorded merge against
it" mode may highlight a malicious merger, but it will not show a
cleanly merged hunk of malicious code in the merge, so it cannot be
used with --first-parent when used as a "security audit tool".
Tapsell still needs to drill down to the merged side branch that
introduced the malicious change that merged cleanly with "-p".
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 18:42 ` Junio C Hamano
@ 2013-04-30 10:09 ` John Szakmeister
2013-04-30 16:37 ` Junio C Hamano
0 siblings, 1 reply; 22+ messages in thread
From: John Szakmeister @ 2013-04-30 10:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, John Tapsell, Simon Ruderich, Git List,
Tay Ray Chuan
On Sun, Apr 21, 2013 at 2:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> The thing is, I'm not convinced this is a bad default. "Shows no diff
>> at all for merges" is easy for a person to understand. It is much
>> easier to understand its limitations than -c and --cc.
>
> Making "log -p -m" a default before -c/--cc was introduced would
> have been the stupidest thing to do, as it would make the command
> mostly useless. Nobody would want to see repetitious output from a
> merge that he would eventually get when the traversal drills down to
> individual commits on the merged side branch.
>
> When I added -c/--cc, I contemplated making -p imply --cc, but
> decided against it primarily because it is a change in traditional
> behaviour, and it is easy for users to say --cc instead of -p from
> the command line.
FWIW, security aside, I would've like to have seen that. I find it
confusing that merge commits that introduce code don't have a diff
shown when using -p. And I find it hard to remember --cc. BTW,
what's the mnemonic for it? -p => patch, --cc => ?
> On the other hand, "show" was a newer command and it was easy to
> turn its default to --cc without having to worry too much about
> existing users.
>
>> For that
>> reason, it is a much *better* default for security than --cc or -c
>> (even though I believe one of the latter would be a better default for
>> convenience).
>
> Yes. I do not fundamentally oppose to the idea of "log -p" to imply
> "log --cc" when "-m" is not given ("log -p -m" is specifically
> declining the combined diff simplification). It may be a usability
> improvement.
Would you consider such a patch?
-John
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-21 16:09 ` Jonathan Nieder
2013-04-21 18:42 ` Junio C Hamano
@ 2013-04-30 11:48 ` shawn wilson
1 sibling, 0 replies; 22+ messages in thread
From: shawn wilson @ 2013-04-30 11:48 UTC (permalink / raw)
To: Jonathan Nieder
Cc: John Tapsell, Junio C Hamano, Simon Ruderich, Git List,
Tay Ray Chuan
Sorta OT, but I'm curious,
On Sun, Apr 21, 2013 at 12:09 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> For example, whenever git adds (or plans) support for a new header
> line in commit objects, before you've upgraded, a prankster can
> provide a bad value for that header line in objects they hand-craft.
> "git fsck" in your older version of git will accept the resulting
> objects on the assumption that they came from a newer version of git,
> so you won't notice. Later you upgrade Git and "git fsck" considers
> the objects malformed. Clients with "[transfer] fsckobjects" enabled
> start to reject your history. That is, this person has made your
> repository corrupt in the eyes of "git fsck".
>
> The usual excellent integrity checking will let you pinpoint the
> problem to the merge from that untrusted person so you can avoid
> trusting them again, and all the data will be there to recover without
> them. So it is auditable later. But this does mean that with the
> current design, there is some level of trust required to let someone
> commit into your history unless you inspect their work with a
> fine-toothed comb.
>
Has anyone written a test case for this?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-30 10:09 ` John Szakmeister
@ 2013-04-30 16:37 ` Junio C Hamano
2013-04-30 16:47 ` John Szakmeister
2013-04-30 17:05 ` Matthieu Moy
0 siblings, 2 replies; 22+ messages in thread
From: Junio C Hamano @ 2013-04-30 16:37 UTC (permalink / raw)
To: John Szakmeister
Cc: Jonathan Nieder, John Tapsell, Simon Ruderich, Git List,
Tay Ray Chuan
John Szakmeister <john@szakmeister.net> writes:
>> When I added -c/--cc, I contemplated making -p imply --cc, but
>> decided against it primarily because it is a change in traditional
>> behaviour, and it is easy for users to say --cc instead of -p from
>> the command line.
>
> FWIW, security aside, I would've like to have seen that. I find it
> confusing that merge commits that introduce code don't have a diff
> shown when using -p. And I find it hard to remember --cc. BTW,
> what's the mnemonic for it? -p => patch, --cc => ?
Compact combined.
By the way, these options are _not_ about "showing merge commits
that introduce code", and they do not help your kind of "security".
As I repeatedly said, you would need "-p -m" for that.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-30 16:37 ` Junio C Hamano
@ 2013-04-30 16:47 ` John Szakmeister
2013-04-30 17:05 ` Matthieu Moy
1 sibling, 0 replies; 22+ messages in thread
From: John Szakmeister @ 2013-04-30 16:47 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, John Tapsell, Simon Ruderich, Git List,
Tay Ray Chuan
On Tue, Apr 30, 2013 at 12:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Szakmeister <john@szakmeister.net> writes:
>
>>> When I added -c/--cc, I contemplated making -p imply --cc, but
>>> decided against it primarily because it is a change in traditional
>>> behaviour, and it is easy for users to say --cc instead of -p from
>>> the command line.
>>
>> FWIW, security aside, I would've like to have seen that. I find it
>> confusing that merge commits that introduce code don't have a diff
>> shown when using -p. And I find it hard to remember --cc. BTW,
>> what's the mnemonic for it? -p => patch, --cc => ?
>
> Compact combined.
Thank you.
> By the way, these options are _not_ about "showing merge commits
> that introduce code", and they do not help your kind of "security".
> As I repeatedly said, you would need "-p -m" for that.
I'm sorry, I didn't mean to imply that it's useful for security, just
that it better meets my expectations when -p is turned on. I realize
there are some edges in the logic, but I'm fine with those edges.
-John
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-30 16:37 ` Junio C Hamano
2013-04-30 16:47 ` John Szakmeister
@ 2013-04-30 17:05 ` Matthieu Moy
2013-04-30 17:58 ` John Szakmeister
1 sibling, 1 reply; 22+ messages in thread
From: Matthieu Moy @ 2013-04-30 17:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: John Szakmeister, Jonathan Nieder, John Tapsell, Simon Ruderich,
Git List, Tay Ray Chuan
Junio C Hamano <gitster@pobox.com> writes:
> By the way, these options are _not_ about "showing merge commits
> that introduce code", and they do not help your kind of "security".
> As I repeatedly said, you would need "-p -m" for that.
Actually, while defaulting to --cc may be convenient, it would indeed
increase the security risk: currently, "git log -p" shows nothing for
merges, so it's rather clear that _everything_ is omitted. With --cc,
the user would see a diff, and could hardly guess that not everything is
shown without reading the doc very carefully.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-30 17:05 ` Matthieu Moy
@ 2013-04-30 17:58 ` John Szakmeister
2013-04-30 19:31 ` John Tapsell
0 siblings, 1 reply; 22+ messages in thread
From: John Szakmeister @ 2013-04-30 17:58 UTC (permalink / raw)
To: Matthieu Moy
Cc: Junio C Hamano, Jonathan Nieder, John Tapsell, Simon Ruderich,
Git List, Tay Ray Chuan
On Tue, Apr 30, 2013 at 1:05 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> By the way, these options are _not_ about "showing merge commits
>> that introduce code", and they do not help your kind of "security".
>> As I repeatedly said, you would need "-p -m" for that.
>
> Actually, while defaulting to --cc may be convenient, it would indeed
> increase the security risk: currently, "git log -p" shows nothing for
> merges, so it's rather clear that _everything_ is omitted. With --cc,
> the user would see a diff, and could hardly guess that not everything is
> shown without reading the doc very carefully.
I don't believe it's that clear. I bet people assume there's nothing
to show, and unless you dig in and discover that `-p` doesn't include
merges. In git 1.8.2, `git help log` doesn't seem to make any mention
of `-p` not showing a diff for merges.
Just to see, I asked several people around here whether they knew `-p`
didn't show diffs for merges, and they were all surprised that diffs
were being omitted for merge commits.
-John
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour - security risk?
2013-04-30 17:58 ` John Szakmeister
@ 2013-04-30 19:31 ` John Tapsell
2013-04-30 19:44 ` git log -p unexpected behaviour Junio C Hamano
0 siblings, 1 reply; 22+ messages in thread
From: John Tapsell @ 2013-04-30 19:31 UTC (permalink / raw)
To: Git List
On 30 April 2013 18:58, John Szakmeister <john@szakmeister.net> wrote:
> On Tue, Apr 30, 2013 at 1:05 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> By the way, these options are _not_ about "showing merge commits
>>> that introduce code", and they do not help your kind of "security".
>>> As I repeatedly said, you would need "-p -m" for that.
>>
>> Actually, while defaulting to --cc may be convenient, it would indeed
>> increase the security risk: currently, "git log -p" shows nothing for
>> merges, so it's rather clear that _everything_ is omitted. With --cc,
>> the user would see a diff, and could hardly guess that not everything is
>> shown without reading the doc very carefully.
>
> I don't believe it's that clear. I bet people assume there's nothing
> to show, and unless you dig in and discover that `-p` doesn't include
> merges. In git 1.8.2, `git help log` doesn't seem to make any mention
> of `-p` not showing a diff for merges.
>
> Just to see, I asked several people around here whether they knew `-p`
> didn't show diffs for merges, and they were all surprised that diffs
> were being omitted for merge commits.
Is there no way to fix --cc to work even in the edge cases?
John
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour
2013-04-30 19:31 ` John Tapsell
@ 2013-04-30 19:44 ` Junio C Hamano
2013-04-30 20:12 ` John Tapsell
0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2013-04-30 19:44 UTC (permalink / raw)
To: John Tapsell; +Cc: Git List
John Tapsell <johnflux@gmail.com> writes:
> Is there no way to fix --cc to work even in the edge cases?
Can you clarify what you mean by "fix" and "edge cases"?
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour
2013-04-30 19:44 ` git log -p unexpected behaviour Junio C Hamano
@ 2013-04-30 20:12 ` John Tapsell
2013-04-30 20:38 ` Junio C Hamano
0 siblings, 1 reply; 22+ messages in thread
From: John Tapsell @ 2013-04-30 20:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
On 30 April 2013 20:44, Junio C Hamano <gitster@pobox.com> wrote:
> John Tapsell <johnflux@gmail.com> writes:
>
>> Is there no way to fix --cc to work even in the edge cases?
>
> Can you clarify what you mean by "fix" and "edge cases"?
My understanding is that even with -cc there will be changes that
won't be seen - and hence why --cc could be even more of a "security
risk", no?
John
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour
2013-04-30 20:12 ` John Tapsell
@ 2013-04-30 20:38 ` Junio C Hamano
2013-05-01 7:23 ` John Tapsell
0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2013-04-30 20:38 UTC (permalink / raw)
To: John Tapsell; +Cc: Git List
John Tapsell <johnflux@gmail.com> writes:
> On 30 April 2013 20:44, Junio C Hamano <gitster@pobox.com> wrote:
>> John Tapsell <johnflux@gmail.com> writes:
>>
>>> Is there no way to fix --cc to work even in the edge cases?
>>
>> Can you clarify what you mean by "fix" and "edge cases"?
>
> My understanding is that even with -cc there will be changes that
> won't be seen - and hence why --cc could be even more of a "security
> risk", no?
Combined diff is a way to show a tricky conflict resolved in a
tricky way, so that the tricky-ness of the resolution can be
examined. A trivial resolution that takes one side is not shown
because it is not usually interesting. This design choice of course
have to trust people involved in the project do not pull from
untrustworthy sources.
You would need "log -p -m" (without any pathspec) for the kind of
"security" you are talking about. Note that "-p -m --first-parent"
is not necessarily enough.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: git log -p unexpected behaviour
2013-04-30 20:38 ` Junio C Hamano
@ 2013-05-01 7:23 ` John Tapsell
0 siblings, 0 replies; 22+ messages in thread
From: John Tapsell @ 2013-05-01 7:23 UTC (permalink / raw)
To: Git List
On 30 April 2013 21:38, Junio C Hamano <gitster@pobox.com> wrote:
> John Tapsell <johnflux@gmail.com> writes:
>
>> On 30 April 2013 20:44, Junio C Hamano <gitster@pobox.com> wrote:
>>> John Tapsell <johnflux@gmail.com> writes:
>>>
>>>> Is there no way to fix --cc to work even in the edge cases?
>>>
>>> Can you clarify what you mean by "fix" and "edge cases"?
>>
>> My understanding is that even with -cc there will be changes that
>> won't be seen - and hence why --cc could be even more of a "security
>> risk", no?
>
> Combined diff is a way to show a tricky conflict resolved in a
> tricky way, so that the tricky-ness of the resolution can be
> examined. A trivial resolution that takes one side is not shown
> because it is not usually interesting.
I don't really understand your point sorry. In this trivial
resolution case, you would still just see the commit that added the
code in a later commit. No?
There couldn't be a case where you add or change a line of code, but
not see it with --cc ?
> This design choice of course
> have to trust people involved in the project do not pull from
> untrustworthy sources.
>
> You would need "log -p -m" (without any pathspec) for the kind of
> "security" you are talking about. Note that "-p -m --first-parent"
> is not necessarily enough.
This results in seeing the same change more than once though, right?
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-05-01 7:23 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-11 10:36 git log -p unexpected behaviour - security risk? John Tapsell
2013-04-11 15:19 ` Tay Ray Chuan
2013-04-20 14:00 ` Simon Ruderich
2013-04-21 7:26 ` Junio C Hamano
2013-04-21 8:56 ` John Tapsell
2013-04-21 10:21 ` Jonathan Nieder
2013-04-21 13:46 ` John Tapsell
2013-04-21 15:56 ` Thomas Rast
2013-04-21 16:09 ` Jonathan Nieder
2013-04-21 18:42 ` Junio C Hamano
2013-04-30 10:09 ` John Szakmeister
2013-04-30 16:37 ` Junio C Hamano
2013-04-30 16:47 ` John Szakmeister
2013-04-30 17:05 ` Matthieu Moy
2013-04-30 17:58 ` John Szakmeister
2013-04-30 19:31 ` John Tapsell
2013-04-30 19:44 ` git log -p unexpected behaviour Junio C Hamano
2013-04-30 20:12 ` John Tapsell
2013-04-30 20:38 ` Junio C Hamano
2013-05-01 7:23 ` John Tapsell
2013-04-30 11:48 ` git log -p unexpected behaviour - security risk? shawn wilson
2013-04-21 18:25 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).