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

Avoid infinite redirection #4833

Merged
merged 2 commits into from Nov 1, 2018
Merged

Avoid infinite redirection #4833

merged 2 commits into from Nov 1, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Oct 31, 2018

Closes #4673

@humitos humitos requested a review from Oct 31, 2018
@codecov
Copy link

@codecov codecov bot commented Oct 31, 2018

Codecov Report

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

@@            Coverage Diff            @@
##           master   #4833      +/-   ##
=========================================
+ Coverage    76.2%   76.2%   +<.01%     
=========================================
  Files         158     158              
  Lines       10022   10024       +2     
  Branches     1265    1266       +1     
=========================================
+ Hits         7637    7639       +2     
  Misses       2041    2041              
  Partials      344     344
Impacted Files Coverage Δ
readthedocs/core/views/__init__.py 71.01% <100%> (+0.86%) ⬆️

Copy link
Member

@ericholscher ericholscher left a comment

Looks like a good change. Had some thoughts on the UX around it, but don't have an obvious way forward with it.

@@ -116,7 +116,9 @@ def server_error_404(request, exception=None, template_name='404.html'): # pyli
Marking exception as optional to make /404/ testing page to work.
"""
response = get_redirect_response(request, path=request.get_full_path())
if response:

if response and response.url != request.build_absolute_uri():
Copy link
Member

@ericholscher ericholscher Oct 31, 2018

I wonder if we should add some logging here if there is a loop. It would be best to actually be able to alert the user, because presumably they have misconfigured a redirect and will be confused. Not sure the best course of action tho, because doing a debug log will never be shown in prod :/

Perhaps we should show a different error code in this case, perhaps a 500 level, to show that something is broken vs. just missing?

Copy link
Member Author

@humitos humitos Oct 31, 2018

I wonder if we should add some logging here if there is a loop

👍 on adding a log.warning if this happens.

Perhaps we should show a different error code in this case, perhaps a 500 level, to show that something is broken vs. just missing?

Mmm... Well, for the use case were we detected this, what we want here is a 404 not a 500 actually (you can read the issue that this PR closes for more context).

I suppose that returning a 404 is the first step here to avoid this problem.

Copy link
Member

@ericholscher ericholscher Oct 31, 2018

Yea, better than actually returning an infinite redirect. So I'm +1 on shipping and then improving later.

@humitos humitos merged commit cac4fcc into master Nov 1, 2018
3 checks passed
@humitos humitos deleted the humitos/redirects/avoid-infinite branch Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants