Skip to content

Cors configuration#527

Merged
kermitt2 merged 3 commits intomasterfrom
cors-configuration
Jan 7, 2020
Merged

Cors configuration#527
kermitt2 merged 3 commits intomasterfrom
cors-configuration

Conversation

@lfoppiano
Copy link
Copy Markdown
Member

This should resolve #226

@coveralls
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
Member Author

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

@lfoppiano
Copy link
Copy Markdown
Member 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
Copy Markdown
Collaborator

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
Copy Markdown
Member 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