search

reconciliation-api / specs

Specifications of the reconciliation API
https://reconciliation-api.github.io/specs/draft/
31 stars 10 forks source link

Do we really want to require JSONP support? #19

Closed osma closed 4 years ago

osma commented 4 years ago

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 https://github.com/NatLibFi/Annif/issues/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

What do you think about that?

wetneb commented 4 years ago

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

workergnome commented 4 years ago

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 commented 4 years ago

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

wetneb commented 4 years ago

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 commented 4 years ago

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?