Skip to content

fix: Ballot return to url via url params rather than session #7788

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.

Sign up for GitHub

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

Merged
rjsparks merged 9 commits into from
Aug 9, 2024
Merged

fix: Ballot return to url via url params rather than session #7788

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dcc9e26
fix: #7287 ballot return params
holloway Aug 5, 2024
88d661e
fix: Moving Ballot edit position ballot_edit_return_point from sessio…
holloway Aug 7, 2024
a7cb583
Merge branch 'main' into fix/ballot-return-params
holloway Aug 7, 2024
5d6d12a
fix: tests for return_to_path param
holloway Aug 8, 2024
353e9c7
Merge branch 'fix/ballot-return-params' of gh-ietf:holloway/datatrack…
holloway Aug 8, 2024
0b7817f
chore: removing Playwright tests until we can figure out a plan
holloway Aug 8, 2024
4177021
feat: return_to path utility refactoring
holloway Aug 8, 2024
23354a9
fix: throw HTTP 400 in view rather than bubbling up a 500
holloway Aug 9, 2024
0a7605f
fix: return http400 rather than raising
holloway Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions ietf/doc/views_ballot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@
from django.shortcuts import render, get_object_or_404, redirect
from django.template.defaultfilters import striptags
from django.template.loader import render_to_string
from django.urls import reverse as urlreverse
from django.urls import reverse as urlreverse, resolve as urlresolve, Resolver404
from django.views.decorators.csrf import csrf_exempt
from django.utils.html import escape
from urllib.parse import urlencode as urllib_urlencode


import debug # pyflakes:ignore

Expand Down Expand Up @@ -185,9 +187,11 @@ def edit_position(request, name, ballot_id):

balloter = login = request.user.person

if 'ballot_edit_return_point' in request.session:
return_to_url = request.session['ballot_edit_return_point']
else:
return_to_url = request.GET.get("ballot_edit_return_point")

if return_to_url is None:
print("*** DEFAULT return_to_url *********** ")
raise ValueError("ballot_edit_return_point is required param")
return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id))

# if we're in the Secretariat, we can select a balloter to act as stand-in for
Expand All @@ -209,9 +213,14 @@ def edit_position(request, name, ballot_id):
save_position(form, doc, ballot, balloter, login, send_mail)

if send_mail:
qstr=""
query=dict()
Copy link
Member

@jennifer-richards jennifer-richards Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if request.GET.get('balloter'):
qstr += "?balloter=%s" % request.GET.get('balloter')
query['balloter'] = request.GET.get('balloter')
if request.GET.get('ballot_edit_return_point'):
query['ballot_edit_return_point'] = request.GET.get('ballot_edit_return_point')
qstr = ""
if len(query) > 0:
qstr = "?" + urllib_urlencode(query, safe='/')
return HttpResponseRedirect(urlreverse('ietf.doc.views_ballot.send_ballot_comment', kwargs=dict(name=doc.name, ballot_id=ballot_id)) + qstr)
elif request.POST.get("Defer") and doc.stream.slug != "irtf":
return redirect('ietf.doc.views_ballot.defer_ballot', name=doc)
Expand Down Expand Up @@ -337,11 +346,24 @@ def send_ballot_comment(request, name, ballot_id):

balloter = request.user.person

if 'ballot_edit_return_point' in request.session:
return_to_url = request.session['ballot_edit_return_point']
else:
return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id))
return_to_url = request.GET.get('ballot_edit_return_point')

if return_to_url is not None:
# we need to ensure return_to_url isn't used for phishing attacks.
# return_to_url is used in HttpResponseRedirect() which could redirect to Datatracker or offsite.
# Eg http://datatracker.ietf.org/?ballot_edit_return_point=https://example.com/phishing-attack
# offsite links could be phishing attempts so let's reject them all, and require valid Datatracker
# routes
try:
urlresolve(return_to_url)
Copy link
Member

@jennifer-richards jennifer-richards Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@holloway holloway Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@holloway holloway Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ working on this now

Copy link
Contributor Author

@holloway holloway Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thoughts?

# if it doesn't throw then it's a valid route
pass
except Resolver404:
raise ValueError(f"Invalid ballot_edit_return_point doesn't match a route: {return_to_url}")

if return_to_url is None:
return_to_url = urlreverse("ietf.doc.views_doc.document_ballot", kwargs=dict(name=doc.name, ballot_id=ballot_id))

if 'HTTP_REFERER' in request.META:
back_url = request.META['HTTP_REFERER']
else:
Expand Down
5 changes: 0 additions & 5 deletions ietf/doc/views_doc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1544,7 +1544,6 @@ def document_ballot(request, name, ballot_id=None):
top = render_document_top(request, doc, ballot_tab, name)

c = document_ballot_content(request, doc, ballot.id, editable=True)
request.session['ballot_edit_return_point'] = request.path_info

return render(request, "doc/document_ballot.html",
dict(doc=doc,
Expand All @@ -1562,8 +1561,6 @@ def document_irsg_ballot(request, name, ballot_id=None):

c = document_ballot_content(request, doc, ballot_id, editable=True)

request.session['ballot_edit_return_point'] = request.path_info

return render(request, "doc/document_ballot.html",
dict(doc=doc,
top=top,
Expand All @@ -1581,8 +1578,6 @@ def document_rsab_ballot(request, name, ballot_id=None):

c = document_ballot_content(request, doc, ballot_id, editable=True)

request.session['ballot_edit_return_point'] = request.path_info

return render(
request,
"doc/document_ballot.html",
Expand Down
3 changes: 1 addition & 2 deletions ietf/iesg/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,6 @@ def agenda(request, date=None):
urlreverse("ietf.iesg.views.telechat_agenda_content_view", kwargs={"section": "minutes"})
))

request.session['ballot_edit_return_point'] = request.path_info
return render(request, "iesg/agenda.html", {
"date": data["date"],
"sections": sorted(data["sections"].items(), key=lambda x:[int(p) for p in x[0].split('.')]),
Expand Down Expand Up @@ -398,7 +397,7 @@ def agenda_documents(request):
"sections": sorted((num, section) for num, section in sections.items()
if "2" <= num < "5")
})
request.session['ballot_edit_return_point'] = request.path_info

return render(request, 'iesg/agenda_documents.html', { 'telechats': telechats })

def past_documents(request):
Expand Down
2 changes: 1 addition & 1 deletion ietf/templates/doc/ballot_popup.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
{% if editable and user|has_role:"Area Director,Secretariat,IRSG Member,RSAB Member" %}
{% if user|can_ballot:doc %}
<a class="btn btn-primary"
href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot_id %}">
href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot_id %}?ballot_edit_return_point={{ request.path|urlencode }}">
Edit position
</a>
{% endif %}
Expand Down
2 changes: 1 addition & 1 deletion ietf/templates/doc/document_ballot_content.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
</a>
{% if user|can_ballot:doc %}
<a class="btn btn-primary"
href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot.pk %}">
href="{% url "ietf.doc.views_ballot.edit_position" name=doc.name ballot_id=ballot.pk %}?ballot_edit_return_point={{ request.path|urlencode }}">
Edit position
</a>
{% endif %}
Expand Down