Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bbttxu’s picture

FileSize
4.03 KB
andremolnar’s picture

I'm not opposed to the change, but I don't
think this functionality should be enabled by default.

What would you say to having this be:
a) off by default with an admin option to turn it on

Admins that know they want this on will turn it on and understand what
they are getting themselves into. If the admin wants to be malicious,
they have to jump through one more hoop before unleashing a plague on
the users of their service.

b) more validation of the value of the callback parameter

Not that it really really really matters since the requester is
obviously in control of the page already, but what if they formed their
request like this:

var hello = function(data,response) {
console.log(data);console.log(response) };
$.get('http://example.com/something.php',
{'id':18,'callback':'alert("I like cheese");hello'}, null, 'jsonp' );

c) allow the admin to specify what the name of the expected parameter is
when a client sends a request with a 'callback' (with a default name of
callback).

This is kind of sort of security through obfuscation where the client
would need to know the name of the param to make a successful jsonp
request (instead of just being able to guess that passing
'callback':'somefunction' works, the would need to know that
'custom-method-name':'somefunction' is what they need to do).

abritez’s picture

..subscribing

abritez’s picture

has anyone had luck getting this to work? Not having much luck here. Not sure if i am missing anything. Would you happen to have a simple example calling system.connect? Using json_server 6.x-1.x-dev w/ the latest Services 6.x-0.15

Thanks for any help on this
Alex

andremolnar’s picture

Status: Patch (to be ported) » Needs work

Just updating the status on this- the existing patch does not address everything in #2

nickvidal’s picture

@andremolnar

I disagree. I don't believe you understand how JSONP works. There is no security problem by letting the client define the callback. In fact, that's what JSONP is all about. And this is how it's done by everybody, including Yahoo, etc.

The modification is quite simple, actually, and a client can consume pure JSON simply by not setting the callback. Please see the modifications here:

http://drupal.org/node/624898

andremolnar’s picture

@nickvidal

I think maybe you misread my comments.

The client obviously needs to define the callback function name. I was saying the server admin should have the option of saying what the name of the expected parameter is for the server to respond.

The name 'callback' is a sensible default
$.get('http://example.com/something.php', {'id':18,'callback':'hello'}, null, 'jsonp' );

But lets say the site admin only wants to send back a JSONp respone to the word 'foo' and not the word 'callback'. The above call wouldn't work, but the one below would.
$.get('http://example.com/something.php', {'id':18,'foo':'hello'}, null, 'jsonp' );

I'm just saying that we add that administrative option to the module. Was hoping someone would roll a patch with that.

andre

nickvidal’s picture

FileSize
1.05 KB

That would make things just more confusing. Indeed I misread your comment because I didn't even think that someone would suggest this. The 'method' and 'callback' are pretty standard names. That solves issue (c).

As for issue (a), again: the client can consume pure JSON simply by not setting the callback.

As for issue (b), it really doesn't matter...

Please look at the simple changes to the json_server.module attached.

yajnin’s picture

What was the outcome of this discussion? +1 JSON/P

yajnin’s picture

Category: feature » bug

Deleted my comment as it didn't make sense. Not even to me a few weeks later :P

yajnin’s picture

JSONP + JSON Server Reference Information
====================================

Please refer to other discussions and JSONP examples:

Updates on this topic
++++++++++++++++

See Issues threads for further discussion regarding inclusion of JSONP in JSON Server

- - > http://drupal.org/node/624898
- - > http://drupal.org/node/598358

+

See tips for getting JSONP working with JSON Server 6.x-2.0-alpha1 (below)

- - > http://drupal.org/node/624898#comment-2858192

+

See more discussion from people using it (as well as general JavaScript info, as well as MooTools info)

"JavaScript and Services: JSON Server + JSON + JSONP // + MooTools // Discussions"

- - JSONP info begins about half-way down

- - > http://groups.drupal.org/node/24372

+

See an example of using JSONP (with MooTools) for node.save, node.update

http://drupal.org/node/774116

voxpelli’s picture

I'm very much in support of andremolnar on this one - the points made in #2 are all valid and should be fixed in the patch.

It is a potential security issue to have JSONP-support for an API - you need to be more careful with which resources you expose when it's active.

I marked #624898: GET/POST + JSON/JSONP as a duplicate of this issue since they was both dealing with the same issue.

voxpelli’s picture

Category: bug » feature
yajnin’s picture

In my use case, the risk is a non issue as I'm in control of the domains. The need is valid.

Beyond my use case, I completely agree with #2 and #12. It should be optional. And in the defense of those who need it, you shouldn't need to hack for this option.

yajnin’s picture

Category: bug » feature

Hey everyone,

On the topic of the OP, JSONP, I'm sharing information, but with security warnings:

JavaScript - JSON Server & Request.JSONP (MooTools Example)
http://drupal.org/node/791922

If people take the warning seriously, here is where I direct them to:

JSON Server & Request.JSON (MooTools Example)
http://drupal.org/node/522942

// ## General Info, Re Patches

Was just helping out a guy using Flash, and thought I'd plug converting over to JavaScript. I've been throwing down resources for JS, because, well, it kicks ass.

Once I get through the next little bit of work ninjay-side, I'll look at sharing some patches for review. For this module and Services.

In the meantime, I'd appreciate if everyone can start looking at some of these docs I'm writing for Services.

http://drupal.org/node/762088

//

As Apple, Google and Microsoft have all but made Flash look like a long-term loser... I think we JavaScripters can raise Services and Drupal's profile with some easy service wrapper apps.

I've been working with Drupal for the past 5 or so years and on the fringe of services for a year +. I've been pretty focused now for about 6 or 7 months. And these days, it is my everyday.

I'll be releasing some MooTools modules to the wild that will work directly with Services and JSON Server. It'd make life easier for many people wanting Flash-like capability with plain old JavaScript. It's essentially a branded framework, as part of the Drupal community. I would eventually like to release it for jQuery as well. It's been built to make porting it really easy.

In the meantime, if I can get some official support on JSONP, and if I can help in any way, please let me know.

Cheers

yajnin’s picture

Thoughts re Approach, Permissions

// JSON Server


JSON Server Module, Permissions

access content via JSONP

// Perhaps JSONP should become a separate module, under the same dev banner as JSON Server?
// // JSON P Server

// Services
// // In an ideal world this would have top-down support from the Services team
// // and other Services modules


// Using Comment Service as an example

Comment Service Module, Permissions

access comments via local service // json server 
access comments via remote service  // json p server

Thoughts?

sanduhrs’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev
Status: Needs work » Needs review
FileSize
4.41 KB

Based on #624898: GET/POST + JSON/JSONP

Attached is a patch that that in response to #2 implements:
a) Additional settings on admin/build/services/settings to let admin activate JSONP support
b) Validates the callback parameter to be [alnum]
c) Additional settings on admin/build/services/settings to let admin define a name for the callback parameter

Probably needs some more elaboration:
b) Should provide a descriptive error message, instead of falling back to standard JSON.
c) Callback parameter name should be validated

Is this the way to go?

sanduhrs’s picture

Sorry, too fast–see corrected patch.

sanduhrs’s picture

Small update.

skyredwang’s picture

It looks like someone has already forked this project and added JSONP support. http://drupal.org/project/jsonp_server

nicholasThompson’s picture

@skyredwang: they forked it back in April, but there is still no code for it.

Gonna test this patch now...

nicholasThompson’s picture

The patch seems to work very well. Attached is basically the same patch with a minor tweak to the admin form (like making the descriptions more helpful).

I personally think this is RTBC. The idea of whitelisting the callback values sounds like a good "phase 2" addon, but on the other hand I dont see what it would achieve. Requesting the URL (even with a whitelisted callback) will still respond with the data which could then manually be processed. I can't see what it protects you against.

voxpelli’s picture

Status: Needs review » Needs work

I haven't checked that the rest of the patch is okay - but I did notice some things that I think should be looked at.

+++ json_server.module	18 Jul 2010 13:48:59 -0000
@@ -13,7 +13,7 @@
+    '#name' => 'JSON/P',

Why the name change?

