LGTM3

On 21/02/2024 07:48, Yoav Weiss (@Shopify) wrote:
LGTM2

On Wednesday, February 21, 2024 at 6:19:50 AM UTC+1 Domenic Denicola wrote:

    LGTM1. I recall these methods getting lots of good design review and
    discussion in the PR, from multiple parties. I'm excited to see them
    ship.

    Thanks Luke for spotting the trusted types interaction, and fixing it!

    On Saturday, February 17, 2024 at 2:15:09 AM UTC+9 Joey Arhar wrote:

         > Is this the relevant explainer (referenced from the PR
        below):
        https://github.com/WICG/sanitizer-api/blob/main/explainer.md
        <https://github.com/WICG/sanitizer-api/blob/main/explainer.md>

        Yes, as far as I know.

         > This seems positive, right?

        Whoops, meant to put positive. I updated the chromestatus.

         > Both of these look like "Shipped/Shipping", per
        https://bit.ly/blink-signals <https://bit.ly/blink-signals>.
        That status is a little odd, because it doesn't look like
        they've actually made it to a stable release, but if I'm reading
        the bug trackers right they're both merged, so they're past "In
        Development".

        Ok, I'll change them to shipped/shipping.

        On Thu, Feb 15, 2024 at 9:35 AM Luke <lwar...@igalia.com
        <mailto:lwar...@igalia.com>> wrote:

            Just to keep everyone up to date, you can disregard my
            remarks above I've landed a patch which addresses the lack
            of trusted types protection, thanks for the quick review Joey.

            Regards,
            Luke

            On Wednesday, February 14, 2024 at 10:49:23 PM UTC Luke wrote:

                Hi,

                In it's current form Chromium's implementation of these
                functions bypasses trusted types protection.

                The below WPT tests cover this behaviour:

                
https://wpt.fyi/results/trusted-types/block-string-assignment-to-ShadowRoot-setHTMLUnsafe.html?label=experimental&label=master&aligned
 
<https://wpt.fyi/results/trusted-types/block-string-assignment-to-ShadowRoot-setHTMLUnsafe.html?label=experimental&label=master&aligned>
                
https://wpt.fyi/results/trusted-types/block-string-assignment-to-Element-setHTMLUnsafe.html?label=experimental&label=master&aligned
 
<https://wpt.fyi/results/trusted-types/block-string-assignment-to-Element-setHTMLUnsafe.html?label=experimental&label=master&aligned>
                
https://wpt.fyi/results/trusted-types/block-string-assignment-to-Document-parseHTMLUnsafe.html?label=experimental&label=master&aligned
 
<https://wpt.fyi/results/trusted-types/block-string-assignment-to-Document-parseHTMLUnsafe.html?label=experimental&label=master&aligned>

                This should be addressed before shipping, else it will
                be an unexpected security regression.

                On Wednesday, February 14, 2024 at 10:23:01 PM UTC
                Vladimir Levin wrote:

                    On Wed, Feb 14, 2024 at 1:53 PM Jeffrey Yasskin
                    <jyas...@chromium.org> wrote:

                        Non-API-owner opinions inline:

                        On Wed, Feb 14, 2024 at 1:42 PM 'Vladimir Levin'
                        via blink-dev <blin...@chromium.org> wrote:

                            I just had some clarifying questions

                            On Wed, Feb 14, 2024 at 1:13 PM Joey Arhar
                            <jar...@chromium.org> wrote:

                                Some additional notes:
                                - This API is tested in the declarative
                                ShadowDOM tests in interop2024, and it
                                is counting against us to not have it
                                enabled by default.
                                - The future sanitization options will
                                be added as an optional second parameter
                                to both methods, so there will not be
                                any compat issues with shipping now.

                                On Wed, Feb 14, 2024 at 1:11 PM Joey
                                Arhar <jar...@chromium.org> wrote:


                                            Contact emails

                                    jar...@chromium.org


                                            Explainer

                                    None


                            Is this the relevant explainer (referenced
                            from the PR below):
                            
https://github.com/WICG/sanitizer-api/blob/main/explainer.md 
<https://github.com/WICG/sanitizer-api/blob/main/explainer.md>



                                            Specification

                                    
https://html.spec.whatwg.org/C/#unsafe-html-parsing-methods 
<https://html.spec.whatwg.org/C/#unsafe-html-parsing-methods>
                                    https://github.com/whatwg/html/pull/9538 
<https://github.com/whatwg/html/pull/9538>


                                            Summary

                                    The setHTMLUnsafe and
                                    parseHTMLUnsafe methods allow
                                    Declarative ShadowDOM to be used
                                    from javascript. In the future, they
                                    may also get new parameters for
                                    sanitization.



                                            Blink component

                                    Blink>HTML
                                    
<https://bugs.chromium.org/p/chromium/issues/list?q=component:Blink%3EHTML>


                                            TAG review

                                    None


                                            TAG review status

                                    Not applicable


                            There seems to be consensus within browser
                            vendors that this is a good idea, but I'm
                            just wondering why you decided against
                            filing TAG here?


                        IMO, either Firefox or Safari folks should have
                        filed a TAG review for this before they merged
                        their patches. Now that they've merged, I think
                        it falls into the "[already specified && already
                        shipped]" exception category
                        
<https://www.chromium.org/blink/guidelines/api-owners/process-exceptions/>, and 
it's probably too fixed to ask the TAG to spend time on it.


                    (also non-api-owner, but responding anyway): if that
                    is in fact shipping then I agree that this should be
                    the exception here, thanks.


                                            Risks



                                            Interoperability and
                                            Compatibility

                                    None



                                    /Gecko/: No signal
                                    (https://bugzilla.mozilla.org/show_bug.cgi?id=1850675 
<https://bugzilla.mozilla.org/show_bug.cgi?id=1850675>) 
https://github.com/whatwg/html/pull/9538#issuecomment-1728947778 
<https://github.com/whatwg/html/pull/9538#issuecomment-1728947778>


This seems positive, right?

                                    /WebKit/: Positive
                                    (https://bugs.webkit.org/show_bug.cgi?id=261143 
<https://bugs.webkit.org/show_bug.cgi?id=261143>)


                            I'm not sure how to read this properly, but
                            is this a positive signal or
                            "shipped/shipping" signal?


                        Both of these look like "Shipped/Shipping", per
                        https://bit.ly/blink-signals
                        <https://bit.ly/blink-signals>. That status is a
                        little odd, because it doesn't look like they've
                        actually made it to a stable release, but if I'm
                        reading the bug trackers right they're both
                        merged, so they're past "In Development".


                    Yeah, that's my thought here too. My understanding
                    is that all of the patches here are merged, but I
                    just wanted to double check in case I'm
                    misunderstanding what those bugs are implying.

                                    /Web developers/: No signals

                                    /Other signals/:


                                            Ergonomics

                                    This API will likely be used in
                                    tandem with Declarative ShadowDOM.
                                    The default usage of this API will
                                    not make it hard for chrome to
                                    maintain good performance.



                                            Activation

                                    It will not be challenging for
                                    developers to use this feature
                                    immediately.



                                            Security

                                    There are no security risks. This
                                    API just does declarative ShadowDOM.
                                    There is an "unsafe" in the name
                                    because there are future plans to
                                    add sanitization options.
                                    https://github.com/WICG/sanitizer-api/issues/185 
<https://github.com/WICG/sanitizer-api/issues/185> 
https://github.com/whatwg/html/issues/8627 <https://github.com/whatwg/html/issues/8627> 
https://github.com/whatwg/html/issues/8759 <https://github.com/whatwg/html/issues/8759>



                                            WebView application risks

                                    Does this intent deprecate or change
                                    behavior of existing APIs, such that
                                    it has potentially high risk for
                                    Android WebView-based applications?

                                    None



                                            Debuggability

                                    This API does not need any special
                                    DevTools features. You can call the
                                    method from the console panel.



                                            Will this feature be
                                            supported on all six Blink
                                            platforms (Windows, Mac,
                                            Linux, ChromeOS, Android,
                                            and Android WebView)?

                                    Yes


                                            Is this feature fully tested
                                            by web-platform-tests
                                            
<https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md>?

                                    Yes


                                            Flag name on chrome://flags

                                    HTMLUnsafeMethods


                                            Finch feature name

                                    HTMLUnsafeMethods


                                            Requires code in //chrome?

                                    False


                                            Estimated milestones

                                    DevTrial on desktop 120

                                    DevTrial on Android 120



                                            Anticipated spec changes

                                    Open questions about a feature may
                                    be a source of future web compat or
                                    interop issues. Please list open
                                    issues (e.g. links to known github
                                    issues in the project for the
                                    feature specification) whose
                                    resolution may introduce web
                                    compat/interop risk (e.g., changing
                                    to naming or structure of the API in
                                    a non-backward-compatible way).

                                    None


                                            Link to entry on the Chrome
                                            Platform Status

                                    https://chromestatus.com/feature/6560361081995264 
<https://chromestatus.com/feature/6560361081995264>

                                    This intent message was generated by
                                    Chrome Platform Status
                                    <https://chromestatus.com/>.

-- You received this message because you
                                are subscribed to the Google Groups
                                "blink-dev" group.
                                To unsubscribe from this group and stop
                                receiving emails from it, send an email
                                to blink-dev+...@chromium.org.
                                To view this discussion on the web visit
                                
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAK6btwJiEbhk_YGbVhuUg0emSJTfT%3D20_1bTDMFJxcH5i9tbMQ%40mail.gmail.com
 
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAK6btwJiEbhk_YGbVhuUg0emSJTfT%3D20_1bTDMFJxcH5i9tbMQ%40mail.gmail.com?utm_medium=email&utm_source=footer>.

-- You received this message because you are
                            subscribed to the Google Groups "blink-dev"
                            group.
                            To unsubscribe from this group and stop
                            receiving emails from it, send an email to
                            blink-dev+...@chromium.org.
                            To view this discussion on the web visit
                            
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADsXd2MH_fZddPf6c_QwhEP5JU767nEy1ck338Cx_HYFsytO4w%40mail.gmail.com
 
<https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CADsXd2MH_fZddPf6c_QwhEP5JU767nEy1ck338Cx_HYFsytO4w%40mail.gmail.com?utm_medium=email&utm_source=footer>.

--
You received this message because you are subscribed to the Google Groups "blink-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscr...@chromium.org <mailto:blink-dev+unsubscr...@chromium.org>. To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/28b97986-2078-4cfb-aae2-58514a35bff6n%40chromium.org <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/28b97986-2078-4cfb-aae2-58514a35bff6n%40chromium.org?utm_medium=email&utm_source=footer>.

--
You received this message because you are subscribed to the Google Groups 
"blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to blink-dev+unsubscr...@chromium.org.
To view this discussion on the web visit 
https://groups.google.com/a/chromium.org/d/msgid/blink-dev/695f6359-ea88-43c9-84ad-669f1743cf56%40igalia.com.

Reply via email to