Skip to content
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.

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

Block NULL characters in all POST methods #1890

Merged
merged 2 commits into from
Oct 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions funnel/models/account.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,14 @@ class PROFILE_STATE(LabeledEnum): # noqa: N801
class ZBase32Comparator(Comparator[str]): # pylint: disable=abstract-method
"""Comparator to allow lookup by Account.uuid_zbase32."""

def __eq__(self, other: str) -> sa.ColumnElement[bool]: # type: ignore[override]
def __eq__(self, other: object) -> sa.ColumnElement[bool]: # type: ignore[override]
"""Return an expression for column == other."""
return self.__clause_element__() == UUID(bytes=zbase32_decode(other))
try:
return self.__clause_element__() == UUID( # type: ignore[return-value]
bytes=zbase32_decode(str(other))
)
except ValueError: # zbase32 call failed, so it's not a valid string
return sa.false()


class Account(UuidMixin, BaseMixin, Model):
Expand Down
16 changes: 8 additions & 8 deletions funnel/views/api/oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
)
from ...registry import resource_registry
from ...typing import ReturnView
from ...utils import abort_null, make_redirect_url
from ...utils import make_redirect_url
from ..login_session import reload_for_cookies, requires_client_login, requires_login
from .resource import get_userinfo

Expand Down Expand Up @@ -415,20 +415,20 @@ def oauth_token_success(token: AuthToken, **params) -> ReturnView:
def oauth_token() -> ReturnView:
"""Provide token endpoint for OAuth2 server."""
# Always required parameters
grant_type = cast(Optional[str], abort_null(request.form.get('grant_type')))
grant_type = cast(Optional[str], request.form.get('grant_type'))
auth_client = current_auth.auth_client # Provided by @requires_client_login
scope = abort_null(request.form.get('scope', '')).split(' ')
scope = request.form.get('scope', '').split(' ')
# if grant_type == 'authorization_code' (POST)
code = cast(Optional[str], abort_null(request.form.get('code')))
redirect_uri = cast(Optional[str], abort_null(request.form.get('redirect_uri')))
code = cast(Optional[str], request.form.get('code'))
redirect_uri = cast(Optional[str], request.form.get('redirect_uri'))
# if grant_type == 'password' (POST)
username = cast(Optional[str], abort_null(request.form.get('username')))
password = cast(Optional[str], abort_null(request.form.get('password')))
username = cast(Optional[str], request.form.get('username'))
password = cast(Optional[str], request.form.get('password'))
# if grant_type == 'client_credentials'
buid = cast(
Optional[str],
# XXX: Deprecated userid parameter
abort_null(request.form.get('buid') or request.form.get('userid')),
request.form.get('buid') or request.form.get('userid'),
)

# Validations 1: Required parameters
Expand Down
3 changes: 1 addition & 2 deletions funnel/views/api/shortlink.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,12 @@

from ... import app, shortlinkapp
from ...models import Shortlink, db
from ...utils import abort_null
from ..helpers import app_url_for, validate_is_app_url


# Add future hasjobapp route here
@app.route('/api/1/shortlink/create', methods=['POST'])
@requestform(('url', abort_null), ('shorter', getbool), ('name', abort_null))
@requestform('url', ('shorter', getbool), 'name')
def create_shortlink(
url: str | furl, shorter: bool = True, name: str | None = None
) -> tuple[dict[str, str], int]:
Expand Down
3 changes: 1 addition & 2 deletions funnel/views/api/sms_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
)
from ...transports.sms import validate_exotel_token
from ...typing import ReturnView
from ...utils import abort_null


@app.route('/api/1/sms/twilio_event', methods=['POST'])
Expand Down Expand Up @@ -116,7 +115,7 @@ def process_exotel_event(secret_token: str) -> ReturnView:
# If there are too many rejects, then most likely a hack attempt.
statsd.incr('phone_number.event', tags={'engine': 'exotel', 'stage': 'received'})

exotel_to = abort_null(request.form.get('To', ''))
exotel_to = request.form.get('To', '')
if not exotel_to:
return {'status': 'eror', 'error': 'invalid_phone'}, 422
# Exotel sends back 0-prefixed phone numbers, not plus-prefixed intl. numbers
Expand Down
4 changes: 2 additions & 2 deletions funnel/views/contact.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from .. import app
from ..models import ContactExchange, Project, TicketParticipant, db, sa
from ..typing import ReturnRenderWith, ReturnView
from ..utils import abort_null, format_twitter_handle
from ..utils import format_twitter_handle
from .login_session import requires_login


Expand Down Expand Up @@ -141,7 +141,7 @@ def scan(self) -> ReturnView:

@route('scan/connect', endpoint='scan_connect', methods=['POST'])
@requires_login
@requestargs(('puk', abort_null), ('key', abort_null))
@requestargs('puk', 'key')
def connect(self, puk: str, key: str) -> ReturnView:
"""Verify a badge scan and create a contact."""
ticket_participant = TicketParticipant.query.filter_by(puk=puk, key=key).first()
Expand Down
10 changes: 10 additions & 0 deletions funnel/views/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,16 @@ def shortlink(url: str, actor: Account | None = None, shorter: bool = True) -> s
# --- Request/response handlers --------------------------------------------------------


