Skip to content

JS: make sanitization a "common" technique rather than "important"#4141

Merged
esbena merged 2 commits into
github:mainfrom
esbena:js/clarify-sanitization
Aug 27, 2020
Merged

JS: make sanitization a "common" technique rather than "important"#4141
esbena merged 2 commits into
github:mainfrom
esbena:js/clarify-sanitization

Conversation

@esbena

@esbena esbena commented Aug 26, 2020

Copy link
Copy Markdown
Contributor

It was pointed out that sanitization is bordering on being an anti-pattern, and that we should make it clearer in our documentation that better alternatives exist.
I have therefore changed the "sanitization is important" formulations to "sanitization is common", and added an example of an alternative where appropriate.

asgerf
asgerf previously approved these changes Aug 26, 2020

@asgerf asgerf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like it! 👍

@erik-krogh

Copy link
Copy Markdown
Contributor

You could put the An even safer alternative texts first in the <recommendation>.
(E.g. IncompleteSanitization.qhelp: the first proposal given in <recommendation> is to keep using sanitization).

That would require a bit more rewriting, but I think it could be good to present the safest option first.

@esbena

esbena commented Aug 26, 2020

Copy link
Copy Markdown
Contributor Author

but I think it could be good to present the safest option first.

I think the order is fine. We are generally recommending local fixes for the alerts we produce, not architectural rewrites.

Ping @mchammer01 for a minor doc review

<p>

An even safer alternative is to design the application
such that sanitization isn't needed at all, for instance by using HTML

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll leave it up to the documentation team to decide whether it's preferable to say "isn't" or "is not".

felicitymay
felicitymay previously approved these changes Aug 26, 2020

@felicitymay felicitymay left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for making the help more precise and helpful to users. I've made one suggestion several times, but otherwise the changes LGTM once the tests pass.

Comment thread javascript/ql/src/Security/CWE-116/IncompleteHtmlAttributeSanitization.qhelp Outdated
Comment thread javascript/ql/src/Security/CWE-116/IncompleteSanitization.qhelp Outdated
Comment thread javascript/ql/src/Security/CWE-116/UnsafeHtmlExpansion.qhelp Outdated
Comment thread javascript/ql/src/Security/CWE-843/TypeConfusionThroughParameterTampering.qhelp Outdated
Co-authored-by: Felicity Chapman <felicitymay@github.com>
@esbena esbena dismissed stale reviews from felicitymay and asgerf via d27442e August 26, 2020 18:18
@esbena

esbena commented Aug 26, 2020

Copy link
Copy Markdown
Contributor Author

Thank you all. I have applied @felicitymay's suggestions, and just need a final approval.

@esbena esbena merged commit 67278d9 into github:main Aug 27, 2020
@mchammer01

Copy link
Copy Markdown
Contributor

@esbena - thanks for the ping and sorry for not responding (I was away yesterday.)
Thanks Felicity for looking at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants