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.

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

Jump to bottom

Xml entity escape #523

Merged
merged 5 commits into from
Feb 23, 2024
Merged

Xml entity escape #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9cc8590
fix escaping of character entities in XML
Munawwar Jul 13, 2023
95a6578
linting
Munawwar Jul 13, 2023
040860e
tests
Munawwar Jul 13, 2023
eea8501
fix entity escaping to only escape tag attributes
Munawwar Aug 16, 2023
4db9042
use xml-escape npm module
Munawwar Feb 15, 2024
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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"uuid": "^8.3.2",
"xml": "^1.0.1",
"xml-crypto": "^3.0.1",
"xml-escape": "^1.1.0",
"xpath": "^0.0.32"
},
"devDependencies": {
Expand Down
13 changes: 12 additions & 1 deletion src/libsaml.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import * as xmlenc from '@authenio/xml-encryption';
import { extract } from './extractor';
import camelCase from 'camelcase';
import { getContext } from './api';
import xmlEscape from 'xml-escape';

const signatureAlgorithms = algorithms.signature;
const digestAlgorithms = algorithms.digest;
Expand Down Expand Up @@ -240,6 +241,13 @@ const libSaml = () => {
return prefix + camelContent.charAt(0).toUpperCase() + camelContent.slice(1);
}

function escapeTag(text: string): (...args: string[]) => string {
return (match: string, quote?: string) => {
// not having a quote means this interpolation isn't for an attribute, and so does not need escaping
return quote ? `${quote}${xmlEscape(text || '')}` : text;
}
}

return {

createXPath,
Expand All @@ -259,7 +267,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(
Copy link
Owner

Choose a reason for hiding this comment

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

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]));

Copy link
Owner

Choose a reason for hiding this comment

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

we can use https://www.npmjs.com/package/xml-escape

Copy link
Contributor Author

@Munawwar Munawwar Feb 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the change to use xml-escape npm module

Copy link
Contributor Author

@Munawwar Munawwar Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tngan Is this new change ok?

Copy link
Owner

Choose a reason for hiding this comment

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

yes, and in the future we better intercept the proper xml building process instead of using placeholder replacement.

new RegExp(`("?)\\{${t}\\}`, 'g'),
escapeTag(tagValues[t])
);
});
return rawXML;
},
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2430,6 +2430,11 @@ xml-crypto@^3.0.1:
"@xmldom/xmldom" "^0.8.5"
xpath "0.0.32"

xml-escape@^1.1.0:
version "1.1.0"
resolved "https://registry.yarnpkg.com/xml-escape/-/xml-escape-1.1.0.tgz#3904c143fa8eb3a0030ec646d2902a2f1b706c44"
integrity sha512-B/T4sDK8Z6aUh/qNr7mjKAwwncIljFuUP+DO/D5hloYFj+90O88z8Wf7oSucZTHxBAsC1/CTP4rtx/x1Uf72Mg==

xml@^1.0.1:
version "1.0.1"
resolved "https://registry.yarnpkg.com/xml/-/xml-1.0.1.tgz#78ba72020029c5bc87b8a81a3cfcd74b4a2fc1e5"
Expand Down