-
-
Notifications
You must be signed in to change notification settings - Fork 678
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. |
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 |
Encoding or Sanitizing untrusted content can make them safe for innerHTML. Or when untrusted content has been validated to numerics only, that would be safe to drop into innerHTML. Or maybe content was validated to just letters and numbers of a certain size, that would be safe to drop into innerHTML. And we consider valid data to still often be unsafe. I would say the same for encoded data (like in the case of a HTML encoded URL still executing in a href attribute) or even sanitized data (if dropped into the wrong area of your document). So the point is, I still consider validated, sanitized and encoded data to sometimes still be untrusted. Or at least unsafe. So how about this small addition?
|
So my perspective on that was that if we have already applied relevant strict validation or sanitization then I don't see it as untrusted and therefore I feel like your addition is confusing and refers to a case which should be covered by one of the other related requirements such as:
I would propose the following clarification: "[ADDED, SPLIT FROM 5.3.3] Verify that functions (such as createTextNode, textContent) which safely render content as text, are used to apply unsanitized 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 for unsanitized content." |
From #2468 (comment)
This is the exact reason, why we move input validation away from output encoding requirements. To send a clear message, that those are separate topics. My comment (#2476 (comment)) in other thread:
Building an output must not rely on business logic validation rules, as those may change over time. You may argue, that some id-value does not change to text, but the concept stays. Also, there is no reason to send number values to innerHTML.
No, this proposal gives a wrong signal, like sanitization is an alternative to use instead of innerHTML. It is not. The scope for the requirement is only to show correctly the content as is and must only focus on that. I think the requirement should contain the goal "to display the content as is" to not have discussion we have here. Please keep all the sanitization topics away from here, which is out of scope for this requirement. |
I like Josh’s proposal. It directly addresses my concerns.
No, this proposal gives a wrong signal, like sanitization is an alternative to use instead of innerHTML. It is not.
Sanitization is not an “alternative” that is the wrong word and no one said that.
Sanitization is *necessary* to use innerHTML safely. Sanitization *allows* you to use innerHTML safely.
This is so critical to web development that it’s being discussed as a native browser feature. https://github.com/WICG/sanitizer-api/
This is exactly why I like Josh’s proposal to clarify when it’s ok to use these functions and methods. The way the requirement stands it’s implies we must never use them and I’d like clarification here.
|
Let me document and note here that taking one line out of context and going to the opposite with "fight mode on" is just wasting contributors' time and motivation. |
Josh made a minor but effective suggestion that addresses my concern.
Overall, this requirement is strong and reflects a lot of solid work. I really like its general direction—I'm simply asking for a small adjustment for clarity. The concern I mentioned stood out enough that I felt it necessary to raise.
|
@elarlang let's discuss but for the record can you provide an alternative requirement text that you would prefer to help understand the direction you are going in. |
|
Hi @jmanico, I think the gap here is that the requirement is intended for a specific situation where sanitization would not be necessary. Do you think the following rewording makes this clearer: "Verify that, for content which needs to be displayed as text (and not to be rendered as HTML), only functions which safely render content (such as createTextNode, textContent) are used, to prevent unintended execution of HTML or JavaScript." |
“Verify that, for content which needs to be displayed as text (and not to be rendered as HTML), only functions which safely render content (such as createTextNode, textContent) are used, to prevent unintended execution of HTML or JavaScript."
I like this suggestion the best, it’s closest to my original suggestion. A small change:
“Verify that content intended to be displayed as text, rather than rendered as HTML, is handled using safe rendering functions (such as createTextNode or textContent) to prevent unintended execution of HTML, JavaScript, CSS or other markup.”
|
I did not like to have the "to JavaScript at the end, if we want to make it more wide open
|
I like it. Small change:
…to prevent unintended execution of content such as HTML or JavaScript.
|
@elarlang can this now be closed? |
Seems that this requirement needs clarification or an example.
Something like:
The text was updated successfully, but these errors were encountered: