Fixed redirection to custom bug URL

Review Request #7925 — Created Jan. 30, 2016 and submitted

Information

Review Board
release-2.5.x

Reviewers

If a repository was configured to have a custom bug tracker URL like custom:some/path/test?id=%s (for example, if we wanted to point to an application that used a custom protocol for URLs), and a review for that repository was created with an associated bug, clicking the bugID link was causing a 400 Bad Request. This was happening because the code, as implemented, used HttpResponseRedirect for this redirection, which expects a HTTP(s) url scheme. So, redirection to a non-HTTP url scheme led to a DisallowedRedirect: Unsafe redirect to URL with protocol 'custom'.

This problem was fixed this by creating a custom HTTPResponse, which allows redirection, irrespective of url scheme.

I added a new unit test, which passed (along with all the existing units tests).

I tested my code manually also by creating a couple of custom bug Urls, which Google Chrome successfully redirected to. The first one was custom:some/path/test?id=%s (as suggested in the bug description). For the next few tests, I configured a GitHub repository on ReviewBoard to have a custom bug tracker URL. On GitHub, I created some issues and tried redirecting to them by clicking the bugID link from ReviewBoard. Redirection to Github's issue tracker worked fine too.

Description From Last Updated

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

You can just override response.

chipx86chipx86

No need for the first parameter. That's the default.

chipx86chipx86

Col: 5 E265 block comment should start with '# '

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 70 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 71 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 32 E231 missing whitespace after ','

reviewbotreviewbot

Col: 56 E231 missing whitespace after ','

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 9 E265 block comment should start with '# '

reviewbotreviewbot

Col: 47 W291 trailing whitespace

reviewbotreviewbot

Col: 29 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 70 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 57 E231 missing whitespace after ','

reviewbotreviewbot

Col: 25 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 65 W291 trailing whitespace

reviewbotreviewbot

This test does not test non-HTTP schemes.

brenniebrennie

This should be testing non-HTTP schemes, so the docstring should be updated.

brenniebrennie

Col: 70 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Format as: self.create_repository( public=True, bug_tracker='scheme://bugid=%s') or similar if that goes over line length.

brenniebrennie

Single quotes for strings.

brenniebrennie

Single quotes for strings.

brenniebrennie

Single quotes for strings.

brenniebrennie

These should be in alphabetical order.

daviddavid

Instead of "custom bug url" how about "a bug tracker that uses a non-standard URL scheme"?

daviddavid

This comment doesn't explain why HttpResponseRedirect is inappropriate. Can we mention why we can't use that? (so that someone in …

daviddavid

No blank line here.

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (84 > 79 characters)
    
  3. 
      
SS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
    
    
  2. 
      
chipx86
  1. The description isn't telling me why this change is needed (and the summary tells me nothing without forcing me to look up the bug). Make sure to follow the guidelines outlined here:

    https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

    This should describe the use cases for why this needs to be fixed as part of it. (It seems that use case is that there may be a custom protocol for URLs that point to an application, and that should be allowed.)

    Also make sure the bug field is filled out with the number.

    This also needs unit tests to test this behavior, plus a comment above this block of code explaining (like the description) why this custom redirect behavior is necessary.

  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    You can just override response.

  3. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Show all issues

    No need for the first parameter. That's the default.

  4. 
      
SS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
  2. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 5
     E265 block comment should start with '# '
    
  3. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  5. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 70
     E251 unexpected spaces around keyword / parameter equals
    
  6. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 71
     E502 the backslash is redundant between brackets
    
  7. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 32
     E231 missing whitespace after ','
    
  8. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 56
     E231 missing whitespace after ','
    
  9. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 1
     W293 blank line contains whitespace
    
  10. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 9
     E265 block comment should start with '# '
    
  11. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 47
     W291 trailing whitespace
    
  12. reviewboard/reviews/tests.py (Diff revision 3)
     
     
    Show all issues
    Col: 29
     E127 continuation line over-indented for visual indent
    
  13. 
      
SS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
  2. reviewboard/reviews/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 70
     E251 unexpected spaces around keyword / parameter equals
    
  3. reviewboard/reviews/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 57
     E231 missing whitespace after ','
    
  4. reviewboard/reviews/tests.py (Diff revision 4)
     
     
    Show all issues
    Col: 25
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/reviews/views.py (Diff revision 4)
     
     
    Show all issues
    Col: 65
     W291 trailing whitespace
    
  6. 
      
SS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
  2. reviewboard/reviews/tests.py (Diff revision 5)
     
     
    Show all issues
    Col: 70
     E251 unexpected spaces around keyword / parameter equals
    
  3. 
      
brennie
  1. 
      
  2. reviewboard/reviews/tests.py (Diff revision 5)
     
     
    Show all issues

    This test does not test non-HTTP schemes.

  3. reviewboard/reviews/tests.py (Diff revision 5)
     
     
    Show all issues

    This should be testing non-HTTP schemes, so the docstring should be updated.

  4. reviewboard/reviews/tests.py (Diff revision 5)
     
     
     
    Show all issues

    Format as:

    self.create_repository(
        public=True, bug_tracker='scheme://bugid=%s')
    

    or similar if that goes over line length.

  5. reviewboard/reviews/tests.py (Diff revision 5)
     
     
    Show all issues

    Single quotes for strings.

  6. reviewboard/reviews/tests.py (Diff revision 5)
     
     
    Show all issues

    Single quotes for strings.

  7. reviewboard/reviews/tests.py (Diff revision 5)
     
     
    Show all issues

    Single quotes for strings.

  8. 
      
SS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/tests.py (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    These should be in alphabetical order.

  3. reviewboard/reviews/tests.py (Diff revision 6)
     
     
    Show all issues

    Instead of "custom bug url" how about "a bug tracker that uses a non-standard URL scheme"?

  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
     
    Show all issues

    This comment doesn't explain why HttpResponseRedirect is inappropriate. Can we mention why we can't use that? (so that someone in the future doesn't try to "clean up" the code by switching back to it?)

  5. 
      
SS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
  2. 
      
brennie
  1. Looks good to me!

  2. reviewboard/reviews/tests.py (Diff revision 7)
     
     
    Show all issues

    No blank line here.

  3. 
      
SS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests.py
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
SS
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (c44e4a2)