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

Add CORS headers to fix issue with webfonts #169

Merged
merged 2 commits into from
Jan 19, 2018

Conversation

panuta
Copy link
Contributor

@panuta panuta commented Jan 18, 2018

For web browser to be able to load and apply webfonts to web page correctly from different domain, we need to add these Cross-origin Resource Sharing headers to static files served from devtools

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

Can you also add a test that Access-Control-Allow-Origin is being set?

@@ -350,6 +350,10 @@ def _insert_footer(self, response):
response = web.Response(body=_404_msg.encode(), status=404, content_type='text/plain')
status, length = response.status, response.content_length
else:
# Inject CORS headers to allow webfonts to load correctly
response.headers['Access-Control-Allow-Methods'] = 'GET'
Copy link
Member

Choose a reason for hiding this comment

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

Do we need Access-Control-Allow-Methods? If so should we allow more methods?

Copy link
Member

Choose a reason for hiding this comment

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

I believe this isn't required.

@samuelcolvin
Copy link
Member

I think setting Access-Control-Allow-Origin for all files types/extensions is fine.

@samuelcolvin
Copy link
Member

Needs #165 to be merged before this will pass.

@samuelcolvin
Copy link
Member

please rebase or merge to current master.

For web browser to be able to load and apply webfonts to web page correctly from different domain, we need to add these Cross-origin Resource Sharing headers to static files served from devtools
@codecov
Copy link

codecov bot commented Jan 19, 2018

Codecov Report

Merging #169 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #169      +/-   ##
=========================================
+ Coverage   94.89%   94.9%   +<.01%     
=========================================
  Files          12      12              
  Lines         764     765       +1     
  Branches       94      94              
=========================================
+ Hits          725     726       +1     
  Misses         25      25              
  Partials       14      14
Impacted Files Coverage Δ
aiohttp_devtools/runserver/serve.py 94.29% <100%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd1ea6a...28bfb21. Read the comment docs.

@panuta
Copy link
Contributor Author

panuta commented Jan 19, 2018

@samuelcolvin

  • I've removed http method check and added a test for this header.
  • I've also rebase with latest commit from master and squash all my commits into one.

I'm not sure why Travis CI failed, though.

@asvetlov
Copy link
Member

Reuse aiohttp-cors maybe?

@panuta
Copy link
Contributor Author

panuta commented Jan 19, 2018

@asvetlov I think if we only want to add a single header, adding aiohttp-cors would be a bit overkill? However, if we want devtools users to be able to control how CORS headers was set for static files serving from devtools, leveraging aiohttp-cors is a good idea. Just my opinion.

@samuelcolvin
Copy link
Member

I agree with @panuta people recent extra dependencies for devtools, seems unnecessary for one line.

@samuelcolvin
Copy link
Member

@panuta looks good, but can you add a tests to confirm that Access-Control-Allow-Origin is present.

@asvetlov
Copy link
Member

One extra dependency is not a problem, aiohttp-devtools already depends on many libs. pip does it work perfectly fine.
I like to encourage good practices. Proper CORS support is not hard task for non-trivial cases, teaching library readers to aiohttp-cors is good thing I believe.

But it's very minor issue, I agree with any your decision -- you are aiohttp-devtools author :)

@panuta
Copy link
Contributor Author

panuta commented Jan 19, 2018

@samuelcolvin I've added the test.

@samuelcolvin samuelcolvin merged commit 4423406 into aio-libs:master Jan 19, 2018
@samuelcolvin
Copy link
Member

Great thanks very much. I'll wait a couple of days to make sure we don't get any errors on v0.7 then release a patch with this in.

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.

None yet

3 participants