Xml entity escape #523
Xml entity escape #523
Conversation
e.g. & in a url in an attribute should become & in XML
|
@tngan Would you like to review this PR? |
| @@ -259,7 +274,10 @@ const libSaml = () => { | |||
| */ | |||
| replaceTagsByValue(rawXML: string, tagValues: any): string { | |||
| Object.keys(tagValues).forEach(t => { | |||
| rawXML = rawXML.replace(new RegExp(`{${t}}`, 'g'), tagValues[t]); | |||
| rawXML = rawXML.replace( | |||
There was a problem hiding this comment.
I think we can keep the original target as its well documented, and only the interpolated value is escaped.
rawXML = rawXML.replace(new RegExp(`{${t}}`, 'g'), escape(tagValues[t]));There was a problem hiding this comment.
we can use https://www.npmjs.com/package/xml-escape
There was a problem hiding this comment.
We cannot use original regex, because there are templates like <saml:AttributeStatement>{Attributes}</saml:AttributeStatement> (line 159) that accepts XML inside the placeholder. Those shouldn't be escaped. So we need to differentiate between placeholders for XML attributes attr="{val}" vs placeholders for tag content <tag>{content}</tag>.
Now in future if we want to escape content as well, then we can escape all content by default, and use a new placeholder convention to prevent escaping for specific parts (maybe something like mustache/handlebars inspired {{{content}}}). Right now, we don't need this from my tests.
As for xml-escape, I will make that change (though implementation-wise it is identical. Only 5 characters need escaping - https://github.com/miketheprogrammer/xml-escape/blob/master/index.js#L16).
There was a problem hiding this comment.
I have made the change to use xml-escape npm module
There was a problem hiding this comment.
@tngan Is this new change ok?
There was a problem hiding this comment.
yes, and in the future we better intercept the proper xml building process instead of using placeholder replacement.
No description provided.