fix: Ballot return to url via url params rather than session by holloway · Pull Request #7788 · ietf-tools/datatracker

holloway · 2024-08-05T05:02:58Z

This migrates the ballot_edit_return_point from session to a url param, with some additional safety checks to protect against phishing attacks.

Tasks

verify all incoming routes
~~[x] Playwright tests~~

ietf/doc/views_ballot.py

…n to query param

codecov · 2024-08-07T06:18:45Z

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.79%. Comparing base (c7f6bde) to head (0a7605f).
Report is 36 commits behind head on main.

Files	Patch %	Lines
ietf/doc/views_ballot.py	77.27%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #7788    +/-   ##
========================================
  Coverage   88.78%   88.79%            
========================================
  Files         296      303     +7     
  Lines       41320    41429   +109     
========================================
+ Hits        36687    36787   +100     
- Misses       4633     4642     +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jennifer-richards

A trivial style nit, plus a worry about the impact of moving the redirect into the URL. You've guarded against phishing, but I wonder if we need a more aggressive guard there. One thought: we only have a few places to return to; would it be tractable to put parameter less general than the URL in the query parameter so that it can't be exploited?

Also, I think my inline comment was wrong and you are putting an absolute URL in the parameter - if that could be made into a relative one, that'd be somewhat safer.

Finally, I think the playwright tests are failing. Is that something to fix in the code or is that github struggling?

jennifer-richards · 2024-08-07T20:46:37Z

ietf/doc/views_ballot.py


            if send_mail:
-                qstr=""
+                query=dict()


syntax nit: query = dict() (or query = {} is perhaps trivially more efficient); either way, style is to have spaces around the =

jennifer-richards · 2024-08-07T21:33:03Z

ietf/doc/views_ballot.py

+        # offsite links could be phishing attempts so let's reject them all, and require valid Datatracker
+        # routes
+        try:
+            urlresolve(return_to_url)


I wonder if we should instead (or maybe in addition) check that the URL is not absolute? I think that'd prevent the phishing issue.

I wonder if it'd be worth putting an even tighter check here and only redirect to the view that we want. E.g., someone could make a nuisance link that'd cause logout (at least, until we disable log out by GET). I worry that there might be more harmful games possible with this.

check that the URL is not absolute?

Hmm... I thought about checking whether it was a relative vs absolute URL through checking if it started with a / but then there's that protocol-relative URL syntax of //example.com/phish (now considered an antipattern) so I'd have to use a real URL parser to safely detect relative links, and then if I were checking for valid routes that's a subset of relative URLs so it didn't feel necessary.

only redirect to the view that we want

there are currently 5 views that assign request.session['ballot_edit_return_point'] so I'm guessing there are 5 ways into the ballot editing and 5 ways we should redirect out. We could have an allow list of those 5 route patterns.

^^ working on this now

Done. Thoughts?

…er into fix/ballot-return-params

jennifer-richards

I like the approach. I think it is likely to be useful outside this spot so I have some slight reorg suggestions inline.

jennifer-richards · 2024-08-08T15:41:40Z

ietf/doc/return_to_url.py

@@ -0,0 +1,34 @@
+from django.urls import reverse as urlreverse, resolve as urlresolve, Resolver404
+
+def _parse_return_to_path(return_to_path, default_return_to_path, allowed_path_handlers):


This seems like a generally useful method. Maybe rename to something starting with validate_ or similar and move to ietf.utils.http?

jennifer-richards · 2024-08-08T15:47:06Z

ietf/doc/return_to_url.py

+
+    return return_to_path
+
+def parse_ballot_edit_return_point(ballot_edit_return_point: str, doc_name: str, ballot_id: str):


Probably goes in views_ballot.py if you accept the suggestion to relocate the helper method. (I'm not opposed to moving away from mammoth files toward more, smaller files, but I think we should develop a strategy for organizing that)

jennifer-richards · 2024-08-08T15:53:55Z

ietf/doc/test_return_to_url.py

+from .return_to_url import parse_ballot_edit_return_point
+
+
+class ReturnToUrlTests(TestCase):


Now assuming return_to_url.py is empty, make this a class in tests_ballot.py?

jennifer-richards · 2024-08-08T15:58:54Z

ietf/doc/views_ballot.py

 from django.utils.html import escape
 from urllib.parse import urlencode as urllib_urlencode
-
+from .return_to_url import parse_ballot_edit_return_point


Minor, but organizationally we generally split imports into "basic packages", "django stuff", and "datatracker stuff" (i.e., ietf.*), roughly alphabetically in each section. This should then move down into the ietf.* section.

We're also slowly adopting the relative import syntax, so great to use it. If it's in a module that's not already using it, I've usually just followed whatever style is used there, but we should probably convert existing imports from ietf.thisapp.whatever to .whatever when doing so to keep it consistent.

(If you accept my reorg suggestions then that'll be moot here, though)

jennifer-richards

I probably should have caught this on the last round, but the ValueErrors in the validate method will bubble up and, because they aren't handled, cause 500 errors. They should be caught in the views and lead to a 4xx response instead (probably a 400 unless there's a more specific appropriate one).

I'd be inclined to keep the response message simple and just say that the url is not allowed, rather than throwing url path names at the user. If you want to preserve the details you could log them, but I don't know if that's needed.

fix: ietf-tools#7287 ballot return params

dcc9e26

holloway commented Aug 5, 2024

View reviewed changes

ietf/doc/views_ballot.py Outdated Show resolved Hide resolved

fix: Moving Ballot edit position ballot_edit_return_point from sessio…

88d661e

…n to query param

holloway requested a review from jennifer-richards August 7, 2024 05:39

holloway marked this pull request as ready for review August 7, 2024 05:39

Merge branch 'main' into fix/ballot-return-params

a7cb583

jennifer-richards requested changes Aug 7, 2024

View reviewed changes

holloway added 3 commits August 8, 2024 16:56

fix: tests for return_to_path param

5d6d12a

Merge branch 'fix/ballot-return-params' of gh-ietf:holloway/datatrack…

353e9c7

…er into fix/ballot-return-params

chore: removing Playwright tests until we can figure out a plan

0b7817f

jennifer-richards requested changes Aug 8, 2024

View reviewed changes

feat: return_to path utility refactoring

4177021

holloway requested a review from jennifer-richards August 8, 2024 22:51

jennifer-richards requested changes Aug 9, 2024

View reviewed changes

holloway added 2 commits August 9, 2024 15:26

fix: throw HTTP 400 in view rather than bubbling up a 500

23354a9

fix: return http400 rather than raising

0a7605f

jennifer-richards approved these changes Aug 9, 2024

View reviewed changes

rjsparks merged commit ff88981 into ietf-tools:main Aug 9, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2024

		@@ -0,0 +1,34 @@
		from django.urls import reverse as urlreverse, resolve as urlresolve, Resolver404

		def _parse_return_to_path(return_to_path, default_return_to_path, allowed_path_handlers):


		return return_to_path

		def parse_ballot_edit_return_point(ballot_edit_return_point: str, doc_name: str, ballot_id: str):

		from .return_to_url import parse_ballot_edit_return_point


		class ReturnToUrlTests(TestCase):

Conversation

holloway commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jennifer-richards left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

holloway Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jennifer-richards left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jennifer-richards left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

holloway commented Aug 5, 2024 •

edited

Loading

codecov bot commented Aug 7, 2024 •

edited

Loading

holloway Aug 7, 2024 •

edited

Loading