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.

Sign up for GitHub

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

Jump to bottom

[HotFix] Fix BingPreview Issue [OSF-7581] #6938

Merged
merged 2 commits into from
Mar 10, 2017
Merged

[HotFix] Fix BingPreview Issue [OSF-7581] #6938

merged 2 commits into from
Mar 10, 2017

Conversation

cslzchen
Copy link
Collaborator

@cslzchen cslzchen commented Mar 6, 2017

Note

@icereval
FYI @brianjgeiger

Security related issue. Please refer to the ticket.

Tickets

https://openscience.atlassian.net/browse/OSF-7581

@icereval icereval self-requested a review March 6, 2017 19:47
icereval
icereval suggested changes Mar 6, 2017
View reviewed changes
Copy link
Contributor

@icereval icereval left a comment

Choose a reason for hiding this comment

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

first pass complete

framework/auth/views.py Outdated
@@ -50,6 +50,12 @@ def reset_password_get(auth, uid=None, token=None):
:raises: HTTPError(http.BAD_REQUEST) if verification key for the user is invalid, has expired or was used
"""

# TODO [CAS-10][OSF-7566]: implement long-term fix for URL preview/prefetch
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a function decorator, see https://github.com/CenterForOpenScience/osf.io/blob/master/framework/auth/decorators.py#L17

@cslzchen cslzchen force-pushed the hotfix/confimation-url-preview-tolerance branch from e33cb49 to 78c91f3 Compare March 6, 2017 20:42
icereval
icereval suggested changes Mar 6, 2017
View reviewed changes
Copy link
Contributor

@icereval icereval left a comment

Choose a reason for hiding this comment

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

Add tests please.

@icereval
Copy link
Contributor

icereval commented Mar 7, 2017

@cslzchen, appears this might be failing due to an earlier issue in master, maybe rebase/merge the latest master? e.g. https://travis-ci.org/CenterForOpenScience/osf.io/jobs/206793730#L5994

@cslzchen cslzchen force-pushed the hotfix/confimation-url-preview-tolerance branch from ea3f44e to 9ebba39 Compare March 7, 2017 05:18
@cslzchen
Copy link
Collaborator Author

cslzchen commented Mar 7, 2017

@icereval I do have the latest master for the failed tests. Maybe just a probabilistic failure? I amended the commit to trigger a new run of travis.

@cslzchen cslzchen force-pushed the hotfix/confimation-url-preview-tolerance branch from 9ebba39 to fcc1b13 Compare March 7, 2017 05:56
@cslzchen
Prevent BingPreview from prefetching confirmation URLs:
bc666ec 
    - new user
    - link/create ORCiD user
    - user add email
    - user merge account
    - reset password
    - new user claim contributorship
    - existing user claim contributorship
@cslzchen cslzchen force-pushed the hotfix/confimation-url-preview-tolerance branch 2 times, most recently from 46adef6 to bc666ec Compare March 7, 2017 16:01
@cslzchen
Copy link
Collaborator Author

cslzchen commented Mar 7, 2017

in case the else fails again on COS travis, here is my own travis results where osf and else passes while varnish always fails for master though

cslzchen
cslzchen commented Mar 7, 2017
View reviewed changes
tests/test_views.py
@@ -4701,5 +4702,193 @@ def test_dashboard_institutions(self):
assert_not_equal(dashboard_institutions[0]['id'], self.inst_four._id)
assert_not_equal(dashboard_institutions[0]['id'], self.inst_five._id)


class TestConfirmationViewBlockBingPreview(OsfTestCase):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decide to put all BingPreview blocking tests together in one class TestConfirmationViewBlockBingPreview in test/test_views.py since tests for reset_password_get, confirm_email_get, external_login_confirm_email_get, claim_user_form and claim_user_registered are scattered in different places.

Copy link
Collaborator Author

@cslzchen cslzchen Mar 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

These tests (and the decorator block_bing_preview) will go away or be replaced when long-term fix for URL preview/prefetch is implemented in CAS-10 an OSF-7566.

cslzchen
cslzchen commented Mar 10, 2017
View reviewed changes
framework/auth/decorators.py
@functools.wraps(func)
def wrapped(*args, **kwargs):
user_agent = request.headers.get('User-Agent')
if user_agent and 'BingPreview' in user_agent:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Request may not have the User-Agent header.

framework/auth/decorators.py
def wrapped(*args, **kwargs):
user_agent = request.headers.get('User-Agent')
if user_agent and 'BingPreview' in user_agent:
return HTTPError(httplib.FORBIDDEN)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Return HTTP 403.

@icereval icereval merged commit 31887da into CenterForOpenScience:master Mar 10, 2017
@cslzchen cslzchen deleted the hotfix/confimation-url-preview-tolerance branch July 5, 2017 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@icereval icereval icereval approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cslzchen @icereval