Repository navigation
[ENG-12178] Fix Mendeley OAuth: handle callback errors, retry Cloudflare-blocked requests - #344
Conversation
4e257dc to
19d8184
Compare
adlius
left a comment
adlius
left a comment
There was a problem hiding this comment.
Some security related issues. Otherwise looks good.
| # https://www.rfc-editor.org/rfc/rfc6749.html#section-4.1.2.1 | ||
| if "error" in request.GET: | ||
| return HttpResponse( | ||
| f"oauth2 authorization failed: {request.GET['error']}" |
There was a problem hiding this comment.
We'd either want to escape this or make the HttpResponse not use HTML to avoid XSS attacks. For example, the user can click a malicious link with <gvdomain>/oauth2/callback/?error=<script>(some malicious script here...)</script> and the browser would execute it on GV's domain.
There was a problem hiding this comment.
Some more questions. To summarize:
- Is the urllib fallback really necessary? Can we apply the user agent string to aiohttp and see if that passes cloudflare check?
- Should we drop
Authorizationheaders on redirects? - Should we limit the scope to only Mendeley if it is the only provider that we currently have issues with?
| """fallback http sending for providers whose CDN challenges aiohttp requests | ||
|
|
||
| Some providers (e.g. Mendeley, behind Cloudflare) answer aiohttp requests with a | ||
| "403 text/html" bot challenge, though the same request sent with stdlib `urllib` gets through. |
There was a problem hiding this comment.
Actually I have another question for the choice of using urllib for making the request: I can't think of any reason why Cloudflare would choose to reject a request from aiohttp but accept a request from urllib.request. It seems that this files add a user agent string GravyValet/1.0 instead of using whatever that is default for aiohttp. Have you tried using the GravyValet/1.0 for aiohttp?
| url = f"{url}{_sep}{urllib.parse.urlencode(list(params))}" | ||
| _request = urllib.request.Request(url, data=_body, method=method, headers=_headers) | ||
| try: | ||
| with urllib.request.urlopen(_request, timeout=_TIMEOUT_SECONDS) as _response: |
There was a problem hiding this comment.
There is another security issue here where it seems that, if the request is redirected, this would send the Authorization header to the redirected url again, which maybe shouldn't happen. Aiohttp drops Authorization header on redirects.
| _response.status == HTTPStatus.UNAUTHORIZED | ||
| and _private.account.credentials_format == CredentialsFormats.OAUTH2 | ||
| self._raise_if_expired_token(_response.status) | ||
| if not urllib_fallback.is_bot_challenge( |
There was a problem hiding this comment.
So the only provider we have issues is Mendeley. This change would apply to all providers if it runs into a bot challenge. Can we limit the scope to only Mendeley?
Ticket
Purpose
Changes
Side Effects
API Documentation
QE Notes
CE Notes