Skip to content

Use string body instead of Json in compile request #217

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

Merged
merged 1 commit into from
Apr 11, 2021
Merged

Conversation

thomashoneyman
Copy link
Member

This fixes the CORS issue also noticed in #215, but we can merge this until folks have time to review that pull request.

@thomashoneyman thomashoneyman merged commit ace6c19 into master Apr 11, 2021
@thomashoneyman thomashoneyman deleted the fix-cors branch April 11, 2021 20:11
@JordanMartinez
Copy link
Contributor

Does Scotty automatically handle OPTIONS HTTP requests if we specify a GET handler? If not, couldn't this have also been solved by adding a handler for that?

options "/compile" $ do
      Scotty.setHeader "Access-Control-Allow-Origin" "*"
      Scotty.setHeader "Content-Type" "application/json"

@hdgarrood
Copy link
Collaborator

I doubt Scotty automatically handles OPTIONS, but I also don't think we should add a handler for it, because OPTIONS isn't necessary for Try PureScript, because we are only using GET and POST (which are CORS-safelisted methods: https://fetch.spec.whatwg.org/#methods).

There were two problems here. Firstly, the request body was using the wrong format. It was using JSON-encoded string containing PureScript source code, rather than just PureScript source code. See #215 (comment). The only way of addressing this problem is by making the change in this PR.

Secondly, the original problem was obscured because this error surfaced as a CORS error. I suspect this was because we are only returning the Access-Control-Allow-Origin header on success responses right now - we should include that header on all responses, including error responses. I imagine that this is still a problem.

@JordanMartinez
Copy link
Contributor

we are only using GET and POST (which are CORS-safelisted methods

Isn't that for fetch? Affjax is not built on fetch, so does that still apply?

In my own experience, the dev tools network tab showed that an OPTIONS request was sent to scotty before the post request was sent. That's where the CORS issue occurred. When I added the above OPTIONS handler and reverted the change here, that issue no longer occurred and I instead got a 200. However, in my test the server code was also using the original compile code, not the buildMakeActions-based approach done later.

@hdgarrood
Copy link
Collaborator

I think it does still apply, yes - that's where the definition of CORS lives now, as I understand it.

It appears that I was mistaken though; using a CORS-safelisted method is not sufficient to avoid OPTIONS requests. See https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#simple_requests. I suppose the use of a Content-Type of application/json as opposed to text/plain is what was triggering the OPTIONS request.

In any case, I am quite confident that this is the correct fix. The server is not expecting JSON in the request body so we should not send JSON in the request body.

@JordanMartinez
Copy link
Contributor

Ok, thanks for clarifying. I'll drop this point.

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

Successfully merging this pull request may close these issues.

3 participants