New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow periods in jsonp callback function name #455

Merged
merged 1 commit into from Aug 31, 2014

Conversation

Projects
None yet
5 participants
@codfish
Contributor

codfish commented Aug 26, 2014

  • now it plays nice with angularjs jsonp calls
  • Reference: #144 (comment)

rmccue responded with "Interesting; it was actually intentional to leave out . (not _) to avoid the possibility of side effects, but I guess I'm OK with adding it." but nothing has been added yet, at least I couldn't find a branch with the changes.

I am currently using WP-API with an Angular frontend, and I needed to make this fix, so I thought I would contribute it back as well.

allow periods in jsonp callback function name
- now it plays nice with angularjs jsonp calls
- Reference: #144 (comment)

rachelbaker added a commit that referenced this pull request Aug 31, 2014

Merge pull request #455 from codonnell822/jsonp-function-name
Allow periods in jsonp callback function name.

@rachelbaker rachelbaker merged commit fb894bb into WP-API:master Aug 31, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@codfish codfish deleted the codfish:jsonp-function-name branch Sep 2, 2014

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Nov 10, 2014

Member

FWIW, I'm still curious about potential security concerns here. Giving access to . means you could theoretically force a call to whatever function you want. That said, I'm not sure what could actually be exploitable from that, since you'd have to already have control of the client side.

cc @mdawaffe for any thoughts

Member

rmccue commented Nov 10, 2014

FWIW, I'm still curious about potential security concerns here. Giving access to . means you could theoretically force a call to whatever function you want. That said, I'm not sure what could actually be exploitable from that, since you'd have to already have control of the client side.

cc @mdawaffe for any thoughts

@rachelbaker

This comment has been minimized.

Show comment
Hide comment
@rachelbaker

rachelbaker Nov 10, 2014

Member

@rmccue the . is required for Angular, so I am also curious on @mdawaffe's opinion here.

Member

rachelbaker commented Nov 10, 2014

@rmccue the . is required for Angular, so I am also curious on @mdawaffe's opinion here.

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Nov 10, 2014

Member

https://news.ycombinator.com/item?id=809815 has a pretty nice summary of why we have the existing restrictions on JSONP callbacks here.

Really what we're doing if we allow . is allowing a subset of Javascript expressions. We'd then probably want to also allow []'" as well to allow array syntax. Implementing our own Javascript validator isn't something I particularly want to allow.

Being strict is a nice thing for us to have, since it reduces the attack surface significantly. For example, the Reflected File Download exploit is made much harder to use, since you can't use any control characters. I'd prefer us to be as strict as possible, but also don't want to ruin the developer experience unnecessarily.

That said, it doesn't look like Angular is going to change. As noted on angular/angular.js#1551, Angular is treating this as a problem with the server code for not allowing it. (caitp is the 7th most active committer on the project, and a member of the organisation)

Member

rmccue commented Nov 10, 2014

https://news.ycombinator.com/item?id=809815 has a pretty nice summary of why we have the existing restrictions on JSONP callbacks here.

Really what we're doing if we allow . is allowing a subset of Javascript expressions. We'd then probably want to also allow []'" as well to allow array syntax. Implementing our own Javascript validator isn't something I particularly want to allow.

Being strict is a nice thing for us to have, since it reduces the attack surface significantly. For example, the Reflected File Download exploit is made much harder to use, since you can't use any control characters. I'd prefer us to be as strict as possible, but also don't want to ruin the developer experience unnecessarily.

That said, it doesn't look like Angular is going to change. As noted on angular/angular.js#1551, Angular is treating this as a problem with the server code for not allowing it. (caitp is the 7th most active committer on the project, and a member of the organisation)

@scottopolis

This comment has been minimized.

Show comment
Hide comment
@scottopolis

scottopolis Nov 10, 2014

Hey guys, we are using the API extensively for our app builder, Reactor. We really need the "." supported for jsonp in Angular so we can make cross domain data requests. I understand your security concerns, but Angular is pretty widely used, and I think supporting it is a necessity.

scottopolis commented Nov 10, 2014

Hey guys, we are using the API extensively for our app builder, Reactor. We really need the "." supported for jsonp in Angular so we can make cross domain data requests. I understand your security concerns, but Angular is pretty widely used, and I think supporting it is a necessity.

@mdawaffe

This comment has been minimized.

Show comment
Hide comment
@mdawaffe

mdawaffe Nov 11, 2014

I think the . is fine. It's hard to imagine a client that can call JSONP
endpoints but that does not have access to window.

(Out of country at the moment. Very limited internet.)

On Sunday, November 9, 2014, Ryan McCue notifications@github.com wrote:

FWIW, I'm still curious about potential security concerns here. Giving
access to . means you could theoretically force a call to whatever
function you want. That said, I'm not sure what could actually be
exploitable from that, since you'd have to already have control of the
client side.

cc @mdawaffe https://github.com/mdawaffe for any thoughts


Reply to this email directly or view it on GitHub
#455 (comment).

mdawaffe commented Nov 11, 2014

I think the . is fine. It's hard to imagine a client that can call JSONP
endpoints but that does not have access to window.

(Out of country at the moment. Very limited internet.)

On Sunday, November 9, 2014, Ryan McCue notifications@github.com wrote:

FWIW, I'm still curious about potential security concerns here. Giving
access to . means you could theoretically force a call to whatever
function you want. That said, I'm not sure what could actually be
exploitable from that, since you'd have to already have control of the
client side.

cc @mdawaffe https://github.com/mdawaffe for any thoughts


Reply to this email directly or view it on GitHub
#455 (comment).

@rmccue

This comment has been minimized.

Show comment
Hide comment
@rmccue

rmccue Nov 11, 2014

Member

Cool. Given that we require nonces to be able to access any user-based data (which is the main issue with allowing more), I'm OK to allow . as well as [']" if we need to, but ideally no more than that. :)

Member

rmccue commented Nov 11, 2014

Cool. Given that we require nonces to be able to access any user-based data (which is the main issue with allowing more), I'm OK to allow . as well as [']" if we need to, but ideally no more than that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment