Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

disabling JSONP from controllers and from expressjs by default #213

Merged
merged 2 commits into from
Oct 21, 2014

Conversation

lirantal
Copy link
Member

disabling JSONP from controllers and commenting out from expressjs configuration, allowing users to enable if they need to

closes issue #208

…nfiguration, allowing users to enable if they need to
@ilanbiala
Copy link
Member

What does JSONP do that will indicate that there is a global user variable?

@lirantal
Copy link
Member Author

@ilanbiala I don't understand your question at all.

@snakamura
Copy link

Basically it looks good to me, but the patch to express/config.js may be somewhat confusing because people would think commenting out the line enable JSONP in all controllers. Actually they need to replace res.json to res.jsonp to use JSONP.

@lirantal
Copy link
Member Author

Of course it's not just about commenting it out but also actually sending the responses a JSONP.
I guess you're right, it might be confusing so I'll go ahead and remove the commented JSONP code entirely.

@ilanbiala
Copy link
Member

@lirantal What does JSONP do differently than JSON? JSON and JSONP both will send the user and make it global, so how does that fix #208? Am I missing something? I'm not too familiar with JSONP.

@lirantal
Copy link
Member Author

@ilanbiala you're incorrectly using terms and this just causes confusion. Neither JSON nor JSONP makes anything 'global'. You're right in thinking that if the response is just JSON then the user data is being returned, as should be, it's an API after all (this is what I assume you mean by 'global'). With that said, JSONP responses makes it possible for websites not under your control to call the that API by embedding a script tag and passing the returned JSON from the API to the website's control which can further be manipulated by callback functions available on the requesting website's. This bypasses the same origin policy which browsers block by default. That's my 2 cents on the matter if the issue is still not clear, please google up JSONP and security issues related to it.

@ilanbiala
Copy link
Member

So why have it enabled in the first place?

@lirantal
Copy link
Member Author

@ilanbiala your questions aren't really leading anywhere... are they? :)

@rschwabco
Copy link
Member

We're removing JSONP guys, it was a bad idea in the first place.

… be enabled for JSONP to be globally enabled which isnt true
@lirantal
Copy link
Member Author

I updated the PR with removing express's JSONP commented code entirely as @snakamura suggested.

rschwabco added a commit that referenced this pull request Oct 21, 2014
disabling JSONP from controllers and from expressjs by default
@rschwabco rschwabco merged commit 158c373 into meanjs:master Oct 21, 2014
@rschwabco
Copy link
Member

I see no problem here. Merging.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants