Skip to content
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

Do we really want to require JSONP support? #19

Closed
osma opened this issue Nov 28, 2019 · 8 comments
Closed

Do we really want to require JSONP support? #19

osma opened this issue Nov 28, 2019 · 8 comments
Labels
API design Where we discuss any changes we want to introduce in the API
Milestone

Comments

@osma
Copy link
Contributor

osma commented Nov 28, 2019

As currently written, the spec states that

All HTTP(S) endpoints exposed by the service MUST support JSONP [RFC6902], which enables web-based clients to access the service from a different domain.

Then a note says that OpenRefine 3.2 uses JSONP requests.

I wonder if this is a good idea going forward. JSONP was invented to circumvent CORS restrictions, but it has some problems (e.g. lack of error handling, excessive trust and potential CSRF vulnerabilities) - see this blog post for some details. Nowadays better CORS techniques are well developed and supported in browsers.

I'm currently planning the implementation of the reconciliation API in Annif (see NatLibFi/Annif#338) and having to implement JSONP looks like an extra obstacle. For one thing, Swagger/OpenAPI and the Connexion toolkit don't seem to support it that well - I may be mistaken here but e.g. the built in validation in Connexion probably won't play well with JSONP. These tools are really made for a post-JSONP world.

Of course I understand that OpenRefine itself may have to be changed if it currently still relies on JSONP support in the API endpoints. But I would suggest that the new spec should try to get rid of old baggage instead of cementing it. A first step would be to make CORS headers a MUST requirement (currently a SHOULD) and JSONP something less, perhaps a SHOULD.

@wetneb
Copy link
Member

wetneb commented Nov 28, 2019

I totally agree that moving to CORS is the way forward.

My intention with the current draft of the specs is that we document the protocol as it currently stands. I think this is a useful thing to have because many services have been designed according to those specs. All existing versions of OpenRefine (and its forks, rebranded versions…) only support JSONP at the moment, so if as a service provider you want to support these, it is useful to have this document available (as a prettier document than OpenRefine's wiki, basically). The intention is not to promote this as a "standard": it just documents how things stand.

Moving forward, I think we should make CORS a requirement (and HTTPS too, probably) but but only in a new version of the specs, such that this is properly recorded as a change (since it is a breaking one).

We can totally start working on a new version of the specs which would include this: we just need to figure out how to handle versioning: #7.

@osma
Copy link
Contributor Author

osma commented Nov 28, 2019

Thanks for the quick response @wetneb! I understand the strategy of first documenting the current API and only then making a new, potentially incompatible version. Perhaps ideas for spec changes could have a specific issue label in GitHub, to keep them separate from housekeeping tasks such as most of the currently open issues.

@wetneb
Copy link
Member

wetneb commented Dec 15, 2019

Now that we have the 0.1 and latest directories we could change this in the latest version.
Perhaps it would still be worth mentioning that old services rely on JSONP (since really few of them support CORS according to the testbench), so I would intuitively suggest something like this:

  • Services MUST implement CORS
  • Services SHOULD or MAY implement JSONP

What do you think about that?

@wetneb wetneb added the API design Where we discuss any changes we want to introduce in the API label Dec 22, 2019
@wetneb wetneb added this to the 0.2 milestone Dec 22, 2019
@wetneb
Copy link
Member

wetneb commented Dec 22, 2019

I think migrating to CORS is pretty important so I propose we change this ASAP, i.e. in version 0.2.

@workergnome
Copy link

I agree--CORS as a MUST and JSONP as a MAY seems right. I think that having JSONP as a SHOULD is too strong: we should have it in there for people who want backwards compatibility, but I don't think we want to be endorsing it as a pattern in 2020.

@osma
Copy link
Contributor Author

osma commented Jan 7, 2020

Agree 100% with CORS as MUST and JSONP as MAY. Great work @wetneb on opening issues and implementing CORS for several endpoints already!

@wetneb
Copy link
Member

wetneb commented Jan 7, 2020

If any of you wants to do the corresponding change in the specs just go ahead!

Great work @wetneb on opening issues and implementing CORS for several endpoints already!

Just to make it clear: I am not expecting that we do that for all API changes obviously… but I just like it when we have more green ticks on the testbench :)

@osma
Copy link
Contributor Author

osma commented Jan 11, 2020

If any of you wants to do the corresponding change in the specs just go ahead!

Did that in PR #34, hope I got it right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API design Where we discuss any changes we want to introduce in the API
Projects
None yet
Development

No branches or pull requests

3 participants