Add JSONP helper #2

Closed
wants to merge 1 commit into from

2 participants

@shtirlic

Extracted from my sinatra-jsonp extension

@rkh
Sinatra member

Isn't jsonp obsolete and super insecure?

@shtirlic

It's only way to do crossdomain ineraction and jQuery like it.))
1,454 total downloads now on rubygems for sinatra-jsonp.

@rkh
Sinatra member

A quick google for CORS jquery reveals plenty of plugins and AFAIK you only need those if you want to support IE.

@shtirlic

Sure CORS is the future, but it's not working on IE7,IE6 and Opera => it's for example 17% of our visitors on our company site.

@rkh
Sinatra member

Yes, I'm not saying your plugin is of no use, to the contrary. I'm just wondering if we should include it in sinatra-contrib. And I have not rejected the pull request, I want this to be discussed. After all, it is a major security risk.

@shtirlic

Ok, maybe we can include it not in Common section. For example: rack-contrib have jsonp. Also I can add checks for js to prevent xss as it done in rack-contrib.

@rkh
Sinatra member

Yes, we could do that. Any reason not to use Rack::JSONP, though? Esp. since it is handling some XSS attacks. All you would have to do is returning JSON.

Serg Podtynnyi Add Jsonp helper 910692c
@shtirlic

I just don't like the way they handle callback names. I moved helper to the custom section and updated commit.

@rkh
Sinatra member

Have you considered submitting a pull request to rack-contrib?
What's your take, @ohhgabriel?

@shtirlic

Nope, they use automatic content-type based response and there is no way to set custom callback params and callback name, also I think bad_request idea is wrong way.

@rkh
Sinatra member

Yeah, I meant submitting a patch for callback name/params.
bad_request is part of the XSS prevention. What would you do instead?

I've not used JSONP myself, @mtodd, if you have a minute, a quick review would be great.

@shtirlic

I would simply return JSON just like no callback passed. Also Sinatra checks URI and returns URI::InvalidURIError

@rkh
Sinatra member

But this is not produced by an invalid URI. It's more that you could provide a callback like jquery = myobject; real_callback. The main reason I'm pushing towards the middleware approach is that it plays well with other extensions, like respond_with/respond_to.

@shtirlic

I rewrite tests to rspec and add XSS checks

 describe "should handle properly XSS vulnerability attempts" do
    context "with XSS vulnerability attempts" do
      def request(callback)
        get "/method?#{callback}"
        body.should == '["hello","hi","hallo"]'
      end
    end

   # Fail with URI::InvalidURIError:bad URI(is not URI?): foo<bar>baz()$
   it "should return bad request for callback with invalid characters" do
      request("foo<bar>baz()$")
    end

    # Fail with URI::InvalidURIError: bad URI(is not URI?): foo<script>alert(1)</script>
    it "should return bad request for callbacks with <script> tags" do
      request("foo<script>alert(1)</script>")
    end
   # Pass and return only json
    it "should return json for callbacks with multiple statements" do
      request("foo%3balert(1)//")
    end
  end

2 of 3 test will fail on sinatra without additiional checks.
Also I have no clue how to add helper method to sinatra via rack middleware approach and if I want to use some plugins from sinatra-contrib I have to use rack-contrib for jsonp?

@rkh
Sinatra member

They fail because you don't escape them properly. Sinatra tries to unescape them and thereby raises the exceptions. This is not a security feature. You don't have to use rack-contrib, only if you want to use middleware offered by it. If you want to add a helper, you'll have to do it the other way around: Add the helper to Sinatra and let that helper use the middleware. But with middleware you usually don't need helpers at all.

@shtirlic

Ok, I got your point and you can close this pull request because of it's uselessness. I just realised that Rack::JSONP is OK for 99% of use cases and thanks for your time.))

@rkh rkh closed this Apr 26, 2011
@rkh rkh added a commit that referenced this pull request Aug 18, 2011
@rkh rkh update for travis #2 f6be961
@zzak zzak pushed a commit to zzak/sinatra-contrib that referenced this pull request Jul 22, 2016
@rkh rkh update for travis #2 df1063d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment