Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

Calling the plugin breaks special XML fields which use CDATA and contain ampersands #61

Closed
sbraz opened this issue Sep 21, 2020 · 4 comments

Comments

@sbraz
Copy link
Contributor

sbraz commented Sep 21, 2020

Hi,

At the moment, the config XML cannot be written / read as-is because some fields are HTML-escaped before being written into a CDATA tag.

The most noticeable result is that calling this plugin breaks my LDAP config.

I have already opened an issue upstream but I doubt they are going to work on it since it's an edge case.

I also made a PR which was rejected because it breaks existing configs: pfsense/pfsense#4432.

I don't really know how we could work around this issue. I have looked at pfSense's code, specifically xml_set_character_data_handler which calls this cData function and it isn't able to parse double-escaped attributes such as
<ldap_extended_query>memberOf=CN=Some Group,OU=One &amp;amp; Two,DC=blah,DC=local</ldap_extended_query> which is what this module produces.

The problem is that, to pfSense, parsing both of these returns &:

  • <ldap_extended_query>&amp;</ldap_extended_query>
  • <ldap_extended_query><![CDATA[&]]></ldap_extended_query>

Maybe we could process all special fields and enclose them in CDATA. However, we must also call html.escape when reading them. There are many corner cases to be aware of. I think a good test value is because the former will be escaped by Python while the latter will only be escaped by PHP's htmlentities.

@sbraz
Copy link
Contributor Author

sbraz commented Nov 18, 2020

Hi @opoplawski @f-bor, do you think you could look into this? I don't really know what to do about it.

We cannot use this module on our firewalls until either pfSense releases a fix (unlikely) or this module gets patched.

@opoplawski
Copy link
Owner

@sbraz can you test if the PR works for you? Thanks.

@sbraz
Copy link
Contributor Author

sbraz commented Feb 9, 2021

Sorry, I had a bit of a busy Monday and haven't had time to test it yet. Hopefully it will work, I'll try to test this today.

@sbraz
Copy link
Contributor Author

sbraz commented Apr 20, 2021

It seems to work, thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants