[2103] fix: Adds author xml escaping#54
Conversation
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
This does appear that it resolves the immediate issue reported by beeware/briefcase#2103; however, two notes:
- We generally don't create a new template variable when the result can be completely derived with filters.
module_nameandapp_nameuse template-pre-processing - but that's just for the default value. They can (and, under normal circumstances, will be) be independently provided as part of the template context. So - preference would be to apply thexml_escapefilter wherever the variable is used, rather than remember that there's a different variable for the xml excaped author name. - While this literally addresses the reported bug with author name, are there any other attributes that can/should be escaped? It would appear that any variable used an attribute or as a value inside a tag should be escaped. Since we're currently looking at escaping, it makes sense to do a comprehensive pass and check every variable insertion to see if it could be problematic.
|
Ok no worries Russell, I'll remove the new template and check for other attributes that could be escaped. :) |
Sorry about the wait, but these points should be resolved now. I did a lot of xml_escapes where I thought would be necessary. Please let me know if I've gone too far when you get a chance :) |
freakboy3742
left a comment
freakboy3742
left a comment
There was a problem hiding this comment.
Looks good; although it's probably a little too safe :-) There's a few tags (flagged inline) that can't contain unsafe XML characters by definition, so they don't need to be protected.
|
|
||
| <Feature Id="DefaultFeature"> | ||
| <ComponentGroupRef Id="{{ cookiecutter.module_name }}_COMPONENTS" /> | ||
| <ComponentGroupRef Id="{{ cookiecutter.module_name|xml_escape }}_COMPONENTS" /> |
There was a problem hiding this comment.
Module name should be XML-safe - it has to be a Python identifier.
| <ComponentGroupRef Id="{{ cookiecutter.module_name|xml_escape }}_COMPONENTS" /> | |
| <ComponentGroupRef Id="{{ cookiecutter.module_name }}_COMPONENTS" /> |
| {% endif -%} | ||
| {% if cookiecutter.author_email -%} | ||
| <Property Id="ARPCONTACT" Value="{{ cookiecutter.author_email }}" /> | ||
| <Property Id="ARPCONTACT" Value="{{ cookiecutter.author_email|xml_escape }}" /> |
There was a problem hiding this comment.
I'm fairly certain email addresses can't contain any XML unsafe characters
| </StandardDirectory> | ||
|
|
||
| <ComponentGroup Id="{{ cookiecutter.module_name }}_COMPONENTS"> | ||
| <ComponentGroup Id="{{ cookiecutter.module_name|xml_escape }}_COMPONENTS"> |
There was a problem hiding this comment.
Module name must be XML safe - it's a Python identifier.
| Id="ProductIcon.{{ document_type_id|xml_escape }}" | ||
| Source="{{ cookiecutter.app_name }}-{{ document_type_id|xml_escape }}.ico" /> | ||
| <ProgId | ||
| Id="{{ cookiecutter.bundle }}.{{ cookiecutter.app_name }}.{{ document_type_id }}" | ||
| Description="{{ document_type.description }}" | ||
| Id="{{ cookiecutter.bundle|xml_escape }}.{{ cookiecutter.app_name|xml_escape }}.{{ document_type_id|xml_escape }}" | ||
| Description="{{ document_type.description|xml_escape }}" | ||
| Icon="ProductIcon.{{ document_type_id }}"> | ||
| <Extension | ||
| Id="{{ document_type.extension }}" | ||
| ContentType="{% if document_type.get('mime_type') %}{{ document_type.mime_type }}{% else %}application/x-{{ cookiecutter.app_name }}-{{ document_type_id }}{% endif %}"> | ||
| Id="{{ document_type.extension|xml_escape }}" | ||
| ContentType="{% if document_type.get('mime_type') %}{{ document_type.mime_type|xml_escape }}{% else %}application/x-{{ cookiecutter.app_name }}-{{ document_type_id }}{% endif %}"> |
There was a problem hiding this comment.
Some of these are a bit enthusiastic in what they're protecting:
- bundle must be a reversed domain name, so it must be XML safe
- app_name is either a python identifier, or an identifier with
_replace with-- so it must be XML safe - A file extension is highly unlikely to be XML safe, as is a document type ID. I guess it doesn't hurt to keep those protected, but it probably isn't needed.
|
I've taken out extra safe escapes you've flagged Russell, hopefully this is good to go now |
|
@caydnn Also - noting that this set of change will need to be backported to the Windows app template as well. |
2 participants
Adds XMLEscape extension to author name usage in {{ cookiecutter.app_name }}.xws
Partially fixes beeware/briefcase#2103
PR Checklist: