Bug 550727 (bz-jsonp)

Add JSONP Support to the JSON-RPC WebService Interface

RESOLVED FIXED in Bugzilla 4.0

Status

()

Bugzilla
WebService
P1
enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

Bugzilla 4.0
Bug Flags:
approval +

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
JSONP is a method of doing cross-domain requests involving JSON. The original proposal is described here:

  http://bob.pythonmac.org/archives/2005/12/05/remote-json-jsonp/

  That is exactly (more or less) what we will be implementing. We will not be implementing any JSONP extensions, like JSONPP or something like that.

  There are significant security concerns associated with JSONP. Here are the ones I'm aware of and how they will be dealt with:

  CSRF: When using JSONP, you cannot use any JSON-RPC method that will modify data. You may only use methods that retrieve data.

  Information Leak (security bug exposure): Because JSONP involves using script tags, cookies will be sent to the remote site, meaning that a site could silently retrieve secure data without the user's knowledge, and then send it to itself, thus effectively stealing private data. The solution to this is to not allow cookie-based login when using JSONP. We will only allow explicit login using method parameters. So a remote site will only be able to access private information if a user explicitly types in their password at that site.

  Code Insertion: If you let people specify any arbitrary thing for the "padding", they could insert all sorts of code that runs in the context of your domain, which is bad. The solution is that we only allow function names that match \w+.

  Are there any other security issues that I should be aware of, or I have I covered them all?
(Assignee)

Comment 1

7 years ago
  Oh, and to be clear, the form of JSONP that we will be implementing is the sort that wraps the returned data in a callback function, because I think that's the most flexible. Theoretically, our data could also be assigned to a variable (like var callback = { };) but the function seems more flexible and less-likely to pollute somebody's namespace with endless variables.
(Assignee)

Updated

7 years ago
Alias: bz-jsonp
(Assignee)

Updated

7 years ago
Depends on: 550732
(Assignee)

Updated

7 years ago
Priority: -- → P1
(Assignee)

Comment 2

7 years ago
Created attachment 430910 [details] [diff] [review]
v1

  Here we go! After implementing the GET stuff, the remaining stuff for implementing JSONP was pretty simple.

  Note that this requires the patch from the blocker.
Assignee: webservice → mkanat
Status: NEW → ASSIGNED
Attachment #430910 - Flags: review?(dkl)
One comment: when I looked into it for the BzAPI, a common name for the URL parameter giving the name of the JavaScript function to be called is "callback" (i.e. "callback=myfunction") - although there didn't seem to be a standard. So that's what I used. It would be good to have the name the same in both APIs. If there's a standard, point me at it. Otherwise, we should agree something :-)

Gerv
(Assignee)

Comment 4

7 years ago
  "callback" seems reasonable. That does seem to be what most things are using. I'll re-post the patch with "callback" as the argument.
(Assignee)

Comment 5

7 years ago
Created attachment 431625 [details] [diff] [review]
v2

  Okay, this version uses "callback" as the URL argument.
Attachment #430910 - Attachment is obsolete: true
Attachment #431625 - Flags: review?(dkl)
Attachment #430910 - Flags: review?(dkl)
(Assignee)

Comment 6

7 years ago
Created attachment 431626 [details] [diff] [review]
v3

  And you know, we might as well allow brackets in the callback, too. The actual example in the original JSONP spec uses brackets.
Attachment #431625 - Attachment is obsolete: true
Attachment #431626 - Flags: review?(dkl)
Attachment #431625 - Flags: review?(dkl)
Comment on attachment 431626 [details] [diff] [review]
v3

With the patch from 550732 applied and this one together I get the following warning from JSONRPC.pm:

Subroutine response redefined at Bugzilla/WebService/Server/JSONRPC.pm line 155.
Bugzilla/WebService/Server/JSONRPC.pm syntax OK

response() is present in two places and needs to be combined into one.

I combined them in my code like below and my testing worked well. Fix this and then r=dkl.

sub response {
    my ($self, $response) = @_;
    my $headers = $response->headers;
    my @header_args;
    foreach my $name ($headers->header_field_names) {
        my @values = $headers->header($name);
        $name =~ s/-/_/g;
        foreach my $value (@values) {
            push(@header_args, "-$name", $value);
        }
    }

    if (my $callback = $self->_bz_callback) {
        my $content = $response->content;
        $response->content("$callback($content)");

    }

    my $cgi = $self->cgi;
    print $cgi->header(-status => $response->code, @header_args);
    print $response->content;
}
Attachment #431626 - Flags: review?(dkl) → review-
(Assignee)

Comment 8

7 years ago
Created attachment 440838 [details] [diff] [review]
v4

Ah, thanks for catching that! Here's the patch with it fixed.
Attachment #431626 - Attachment is obsolete: true
Attachment #440838 - Flags: review?(dkl)
(Assignee)

Updated

7 years ago
Keywords: relnote
Comment on attachment 440838 [details] [diff] [review]
v4

Looks good and works as expected. r=dkl
Attachment #440838 - Flags: review?(dkl) → review+

Updated

7 years ago
Flags: approval?
(Assignee)

Updated

7 years ago
Flags: approval? → approval+
(Assignee)

Comment 10

7 years ago
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/WebService/Server/JSONRPC.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7145.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

7 years ago
Added to the release notes in bug 604256.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.