Summary

Fixed redirection to custom bug URL

Review Request #7925 — Created Jan. 30, 2016 and submitted Feb. 19, 2016, 1:23 p.m.

Information

Owner

ssengar

Repository

Review Board

Branch

release-2.5.x

Bugs

4080

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

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.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (84 > 79 characters)	reviewbot	Jan. 30, 2016, 10:39 a.m.
You can just override response.	chipx86	Feb. 7, 2016, 9:20 p.m.
No need for the first parameter. That's the default.	chipx86	Feb. 7, 2016, 9:20 p.m.
Col: 5 E265 block comment should start with '# '	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 70 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 71 E502 the backslash is redundant between brackets	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 32 E231 missing whitespace after ','	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 56 E231 missing whitespace after ','	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 1 W293 blank line contains whitespace	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 47 W291 trailing whitespace	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 29 E127 continuation line over-indented for visual indent	reviewbot	Feb. 7, 2016, 9:29 p.m.
Col: 70 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 7, 2016, 9:33 p.m.
Col: 57 E231 missing whitespace after ','	reviewbot	Feb. 7, 2016, 9:36 p.m.
Col: 25 E128 continuation line under-indented for visual indent	reviewbot	Feb. 7, 2016, 9:35 p.m.
Col: 65 W291 trailing whitespace	reviewbot	Feb. 7, 2016, 9:36 p.m.
This test does not test non-HTTP schemes.	brennie	Feb. 9, 2016, 12:22 a.m.
This should be testing non-HTTP schemes, so the docstring should be updated.	brennie	Feb. 9, 2016, 12:22 a.m.
Col: 70 E251 unexpected spaces around keyword / parameter equals	reviewbot	Feb. 7, 2016, 9:36 p.m.
Format as: self.create_repository( public=True, bug_tracker='scheme://bugid=%s') or similar if that goes over line length.	brennie	Feb. 9, 2016, 12:22 a.m.
Single quotes for strings.	brennie	Feb. 9, 2016, 12:22 a.m.
Single quotes for strings.	brennie	Feb. 9, 2016, 12:22 a.m.
Single quotes for strings.	brennie	Feb. 9, 2016, 12:22 a.m.
These should be in alphabetical order.	david	Feb. 18, 2016, 8:17 p.m.
Instead of "custom bug url" how about "a bug tracker that uses a non-standard URL scheme"?	david	Feb. 18, 2016, 8:17 p.m.
This comment doesn't explain why HttpResponseRedirect is inappropriate. Can we mention why we can't use that? (so that someone in …	david	Feb. 18, 2016, 8:17 p.m.
No blank line here.	brennie	Feb. 19, 2016, 10:05 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/reviews/views.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/reviews/views.py

reviewboard/reviews/views.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (84 > 79 characters)
```

Diff:

Revision 2 (+4 -1)

Show changes

reviewboard/reviews/views.py

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.

reviewboard/reviews/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
You can just override response.
```
reviewboard/reviews/views.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for the first parameter. That's the default.
```

Summary:

Fixed easyfix bug number 4080

Fixed redirection to custom bug URL

Description:

~		Redirection to custom Bug URL was causing a 400 Bad Request because HttpResponseRedirect expects a HTTP(s) url scheme. Fixed this by creating a custom HTTPResponse.
	~	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.

Testing Done:

~		Tested my code 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, I created some GitHub issues and stored them as custom Urls. Redirection to Github's issue tracker worked fine too.
	~	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.

Bugs:

4080

Diff:

Revision 3 (+20 -1)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

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)
The issue has been resolved. Show all issues
```
Col: 5
 E265 block comment should start with '# '
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 70
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 71
 E502 the backslash is redundant between brackets
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 32
 E231 missing whitespace after ','
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 56
 E231 missing whitespace after ','
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 1
 W293 blank line contains whitespace
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 47
 W291 trailing whitespace
```
reviewboard/reviews/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 29
 E127 continuation line over-indented for visual indent
```

Diff:

Revision 4 (+22 -1)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

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)
The issue has been dropped. Show all issues
```
Col: 70
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/reviews/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 57
 E231 missing whitespace after ','
```
reviewboard/reviews/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 25
 E128 continuation line under-indented for visual indent
```
reviewboard/reviews/views.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 65
 W291 trailing whitespace
```

Diff:

Revision 5 (+22 -1)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

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)
The issue has been dropped. Show all issues
```
Col: 70
 E251 unexpected spaces around keyword / parameter equals
```

reviewboard/reviews/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This test does not test non-HTTP schemes.
```
reviewboard/reviews/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This should be testing non-HTTP schemes, so the docstring should be updated.
```

reviewboard/reviews/tests.py (Diff revision 5)

The issue has been resolved. Show all issues

Format as:

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


or similar if that goes over line length.

reviewboard/reviews/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Single quotes for strings.
```
reviewboard/reviews/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Single quotes for strings.
```
reviewboard/reviews/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Single quotes for strings.
```

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)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

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)
The issue has been resolved. Show all issues
```
These should be in alphabetical order.
```

reviewboard/reviews/tests.py (Diff revision 6)

The issue has been resolved. Show all issues

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

reviewboard/reviews/views.py (Diff revision 6)

The issue has been resolved. 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?)

Change Summary:

Fixed issues opened by David - importing in right order + comments.

Diff:

Revision 7 (+22 -1)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

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

Ship it!

```
Looks good to me!
```
reviewboard/reviews/tests.py (Diff revision 7)
The issue has been resolved. Show all issues
```
No blank line here.
```

Change Summary:

Removed empty line in tests.py

Diff:

Revision 8 (+21 -1)

Show changes

	reviewboard/reviews/tests.py
	reviewboard/reviews/views.py

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

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.5.x (c44e4a2)