Fixed redirection to custom bug URL
Review Request #7925 — Created Jan. 30, 2016 and submitted
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) |
 reviewbot
|
|
|
You can just override response. |
 chipx86
|
|
|
No need for the first parameter. That's the default. |
chipx86
|
|
|
Col: 5 E265 block comment should start with '# ' |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 9 E265 block comment should start with '# ' |
reviewbot
|
|
|
Col: 70 E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
Col: 71 E502 the backslash is redundant between brackets |
reviewbot
|
|
|
Col: 32 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 56 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 1 W293 blank line contains whitespace |
reviewbot
|
|
|
Col: 9 E265 block comment should start with '# ' |
reviewbot
|
|
|
Col: 47 W291 trailing whitespace |
reviewbot
|
|
|
Col: 29 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
Col: 70 E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
Col: 57 E231 missing whitespace after ',' |
reviewbot
|
|
|
Col: 25 E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
Col: 65 W291 trailing whitespace |
reviewbot
|
|
|
This test does not test non-HTTP schemes. |
 brennie
|
|
|
This should be testing non-HTTP schemes, so the docstring should be updated. |
brennie
|
|
|
Col: 70 E251 unexpected spaces around keyword / parameter equals |
reviewbot
|
|
|
Format as: self.create_repository( public=True, bug_tracker='scheme://bugid=%s') or similar if that goes over line length. |
brennie
|
|
|
Single quotes for strings. |
brennie
|
|
|
Single quotes for strings. |
brennie
|
|
|
Single quotes for strings. |
brennie
|
|
|
These should be in alphabetical order. |
 david
|
|
|
Instead of "custom bug url" how about "a bug tracker that uses a non-standard URL scheme"? |
david
|
|
|
This comment doesn't explain why HttpResponseRedirect is inappropriate. Can we mention why we can't use that? (so that someone in … |
david
|
|
|
No blank line here. |
brennie
|
-
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py
-
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.
-
-
SS
Summary: |
|
||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||
Testing Done: |
|
||||||||||||
Bugs: |
|
||||||||||||
Diff: |
Revision 3 (+20 -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 -
-
-
-
reviewboard/reviews/tests.py (Diff revision 3) Col: 70 E251 unexpected spaces around keyword / parameter equals
-
reviewboard/reviews/tests.py (Diff revision 3) Col: 71 E502 the backslash is redundant between brackets
-
-
-
-
-
-
reviewboard/reviews/tests.py (Diff revision 3) Col: 29 E127 continuation line over-indented for visual indent
-
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 -
reviewboard/reviews/tests.py (Diff revision 4) Col: 70 E251 unexpected spaces around keyword / parameter equals
-
-
reviewboard/reviews/tests.py (Diff revision 4) Col: 25 E128 continuation line under-indented for visual indent
-
-
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 -
reviewboard/reviews/tests.py (Diff revision 5) Col: 70 E251 unexpected spaces around keyword / parameter equals
-
-
-
reviewboard/reviews/tests.py (Diff revision 5) This should be testing non-HTTP schemes, so the docstring should be updated.
-
reviewboard/reviews/tests.py (Diff revision 5) Format as:
self.create_repository( public=True, bug_tracker='scheme://bugid=%s')or similar if that goes over line length.
-
-
-
SS
Change Summary:
Fixed issues brought up by Barret i.e. tested whether bug url loads correctly for non-HTTP schemes
Diff: |
Revision 6 (+21 -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
-
-
-
reviewboard/reviews/tests.py (Diff revision 6) Instead of "custom bug url" how about "a bug tracker that uses a non-standard URL scheme"?
-
reviewboard/reviews/views.py (Diff revision 6) This comment doesn't explain why
HttpResponseRedirectis 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?)
SS
Change Summary:
Fixed issues opened by David - importing in right order + comments.
Diff: |
Revision 7 (+22 -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
Tool: Pyflakes Processed Files: reviewboard/reviews/views.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/views.py