Skip to content

Issue #275: Updated search to decode HTML strings before displaying#279

Open
ed-parry wants to merge 2 commits into
umbraco:mainfrom
ed-parry:html-encoding
Open

Issue #275: Updated search to decode HTML strings before displaying#279
ed-parry wants to merge 2 commits into
umbraco:mainfrom
ed-parry:html-encoding

Conversation

@ed-parry

Copy link
Copy Markdown
Contributor

Fixes issue #275 - I think the logic was wrong here - it should encode the strings on the way up, and decode them on the way down? So I've switched it over when rendering the search results, and removed the escaping step from the AJAX search drop-down as well, but shout if I'm miles off the mark here - assuming the text is encoded/escaped before being stored, so in the AJAX case we shouldn't need to escape again (and removing it allows the necessary characters to come through okay.

@ed-parry

Copy link
Copy Markdown
Contributor Author

@nul800sebastiaan Hey Sebastiaan - looks like a merge is needed here. Just wanted to check if (after resolving the conflicts) this would be good to go?

@nul800sebastiaan

Copy link
Copy Markdown
Member

Hey @ed-parry! I am actually unsure if we should do this, it could lead to people being able to craft some javascript to do nasty things. 🤔

@ed-parry

Copy link
Copy Markdown
Contributor Author

Morning @nul800sebastiaan. Yeah, I know what you mean. My thinking here is that the inputs are being checked and sanitized before going into the examine indexes, so it should be safe, but I'd be lying if I said I was sure. Can take another look to see if there's a better way, or at least test further with JS examples?

@nul800sebastiaan

Copy link
Copy Markdown
Member

Yeah I'm not sure if that is true, we do some sanitizing when people post things to the forum but I have also seen it break some things. I'd hate to break the whole search just because 1 forum post always comes up.

Although, the auto suggest search (see #275) gets the titles correct so maybe you can double check what we do there?

@ed-parry

Copy link
Copy Markdown
Contributor Author

Will do, leave it with me :)

@nul800sebastiaan nul800sebastiaan changed the base branch from master to main July 15, 2020 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants