580778 - hash keys containing non-word characters such as dashes are dropped from web service params

Status: RESOLVED WONTFIX

(Bugzilla :: WebService, defect)

Description

[David Lawrence [:dkl]]

Attachment: Patch to allow hash keys in web services that contain dashes (v1)

I noticed that params containing dashes were being dropped from out web service params such as field0-0-0, etc. and therefore breaking out bug search methods in 3.6. In Bugzilla::WebService::Util::_delete_bad_keys (called by taint_data()) is
performing a \w+ match on the hash key and dropping those that do not match.

            if ($key !~ /^\w+$/) {
                delete $item->{$key};
            }

If I change that to $key !~ /^[\w-]+$/ it will no longer drop keys containing dashes.

Attaching a patch for 3.6.

Dave

Comment 1

[Max Kanat-Alexander]

Comment on attachment 459155 [details] [diff] [review]
Patch to allow hash keys in web services that contain dashes (v1)

Unfortunately, "-" is not legal in a SQL identifier, and params are, in some cases, inserted directly into SQL. There are probably situations in which "-" could be used to inject SQL or modify the behavior of Bugzilla, since it means "minus" in SQL.

For your WebServices, you may have to put on a little frontend bit that converts the "field-" to "field_".

Comment 2

[Max Kanat-Alexander]

Because this is a security issue, for the reasons above, I'm unfortunately going to have to mark this WONTFIX.

Comment 3

[David Lawrence [:dkl]]

(In reply to comment #1)
> Comment on attachment 459155 [details] [diff] [review]
> Patch to allow hash keys in web services that contain dashes (v1)
> 
> Unfortunately, "-" is not legal in a SQL identifier, and params are, in some
> cases, inserted directly into SQL. There are probably situations in which "-"
> could be used to inject SQL or modify the behavior of Bugzilla, since it means
> "minus" in SQL.
> 
> For your WebServices, you may have to put on a little frontend bit that
> converts the "field-" to "field_".

This will need more thought as I would assume that the webservices API will eventually expand to allow similar search capability that web UI provides. Maybe converting the web UI to use "field0_0_0" instead of "field0-0-0"
for example?

When you say add a frontend bit for conversion, do you mean customize _delete_bad_keys to do the following?:


sub _delete_bad_keys {
    foreach my $item (@_) {
        next if ref $item ne 'HASH';
        foreach my $key (keys %$item) {
            $key =~ s/-/_/g;
            if ($key !~ /^[\w-]+$/) {
                delete $item->{$key};
            }
        }
    }
    return @_;
}

Comment 4

[David Lawrence [:dkl]]

(In reply to comment #3) 
> sub _delete_bad_keys {
>     foreach my $item (@_) {
>         next if ref $item ne 'HASH';
>         foreach my $key (keys %$item) {
>             $key =~ s/-/_/g;
>             if ($key !~ /^[\w-]+$/) {
>                 delete $item->{$key};
>             }

Acutally it would be:

sub _delete_bad_keys {
    foreach my $item (@_) {
        next if ref $item ne 'HASH';
        foreach my $key (keys %$item) {
            my $old_key = $key;
            $key =~ s/-/_/g;
            if ($key !~ /^[\w-]+$/) {
                delete $item->{$old_key};
            }
        }
    }
    return @_;
}

Comment 5

[Max Kanat-Alexander]

(In reply to comment #3)
> This will need more thought as I would assume that the webservices API will
> eventually expand to allow similar search capability that web UI provides.

  Yeah. But it most likely won't do that until after the boolean chart redesign, and will have some data structure as input.

> When you say add a frontend bit for conversion, do you mean customize
> _delete_bad_keys to do the following?:

  Yeah, that would work. (With of course the fix you mentioned in your next comment.)