Skip to content

Don't parse html with xml parser #53

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

Closed
liquidpele wants to merge 5 commits into from
Closed

Don't parse html with xml parser #53

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b60149c
Don't parse html with xml parser, and instead get the data we need di…
Apr 11, 2017
d732a3b
Don't use the private method
Apr 11, 2017
43d182e
Remove un-used import, utilize the default option of getattr
Apr 11, 2017
501095a
Merge in my other PR about failing back to supported idp bindings, an…
Apr 12, 2017
2868b31
Somehow deleted the get_location method?
Apr 12, 2017
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
24 changes: 1 addition & 23 deletions djangosaml2/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,11 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from defusedxml import ElementTree
from django.conf import settings


def get_custom_setting(name, default=None):
if hasattr(settings, name):
return getattr(settings, name)
else:
return default
return getattr(settings, name, default)


def available_idps(config, langpref=None):
Expand All @@ -46,21 +42,3 @@ def get_location(http_info):
header_name, header_value = headers[0]
assert header_name == 'Location'
return header_value


def get_hidden_form_inputs(html):
""" Extracts name/value pairs from hidden input tags in an html form."""
pairs = dict()
tree = ElementTree.fromstring(html.replace('&', '&'), forbid_dtd=True)
# python 2.6 doesn't have iter
if hasattr(tree, 'iter'):
node_iter = tree.iter()
else:
node_iter = tree.getiterator()
for node in node_iter:
if node.tag == 'input':
element = dict(node.items())
if element['type'] == 'hidden':
pairs[element['name']] = element['value']
return pairs

27 changes: 14 additions & 13 deletions djangosaml2/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,13 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import base64
import logging

try:
from xml.etree import ElementTree
except ImportError:
from elementtree import ElementTree
from defusedxml.common import (DTDForbidden, EntitiesForbidden,
ExternalReferenceForbidden)

from django.conf import settings
from django.contrib import auth
Expand Down Expand Up @@ -54,8 +53,7 @@ def csrf_exempt(view_func):
from djangosaml2.cache import StateCache
from djangosaml2.conf import get_config
from djangosaml2.signals import post_authenticated
from djangosaml2.utils import get_custom_setting, available_idps, get_location, \
get_hidden_form_inputs
from djangosaml2.utils import get_custom_setting, available_idps, get_location


logger = logging.getLogger('djangosaml2')
Expand Down Expand Up @@ -177,17 +175,20 @@ def login(request,
return HttpResponseRedirect(get_location(result))
elif binding == BINDING_HTTP_POST:
if not post_binding_form_template:
# use the html provided by pysaml2
return HttpResponse(result['data'])
try:
params = get_hidden_form_inputs(result['data'][3])
else:
# manually get request XML to build our own template
request_id, request_xml = client.create_authn_request(
Copy link
Collaborator

@knaperek knaperek Apr 12, 2017

Choose a reason for hiding this comment

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

I don't like doing this :-(.
We already have a result from prepare_for_authenticate, now we should disregard it and ask for a new, just because we can "parse it better"?
I take it that you're trying to fine-tune the code and I appreciate it, but this is ain't broke (it works!) and I prefer stability here. There's really no need to do this in my opinion.

Copy link
Contributor Author

@liquidpele liquidpele Apr 12, 2017

Choose a reason for hiding this comment

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

Oh no, it's still very broken if you have any characters in your IDP SSO url that choke an xml parser. I only fixed the & because the was one we had happen in the field, and I intended it only as a temporary fix until I dug more into pysaml2.

I can change it so it does only one or the other instead of both in that case if you like, but explicitly getting the parameters instead of parsing them from html that pysaml2 generates is going to be far less bug-prone. Do you want me to go ahead and have it only do one or the other?

client.sso_location(selected_idp, binding),
binding=binding)
return render(request, post_binding_form_template, {
'target_url': result['url'],
'params': params,
})
except (DTDForbidden, EntitiesForbidden, ExternalReferenceForbidden):
raise PermissionDenied
except TemplateDoesNotExist:
return HttpResponse(result['data'])
'target_url': result['url'],
'params': {
'SAMLRequest': base64.b64encode(request_xml),
'RelayState': came_from,
},
})
else:
raise NotImplementedError('Unsupported binding: %s', binding)

Expand Down