+++ json_server.module	18 Jul 2010 13:48:59 -0000
@@ -31,42 +31,55 @@ function json_server_server_error($messa
-if (!isset($_POST)) {
-  return drupal_to_js(array('#error' => TRUE, '#data' => "JSON server accepts POST requests only."));
-}

By not requiring POST-requests anymore you're opening up a big XSS-vulnerability when any write operations are allowed to a service.

You need to add a separate choice for whether GET-requests should be allowed or not.

+++ json_server.module	18 Jul 2010 13:48:59 -0000
@@ -31,42 +31,55 @@ function json_server_server_error($messa
+  $request_method = $_REQUEST['method'];

Why is there no longer a drupal_parse_json() here?

+++ json_server.module	18 Jul 2010 13:48:59 -0000
@@ -31,42 +31,55 @@ function json_server_server_error($messa
+  if ($json_server_jsonp && isset($_REQUEST[$callback]) && ctype_alnum($_REQUEST[$callback])) {

If the callback is invalid return a HTTP-error instead of JSON. The clients know how to handle javascript and http-errors but probably not json.

+++ json_server.module	18 Jul 2010 13:48:59 -0000
@@ -102,4 +119,40 @@ function drupal_parse_json($v) {
+    '#description' => t('Enable JSON-P Server. This will wrap the result as a callback, rather than just a JSON object.'),

Add a warning of the possible XSS-vulnerabilities that comes with JSONP - eg. that other sites can read personal data regarding the logged in user if a service that enables that has been activated.

+++ json_server.module	18 Jul 2010 13:48:59 -0000
@@ -102,4 +119,40 @@ function drupal_parse_json($v) {
+    '#title' => t('JSON-P callback'),

It's called JSONP and nothing else. Every other spelling should be replaced with JSONP.

+++ json_server.module	18 Jul 2010 13:48:59 -0000
@@ -102,4 +119,40 @@ function drupal_parse_json($v) {
+  variable_set('json_server_jsonp', $form_state['values']['json_server']['json_server_jsonp']);
+  variable_set('json_server_jsonp_callback', $form_state['values']['json_server']['json_server_jsonp_callback']);

Remove these variables on uninstall.

Powered by Dreditor.

jazzdrive3’s picture

So can key authentication still be used with jsonp? How would this be done?

I assume it's something like what is done here: http://blog.soniccode.com/php/drupal-connected-iphone-application-using-...

But I really don't understand the whole key authentication stuff. Like, why is the nonce just a random number? The rabbit whole goes deep indeed...

Thanks!

primerg’s picture

I am using this module for a jsonp call and I used the patches above but I encountered some error. This patch is adss fixes on those issues and also I tried to add voxpelli's comments.

Attached is the actual modified module and the patch.

primerg

gdd’s picture

I haven't looked at the patch carefully but please keep the security considerations in mind

http://www.metaltoad.com/blog/using-jsonp-safely

primerg’s picture

thanks heyrocker for the note. I'll be update this with the security fix.

thanks.

primerg’s picture

Status: Needs work » Needs review
FileSize
7.38 KB
8.04 KB

Added security fix. Please review.

voxpelli’s picture

Status: Needs review » Needs work

Brief review that's not at all complete.

+++ json_server.module	26 Sep 2010 15:37:00 -0000
@@ -31,42 +31,94 @@
+      return drupal_to_js(array('#error' => TRUE, '#data' => "Callback only accepts alphanumeric characters."));

If JSONP can't return javascript it should return a http-code error code instead - never JSON.

+++ json_server.module	26 Sep 2010 15:37:00 -0000
@@ -31,42 +31,94 @@
+          if ($json_server_jsonp) {
+            drupal_alter('json_data_' . str_replace('.', '_', $request_method), &$data);          ¶
+          }

Why only for JSONP?

+++ json_server.module	26 Sep 2010 15:37:00 -0000
@@ -31,42 +31,94 @@
+    header('Access-Control-Allow-Origin: *');
+    header('Access-Control-Allow-Methods: GET');

This has nothing to do with JSONP - it's an alternate approach to cross-domain JSON calls which would be used instead of JSONP.

Also: You've got some trailing white spaces

Powered by Dreditor.

primerg’s picture

If JSONP can't return javascript it should return a http-code error code instead - never JSON.
-- i figured that might be an issue

Why only for JSONP?
-- never tested in json calls. it can be called for both if needed

This has nothing to do with JSONP - it's an alternate approach to cross-domain JSON calls which would be used instead of JSONP.
-- this was part of numerous testing that i did. but yeah, there is nothing special on those calls in terms of access control

Also: You've got some trailing white spaces
-- anything i should worry about trailing white spaces?

小文’s picture

Issue summary: View changes

in 7.0 services module ,I find this url is ok! like this:http://[yousiteurl]/[yourendpoint]/[yourpath].jsonp?callback=[yourcustomfunction]