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

Cors configuration #527

Merged
merged 3 commits into from
Jan 7, 2020
Merged

Cors configuration #527

merged 3 commits into from
Jan 7, 2020

Conversation

lfoppiano
Copy link
Collaborator

This should resolve #226

@coveralls
Copy link

coveralls commented Dec 18, 2019

Coverage Status

Coverage increased (+0.02%) to 37.331% when pulling 8ad022d on cors-configuration into f0457c8 on master.

Copy link
Collaborator

@Aazhar Aazhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok for me

@kermitt2
Copy link
Owner

kermitt2 commented Dec 28, 2019

Just tested it, if I didn't make an error, I don't see the CORS info in the response header:

Current master server response headers:

HTTP/1.1 200 OK
Date: Sat, 28 Dec 2019 15:10:21 GMT
Content-Type: application/xml; charset=UTF-8
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, POST, DELETE, PUT
Transfer-Encoding: chunked

PR server response headers (with CORS default config in grobid-service/config/config.yaml):

HTTP/1.1 200 OK
Date: Sat, 28 Dec 2019 15:07:29 GMT
Content-Type: application/xml; charset=UTF-8
Transfer-Encoding: chunked

@kermitt2 kermitt2 self-requested a review December 28, 2019 15:16
@Aazhar
Copy link
Collaborator

Aazhar commented Dec 29, 2019

Just tested it, if I didn't make an error, I don't see the CORS info in the response header:

Current master server response headers:

HTTP/1.1 200 OK
Date: Sat, 28 Dec 2019 15:10:21 GMT
Content-Type: application/xml; charset=UTF-8
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, POST, DELETE, PUT
Transfer-Encoding: chunked

PR server response headers (with CORS default config in grobid-service/config/config.yaml):

HTTP/1.1 200 OK
Date: Sat, 28 Dec 2019 15:07:29 GMT
Content-Type: application/xml; charset=UTF-8
Transfer-Encoding: chunked

I tried it with ajax/jquery and it is ok ! , I ll do further tests, something weird..

@lfoppiano
Copy link
Collaborator Author

I need to have a look at it again ...

@lfoppiano
Copy link
Collaborator Author

Basically this implementation does not return headers if the Origin doesn't match the allowed origin or it's absent... it seems the proper way to do so, but it's not as I remember this thing...

@kermitt2
Copy link
Owner

kermitt2 commented Jan 7, 2020

ah my bad, I didn't put Origin: http://localhost in the requesting header indeed! I though corsAllowedOrigins: "*" would also match an absent Origin field.

Adding for instance Origin: http://localhost with the default server config works as expected - in the server response header, we have Access-Control-Allow-Origin: http://localhost Access-Control-Allow-Credentials: true.

@kermitt2 kermitt2 merged commit ab36463 into master Jan 7, 2020
@lfoppiano lfoppiano deleted the cors-configuration branch January 7, 2020 23:20
@lfoppiano
Copy link
Collaborator Author

I just added some minor corrections which were stuck in my local branch with 66de8d5

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

Successfully merging this pull request may close these issues.

Parameter for Cross-Origin Resource Sharing
4 participants