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

Fix allow_origin_pat property to properly parse regex #603

Merged
merged 2 commits into from
Nov 2, 2021

Conversation

havok2063
Copy link
Contributor

This PR fixes a bug with the allow_origin_pat which prevented the Jupyter server from recognizing and parsing regex patterns. Follows from discussions with @mariobuikhuizen on mariobuikhuizen/voila-embed#24. Example traceback of the current broken implementation that this PR fixes is

Traceback (most recent call last):
  File "/Users/bcherinka/anaconda3/envs/havokve/lib/python3.8/site-packages/tornado/http1connection.py", line 273, in _read_message
    delegate.finish()
  File "/Users/bcherinka/anaconda3/envs/havokve/lib/python3.8/site-packages/tornado/routing.py", line 268, in finish
    self.delegate.finish()
  File "/Users/bcherinka/anaconda3/envs/havokve/lib/python3.8/site-packages/tornado/web.py", line 2290, in finish
    self.execute()
  File "/Users/bcherinka/anaconda3/envs/havokve/lib/python3.8/site-packages/tornado/web.py", line 2309, in execute
    self.handler = self.handler_class(
  File "/Users/bcherinka/anaconda3/envs/havokve/lib/python3.8/site-packages/tornado/web.py", line 227, in __init__
    self.clear()
  File "/Users/bcherinka/anaconda3/envs/havokve/lib/python3.8/site-packages/tornado/web.py", line 328, in clear
    self.set_default_headers()
  File "/Users/bcherinka/anaconda3/envs/havokve/lib/python3.8/site-packages/jupyter_server/base/handlers.py", line 300, in set_default_headers
    if origin and self.allow_origin_pat.match(origin):
AttributeError: 'str' object has no attribute 'match'

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

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

Thanks, @havok2063.

Because allow_origin_pat is a configurable unicode trait in the main ServerApp, I think we should preserve its type in this property as a unicode string. Instead, I suggest we replace all instances of allow_origin_pat.match(...) with re.match(allow_origin_pat, ...)

@codecov-commenter
Copy link

Codecov Report

Merging #603 (b82c6fd) into master (fda4cc5) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head b82c6fd differs from pull request most recent head 516f92d. Consider uploading reports for the commit 516f92d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
- Coverage   77.40%   77.35%   -0.06%     
==========================================
  Files         110      110              
  Lines       10251    10252       +1     
  Branches     1258     1258              
==========================================
- Hits         7935     7930       -5     
- Misses       1918     1926       +8     
+ Partials      398      396       -2     
Impacted Files Coverage Δ
jupyter_server/base/handlers.py 64.37% <100.00%> (+0.07%) ⬆️
jupyter_server/services/kernels/kernelmanager.py 79.79% <0.00%> (-2.06%) ⬇️

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 fda4cc5...516f92d. Read the comment docs.

@havok2063
Copy link
Contributor Author

@Zsailer Yeah, no problem. I've made the requested changes and did a few checks that allow_origin_pat works as expected.

@Zsailer
Copy link
Member

Zsailer commented Nov 2, 2021

Thanks, @havok2063!

@Zsailer Zsailer merged commit 3ecb4e0 into jupyter-server:master Nov 2, 2021
@welcome
Copy link

welcome bot commented Nov 2, 2021

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@Zsailer Zsailer added the bug label Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants