-
-
Notifications
You must be signed in to change notification settings - Fork 675
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.
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
clarify 50.6.2 #2468
Comments
ping @randomstuff - I saw "in some other source" that this requirement was confusing for you, does the proposed addon make sense and make it more clear? |
Thanks, I intended to make a pass of the requirements I did not understand at some point and either try to understand them or raise an issue :) The thing I did not understand was "context-aware methods" but with the examples I understand what we are talking about. So that's a clear improvement. (However, maybe we could make this clearer or make explicit that (AFAIU) interpretation of the user-provided HTML may depend on where we are on the DOM. Searching "context-aware JavaScript" does not give much help. I don't have any suggestion for now, however.) |
As a side note, here are functions in JS that are safe besides createTextNode
And anything else can be made safe with DOMPurify (which is also an experimental native browser function)
|
I think we just need to pick one most widespread function as an example to explain the idea behind the requirement. I would not recommend DOMPurify here as this does not display the input as is - it changes the value and is part of sanitization. |
That fine, but I just don't want to imply it's the only one. (And I agree on DOMPurify) Perhaps: Verify that untrusted input must not be applied via innerHTML, outerHTML, insertAdjacentHTML, document.write or other properties or functions that render HTML. Instead, createTextNode, textContent and similar safe functions that do not render HTML and only render content as text should be used. |
removed 2 examples, added few commas, should > must:
|
I like it Elar. One small change suggestion at the end. Verify that untrusted input must not be applied via innerHTML, document.write, or other properties or functions that render HTML. Instead, use createTextNode, textContent, and similar safe functions that do not render HTML and only render content as text. |
This seems to have resulted in a negative requirement. Can I suggest:
|
This went a bit in wrong direction I guess. Probably needs some wordsmithing, but alternative direction:
|
"Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used." How about this? |
|
Yeah content is better
Yeah "Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted content to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used." |
Folks, there are a lot of good use cases for innerHTML and document.write. You just need to make sure if its untrusted data, that you use DOMPurify or the browers upcoming native HTML sanitizer function. Perhaps replace the last sentence: Functions or properties that render content as HTML (such as innerHTML and document.write) must only be used when the content is sanitized using a trusted library like DOMPurify. |
Can you provide a use-case that is not covered by any other requirement? |
The point I am making is that this sentence is to extreme. "Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used." There are many use cases for these functions to be used. To say "must not or even should not" is too extreme. This has nothing to do with other requirements. The "must" in this one is not right, as there are use cases for using these functions safely with sanitizers. |
Why to produce fake news (with out of context explanation part), if the requirement says:
Mentioning DOMpurify is allowing sanitization (changing the user data) instead of displaying it correctly (as the data was meant to be displayed). If there is business need to use sanitization, including DOMPurify or similar, we have requirement 5.2.1 for that:
|
Fake news? Are you struggling with English again? Why don’t you read my comment more carefully before you reply. Maybe AI can help you with the English.
The problem is that this “must” is too aggressive. There are many cases where using these functions on untrusted data to modify the DOM is necessary and this requirement as it stands denies that.
If you need help with English or understanding my comment better let me know off-line and we can zoom or similar over this.
|
Coming from person who complains about "personal attacks"? :)
... and such a irony. |
What’s good for the goose is good for the gander. You dish it out all the time. Don’t cry when you get it right back. 😊
|
So, there is no surprise in the fact that English is not my native language. At the same time, I can not recall any situation, where it has caused any problem to achieve the goal. Coming out with that, it's just a lame. You should get over from the proofed vulns in your solution or not admitting you are not correct. Or whatever disturbs you. If you want to have requirement updated, re-read comments from re-open and maybe few comments before. If you want to have a fight, you'll get it. |
Can we please change “must” to “should” at least?
|
At first I am only suggesting one small edit. Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply untrusted content to an HTML page or the DOM. Properties or functions (such as innerHTML, document.write) that render content as HTML should not be used. But even still, something is missing. As we have discussed, there are two use cases: a) If we want to render data as content—exactly as the user typed it in strictly as text —then I agree, use textContent and similar. I realize for (b), we capture DOMPurify in other requirements. My problem with this requirement as is, is that "used to apply untrusted content to an HTML page or the DOM" does not capture the idea that this applies only when you want to render user input as text, as described in (a). So perhaps: Verify that functions (such as createTextNode, textContent) are used to apply untrusted content to an HTML page or the DOM when content needs to be rendered strictly and safely as text and not as HTML. Properties or functions (such as innerHTML, document.write) that render content as HTML should not be used when content should be rendered strictly as text. Again, we do not always want to render data as text. So to say "never use innerHTML" is a bit too strict. |
So I seem to recall that we discussed the "must" vs "should" thing in a cabin in Bedfordshire :) I think the decision was that as a general rule whilst "must" sounds harsh, it is more technically correct and makes things clearer given that RFC 2119 defines "should" as a recommendation. For this requirement specifically, I am not sure that I understand the following statements @jmanico:
The way I understand it is that if we want to render HTML based on untrusted content, we need to make it safe first which is why we have 5.2.1. If we have done that then in principle it is not longer untrusted and therefore shouldn't really be covered by the requirement we are currently discussing. We could make this clearer with the following small change at the end of the requirement:
Does this satisfy your concern @jmanico ? |
Josh, I think in your version the last "untrusted content" just the duplicates already mentioned "untrusted" and does not give anything extra. I think Jim's version for the last proposal works, but should >must.
Jim, take this comment (#2468 (comment)) as standard how to respond. I think you have comments to regret here.
If you need help with ASVS or understanding, what is the scope, context, requirement, or the role for co-leader, let me know off-line and we can Google Meet over this. |
Elar,
I think you’re still struggling with language and I prefer Zoom over Google Meet. Happy to talk live.
This sentence is still wrong when you look at it by itself.
“Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used for untrusted content.”
This is still not accurate, you only want to avoid these properties and functions when you are not trying to render HTML.
As it stands it says “you must NEVER use these functions on untrusted content” and that is just not true as we have discussed.
I’m looking for a fairly small change that explains when it’s not good to use these functions and leave room for when it’s ok to use these functions. It just not true that we should never use them.
And I consider data that has been run through DOMPurify still be be untrusted. So maybe:
“Properties or functions (such as innerHTML, document.write) that render content as HTML must not be used for untrusted content that is meant to be rendered only as text.”
And what I’m trying to imply is:
… But these functions can be used to render untrusted content safely if the content is sanitized.
|
With all the attempts to clue some "you are struggling with language" to me you are just ridiculous because the sentence is written by Josh :)
As I agreed with your proposal (#2468 (comment)) in my comment before you said that I still struggling - so if I proposed your proposal and "I was struggling with the language", then who actually struggles? What was the actual point to come with the rant? There was no reason, and for sure not any good intent. Oh, and: #2468 (comment) The level of failure on this is getting more and more epic. Know when to stop. Also I agree with the comment by Josh, that kind of concludes the points I previously wrote: #2468 (comment) |
This requirements second sentence is just logically wrong. Period. Regardless of who wrote it.
And I’m not trying to mess with you, this seems like a language issue since we both agree on the details, so I’m suggesting we meet up to discuss since we are going round in circles.
I am only asking for some wordamithing here because sentence two is *wrong*.
|
I queried this statement in a comment above and said the following: "The way I understand it is that if we want to render HTML based on untrusted content, we need to make it safe first which is why we have 5.2.1. If we have done that then in principle it is not longer untrusted and therefore shouldn't really be covered by the requirement we are currently discussing. So I am not sure I understand in what context you would use these functions with untrusted content." Can you respond to this question please @jmanico |
Seems that this requirement needs clarification or an example.
Something like:
The text was updated successfully, but these errors were encountered: