Junio C Hamano <gitster@pobox.com> writes:

> avar@cpan.org (Ævar Arnfjörð Bjarmason) writes:
>
>> +    unless ($user eq 'anonymous') {
>> +        # Trying to authenticate a user
>> +        if (not exists $cfg->{gitcvs}->{users}) {
>> +            print "E the repo config file needs a [gitcvs.users] section with user/password key-value pairs\n";
>> +            print "I HATE YOU\n";
>> +            exit 1;
>> +        } elsif (exists $cfg->{gitcvs}->{users} and not exists $cfg->{gitcvs}->{users}->{$user}) {
>> +            print "E the repo config file has a [gitcvs.users] section but the user $user is not defined in it\n";
>> +            print "I HATE YOU\n";
>> +            exit 1;
>> +        } else {
>> +            my $descrambled_password = descramble($password);
>> +            my $cleartext_password = $cfg->{gitcvs}->{users}->{$user};
>> +            if ($descrambled_password ne $cleartext_password) {
>> +                print "E The password supplied for user $user was incorrect\n";
>> +                print "I HATE YOU\n";
>> +                exit 1;
>> +            }
>
> I do not know what the real pserver does but by sending these E lines in
> the latter two different forms back to the client you are leaking
> sensitive information, which is probably not what you want (the first
> one is Ok, though.  It would help the server administrator to notice
> misconfiguration, and until it is corrected nobody would be able to log
> in anyway).

I've commented out those two error messages, it now doesn't provide that
info.

> Admittedly, the pserver password scrambling is not a real security, but
> if we were paranoid, we would probably be even adding random delay in
> "no user found" case and "password does not match" case, so that the
> client cannot even tell from the response latency if a username exists
> at the server.
>
>> @@ -1176,12 +1196,6 @@ sub req_ci
>>  
>>      $log->info("req_ci : " . ( defined($data) ? $data : "[NULL]" ));
>>  
>> -    if ( $state->{method} eq 'pserver')
>> -    {
>> -        print "error 1 pserver access cannot commit\n";
>> -        exit;
>> -    }
>> -
>
> Is this correct?  You are still allowing anonymous pserver access, so
> shouldn't you check if this was an anonymous access or authenticated one
> and refuse access like before for anonymous people?

No it's not, oops. Fixed.

>> +    my ($str) = @_;
>> +
>> +    # This should never happen, the same format has been used since
>> +    # CVS was spawned
>> +    $str =~ s/^(.)//;
>> +    die "invalid password format $1" unless $1 eq 'A';
>
> I do not quite understand what "spawn" means in this sentence.

Shawn O. Pearce put it best but I've changed the comment to be more
readable to someone without a odd sense of humor:)