Communities

Writing
Writing
Codidact Meta
Codidact Meta
The Great Outdoors
The Great Outdoors
Photography & Video
Photography & Video
Scientific Speculation
Scientific Speculation
Cooking
Cooking
Electrical Engineering
Electrical Engineering
Judaism
Judaism
Languages & Linguistics
Languages & Linguistics
Software Development
Software Development
Mathematics
Mathematics
Christianity
Christianity
Code Golf
Code Golf
Music
Music
Physics
Physics
Linux Systems
Linux Systems
Power Users
Power Users
Tabletop RPGs
Tabletop RPGs
Community Proposals
Community Proposals
tag:snake search within a tag
answers:0 unanswered questions
user:xxxx search by author id
score:0.5 posts with 0.5+ score
"snake oil" exact phrase
votes:4 posts with 4+ votes
created:<1w created < 1 week ago
post_type:xxxx type of post
Search help
Notifications
Mark all as read See all your notifications »
Code Reviews

Welcome to Software Development on Codidact!

Will you help us build our independent community of developers helping developers? We're small and trying to grow. We welcome questions about all aspects of software development, from design to code to QA and more. Got questions? Got answers? Got code you'd like someone to review? Please join us.

Is this HTML sanitizer safe?

+7
−0

I wrote this HTML sanitizer for use in web scraping. The idea is to safely copy content from a site but apply my own style-sheet and remove any unsafe elements. This whitelist approach seems very different from other sanitizers that I have seen. Is there any disadvantage of doing it this way that I should be aware of?

const allowedNodeTypes = [
	'p',
	'span',
	'div',
	'a',
	'img',
	'h1',
	'h2',
	'h3',
	'h4',
	'h5',
	'strong',
	'b',
	'em',
	'pre',
	'blockquote',
	'table',
	'tr',
	'td',
	'tbody',
	'u',
	'br',
	'i',
	'ul',
	'ol',
	'li',
	'figure',
	'code',
	'hr',
	'italic',
	'font',
	'sup',
	'sub'
]
	
const allowedAttributeTypes = [
	'href',
	'src',
	'alt',
	'title'
]
const urlTransformAttributeTypes = [
	'src', 
	'href'
]

function recursivelyCopyContent(elem1, elem2, keep_tags) {
	while (elem2.firstChild){
		elem2.removeChild(elem2.firstChild);
	}
	
	if (keep_tags === undefined) {
		keep_tags = true;
	}
	
	elem1.childNodes.forEach(child=>{
		if (child.nodeType === Node.TEXT_NODE){
			elem2.appendChild(
				document.createTextNode(child.data)
			);
		}
		else if (child.nodeType === Node.ELEMENT_NODE)  {
			let tag = child.tagName.toLowerCase();
			if (allowedNodeTypes.indexOf(tag)>=0){
				let element = document.createElement(keep_tags?tag:'span');
				
				if (keep_tags){
					for (var name of child.attributes){
						if (urlTransformAttributeTypes.indexOf(name.nodeName)>=0){
							element.setAttribute(name.nodeName, new URL(name.value, 'https://target_site.com/folder/'));
						}
						else if (allowedAttributeTypes.indexOf(name.nodeName)>=0){
							element.setAttribute(name.nodeName, name.value);
						} else {
							console.log("not alled attribute", name.nodeName);
						}
					}
				}
				
				recursivelyCopyContent(child, element);
				elem2.appendChild(element);
			} else {
				console.log("Not allowed", tag);
			}
		}
	});
}


let htmlDoc = parser.parseFromString(await page.text(), 'text/html');

recursivelyCopyContent(htmlDoc.getElementsByClassName('article-body')[0],
	document.getElementById('content')
);
History
Why does this post require moderator attention?
You might want to add some details to your flag.
Why should this post be closed?

0 comment threads

1 answer

+5
−0

Is there any disadvantage of doing it this way that I should be aware of?

In general whitelisting is the best way to sanitise, but it does create important error classes, especially missing items. I think that your list of allowedNodeTypes could be better ordered to allow manual checking (e.g. 'strong' and 'b' are together, but 'em' and 'i' are not), and it's not obvious that the omission of 'h6' was intentional.

Is there some HTML spec which adds an <italic> tag?


The idea is to safely copy content from a site but apply my own style-sheet

In that case, shouldn't <font> get special treatment?


				let element = document.createElement(keep_tags?tag:'span');

This seems to alias the name of the DOM class Element - or, at least, my editor's JavaScript highlighting thinks so. Maybe rename it to child2?


					for (var name of child.attributes){

Why name? That looks like something that was written when you expected the iterator to give you names instead of nodes and wasn't corrected when the body of the loop was.


						if (urlTransformAttributeTypes.indexOf(name.nodeName)>=0){
							element.setAttribute(name.nodeName, new URL(name.value, 'https://target_site.com/folder/'));
						}
						else if (allowedAttributeTypes.indexOf(name.nodeName)>=0){

This is the only usage of allowedAttributeTypes, and it's only used if the name wasn't found in urlTransformAttributeTypes. Therefore it's inefficient and confusing to have some names present in both.

History
Why does this post require moderator attention?
You might want to add some details to your flag.

1 comment thread

(2 years later) Thanks! I use `(var name of child.attributes)` because it's the name of the attribute... (1 comment)

Sign up to answer this question »