@app.before_request
def no_null_in_form():
"""Disallow NULL characters in any form submit (but don't scan file attachments)."""
if request.method == 'POST':
for values in request.form.listvalues():
for each in values:
if each is not None and '\x00' in each:
abort(400)


@app.after_request
def commit_db_session(response: ResponseType) -> ResponseType:
"""Commit database session at the end of a request if asked to."""
Expand Down
13 changes: 6 additions & 7 deletions funnel/views/label.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
from ..forms import LabelForm, LabelOptionForm
from ..models import Account, Label, Project, db
from ..typing import ReturnRenderWith, ReturnView
from ..utils import abort_null
from .helpers import render_redirect
from .login_session import requires_login, requires_sudo
from .mixins import AccountCheckMixin, ProjectViewMixin
Expand All @@ -29,7 +28,7 @@ class ProjectLabelView(ProjectViewMixin, UrlForView, ModelView):
def labels(self) -> ReturnRenderWith:
form = forms.Form()
if form.validate_on_submit():
namelist = [abort_null(x) for x in request.values.getlist('name')]
namelist = request.values.getlist('name')
for idx, lname in enumerate(namelist, start=1):
lbl = Label.query.filter_by(project=self.obj, name=lname).first()
if lbl is not None:
Expand All @@ -51,8 +50,8 @@ def new_label(self) -> ReturnRenderWith:
# and those values are also available at `form.data`.
# But in case there are options, the option values are in the list
# in the order they appeared on the create form.
titlelist = [abort_null(x) for x in request.values.getlist('title')]
emojilist = [abort_null(x) for x in request.values.getlist('icon_emoji')]
titlelist = request.values.getlist('title')
emojilist = request.values.getlist('icon_emoji')
# first values of both lists belong to the parent label
titlelist.pop(0)
emojilist.pop(0)
Expand Down Expand Up @@ -143,9 +142,9 @@ def edit(self) -> ReturnRenderWith:
return render_redirect(self.obj.project.url_for('labels'))

if form.validate_on_submit():
namelist = [abort_null(x) for x in request.values.getlist('name')]
titlelist = [abort_null(x) for x in request.values.getlist('title')]
emojilist = [abort_null(x) for x in request.values.getlist('icon_emoji')]
namelist = request.values.getlist('name')
titlelist = request.values.getlist('title')
emojilist = request.values.getlist('icon_emoji')

namelist.pop(0)
titlelist.pop(0)
Expand Down
2 changes: 1 addition & 1 deletion funnel/views/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ def login() -> ReturnView:
if request.method == 'GET':
loginmethod = request.cookies.get('login')

formid = abort_null(request.form.get('form.id'))
formid = request.form.get('form.id')
if request.method == 'POST' and formid == 'passwordlogin':
try:
success = loginform.validate()
Expand Down
2 changes: 1 addition & 1 deletion funnel/views/login_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> T | ReturnResponse:
return render_redirect(url_for('change_password'))

elif request.method == 'POST':
formid = abort_null(request.form.get('form.id'))
formid = request.form.get('form.id')
if formid == FORMID_SUDO_OTP:
try:
otp_session = OtpSession.retrieve('sudo')
Expand Down
6 changes: 3 additions & 3 deletions funnel/views/siteadmin.py
Original file line number Diff line number Diff line change
Expand Up @@ -453,8 +453,8 @@ def init_rq_dashboard():
"""Register RQ Dashboard Blueprint if available for import."""
if rq_dashboard is not None:
rq_dashboard.blueprint.before_request(
lambda: None
if current_auth and current_auth.user.is_sysadmin
else abort(403)
lambda: (
None if current_auth and current_auth.user.is_sysadmin else abort(403)
)
)
app.register_blueprint(rq_dashboard.blueprint, url_prefix='/siteadmin/rq')
12 changes: 2 additions & 10 deletions funnel/views/ticket_participant.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,7 @@
from ..proxies import request_wants
from ..signals import user_data_changed, user_registered
from ..typing import ReturnRenderWith, ReturnView
from ..utils import (
abort_null,
format_twitter_handle,
make_qrcode,
mask_email,
split_name,
)
from ..utils import format_twitter_handle, make_qrcode, mask_email, split_name
from .helpers import render_redirect
from .login_session import requires_login
from .mixins import AccountCheckMixin, ProjectViewMixin, TicketEventViewMixin
Expand Down Expand Up @@ -271,9 +265,7 @@ def checkin(self) -> ReturnView:
form = forms.Form()
if form.validate_on_submit():
checked_in = getbool(request.form.get('checkin'))
ticket_participant_ids = [
abort_null(x) for x in request.form.getlist('puuid_b58')
]
ticket_participant_ids = request.form.getlist('puuid_b58')
for ticket_participant_id in ticket_participant_ids:
attendee = TicketEventParticipant.get(self.obj, ticket_participant_id)
attendee.checked_in = checked_in
Expand Down