Correctly support fragment-only URLs in CSS images

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

We need to support https://drafts.csswg.org/css-values/#url-local-url-flag

[Simon Fraser (smfr)]

This was fixed for SVG in https://trac.webkit.org/changeset/249416/webkit, but not for CSS images, and when I unprefix the `mask` property I need to start using CSSImages for mask references.

[Simon Fraser (smfr)]

Currently we lose the knowledge of the URL being a fragment only URL here:

Ref<CSSImageValue> CSSImageValue::valueWithStylesResolved(Style::BuilderState& state)
{
    auto location = makeResolvedURL(reresolvedURL(state.document()));

[Darin Adler]

Given that CSS has special handling for fragment-only URLs, CSSImageValue::reresolvedURL can be changed to leave such URLs untouched.

[Darin Adler]

Maybe; unclear to me, actually.

[Darin Adler]

Before we change anything we need to make sure we have tests with fragment-only URLs in style sheets and in CSS variables in style sheets with different base URLs as well as the ones in documents.

[Simon Fraser (smfr)]

Yeah, I wasn't sure how to test this without doing the mask work first. cssText() already only returns the fragment part.

[Darin Adler]

(In reply to Simon Fraser (smfr) from comment #6)
> cssText() already only returns the fragment part.

Even when called on a value that comes from the computed style?

[Simon Fraser (smfr)]

*** Bug 154551 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Created attachment 437699 [details]
Test

Actually it does seem testable. Attached testcase suggests we do the wrong thing with empty urls and fragment-only urls.

[Simon Fraser (smfr)]

Adding some WPT via https://github.com/web-platform-tests/wpt/pull/30552

[Radar WebKit Bug Importer]

<rdar://problem/83016456>

[Matt Woodrow]

Created attachment 444186 [details]
Patch

[Matt Woodrow]

Created attachment 444204 [details]
Patch

[Darin Adler]

Comment on attachment 444204 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444204&action=review

Thanks for taking this on! We really want to fix this. I don’t think this patch is quite right yet.

review- because the use of String::find here is incorrect.

> Source/WebCore/css/CSSImageValue.cpp:92
> +    if (!m_location.resolvedURL.string().find('#') || m_location.resolvedURL.string().isEmpty())
> +        return m_location.resolvedURL;

Calling find is an inefficient way to to this check. One efficient way to check if something starts with a '#' is to call the startsWith function passing the character '#'. But given that the URL class already parsed the URL there is an even more efficient way to check using URL class functions. However, this call site has a URL but the other call site just has a String.

Also, we don’t want to write this special case explicitly in two different places. We probably want to make a named function to do the check.

In addition, we should encapsulate this as a smarter version of completeURL, I think.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3620
> +            // Image URLs that start with an explicit #, or are empty should be preserved, otherwise
> +            // resolve the complete URL.
> +            if (!string.find('#') || string.isEmpty()) {
> +                return CSSImageValue::create(URL(URL(), string.toAtomString().string()),
> +                    context.isContentOpaque ? LoadedFromOpaqueSource::Yes : LoadedFromOpaqueSource::No);
> +            } else {
> +                return CSSImageValue::create(context.completeURL(string.toAtomString().string()),
> +                    context.isContentOpaque ? LoadedFromOpaqueSource::Yes : LoadedFromOpaqueSource::No);
> +            }

Is this only for image URLs? What about other CSS URLs? I think this logic should probably go in the CSSParserContext::completeURL function.

[Matt Woodrow]

Comment on attachment 444204 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444204&action=review

>> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3620
>> +            }
> 
> Is this only for image URLs? What about other CSS URLs? I think this logic should probably go in the CSSParserContext::completeURL function.

It looks like we already have two (slightly different) implementations of completeURL, in CSSParserContext and Document.

One option would be to add a new method to wtf::URL (static construction helper) which handles this specific logic, and use that from both completeURL imps, though I'm unsure if WTF is an appropriate place for css-values-specific logic.

Having a separate helper method that we call from both completeURL impls would also work, though I'm not sure where the best spot for that to live is.

[Matt Woodrow]

Created attachment 444322 [details]
Patch

[Darin Adler]

Comment on attachment 444322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444322&action=review

> Source/WebCore/dom/Document.cpp:5606
> +    // https://www.w3.org/TR/css-values-4/#local-urls
> +    // Empty URLs and fragment-only URLs should not be resolved relative to the base
> +    // URL.
> +    if (url.isEmpty() || url.startsWith('#'))
> +        return URL(URL(), url);

I think you are correct to say that it may be incorrect to have this here. We’ll need to see which regression tests fail.

Instead we may want to change CSSImageValue to use another function specific to CSS that adds this behavior and then calls through to Document::completeURL. I might put it in CSSParserContext as a static member function that takes a document, or somewhere else that CSSImageValue can find it. I think there are possibly other call sites like CSSCursorImageValue that may need the same code.

[Matt Woodrow]

Comment on attachment 444322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444322&action=review

>> Source/WebCore/dom/Document.cpp:5606
>> +        return URL(URL(), url);
> 
> I think you are correct to say that it may be incorrect to have this here. We’ll need to see which regression tests fail.
> 
> Instead we may want to change CSSImageValue to use another function specific to CSS that adds this behavior and then calls through to Document::completeURL. I might put it in CSSParserContext as a static member function that takes a document, or somewhere else that CSSImageValue can find it. I think there are possibly other call sites like CSSCursorImageValue that may need the same code.

I'm not sure about CSSCursorImageValue, it looks like that's checking the 'href' attribute of an SVG cursor element and likely isn't covered by the css values spec.

How about a static 'should this string be kept as-is without resolving relative to the base url' static helper to CSSValue, and then have both the parser and CSSImageValue check that?

[Darin Adler]

Comment on attachment 444322 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444322&action=review

>>> Source/WebCore/dom/Document.cpp:5606
>>> +        return URL(URL(), url);
>> 
>> I think you are correct to say that it may be incorrect to have this here. We’ll need to see which regression tests fail.
>> 
>> Instead we may want to change CSSImageValue to use another function specific to CSS that adds this behavior and then calls through to Document::completeURL. I might put it in CSSParserContext as a static member function that takes a document, or somewhere else that CSSImageValue can find it. I think there are possibly other call sites like CSSCursorImageValue that may need the same code.
> 
> I'm not sure about CSSCursorImageValue, it looks like that's checking the 'href' attribute of an SVG cursor element and likely isn't covered by the css values spec.
> 
> How about a static 'should this string be kept as-is without resolving relative to the base url' static helper to CSSValue, and then have both the parser and CSSImageValue check that?

CSSCursorImageValue has URLs that come from the CSS "cursor" property. Syntax like: "cursor: url(hand.cur), pointer" that is parsed by the consumeCursor function in CSSPropertyParser.cpp. It’s simply got less test coverage than CSSImageValue, but it has many of the same issues.

It’s fine to put a static helper somewhere; it’s just much better to have this packaged as part of a URL completion algorithm rather than a check outside. Having a separate check will result in repetitive code outside the function. So maybe we should just add new completeImageURL or completeCSSImageURL functions that wrap the existing completeURL functions.

And yes, still nice to share the check for which URLs should not be resolved relative to the base for this case, and name based on the specification. Like a function named isCSSLocalURL.

[Matt Woodrow]

(In reply to Darin Adler from comment #19)
 
> CSSCursorImageValue has URLs that come from the CSS "cursor" property.
> Syntax like: "cursor: url(hand.cur), pointer" that is parsed by the
> consumeCursor function in CSSPropertyParser.cpp. It’s simply got less test
> coverage than CSSImageValue, but it has many of the same issues.

My understanding is that CSSCursorImageValue just wraps a CSSImageValue in that case (via m_imageValue), so fixing the completeURL lookups within CSSImageValue should be sufficient.

The call to document.completeURL from within CSSCursorImageValue looks to be specific to the case where the cursor property references an element within SVG, and we're looking up the href attribute from that.

> 
> It’s fine to put a static helper somewhere; it’s just much better to have
> this packaged as part of a URL completion algorithm rather than a check
> outside. Having a separate check will result in repetitive code outside the
> function. So maybe we should just add new completeImageURL or
> completeCSSImageURL functions that wrap the existing completeURL functions.
> 
> And yes, still nice to share the check for which URLs should not be resolved
> relative to the base for this case, and name based on the specification.
> Like a function named isCSSLocalURL.

The part I'm struggling with is that CSSImageValue wants to use the algorithm in Document, whereas the consumers in the parser want to use the CSSParserContext algorithm. These differ a fair bit in their resolution of the base url, and character encoding.

A single completeCSSImageURL function would need to take either a CSSParserContext or Document, and pick which completeURL function to call based on which was provided at runtime (or I could template it, since the args as the same).

[Darin Adler]

(In reply to Matt Woodrow from comment #20)
> My understanding is that CSSCursorImageValue just wraps a CSSImageValue in
> that case (via m_imageValue), so fixing the completeURL lookups within
> CSSImageValue should be sufficient.

Good point. That seems right!

Do we have test coverage for that case in WPT, or is it just code inspection that gives us the confidence that we handle the cursor image case correctly?

> The call to document.completeURL from within CSSCursorImageValue looks to be
> specific to the case where the cursor property references an element within
> SVG, and we're looking up the href attribute from that.

Agreed, yes.

> > It’s fine to put a static helper somewhere; it’s just much better to have
> > this packaged as part of a URL completion algorithm rather than a check
> > outside. Having a separate check will result in repetitive code outside the
> > function. So maybe we should just add new completeImageURL or
> > completeCSSImageURL functions that wrap the existing completeURL functions.
> > 
> > And yes, still nice to share the check for which URLs should not be resolved
> > relative to the base for this case, and name based on the specification.
> > Like a function named isCSSLocalURL.
> 
> The part I'm struggling with is that CSSImageValue wants to use the
> algorithm in Document, whereas the consumers in the parser want to use the
> CSSParserContext algorithm. These differ a fair bit in their resolution of
> the base url, and character encoding.

Well, you should be aware that the one in CSSImageValue is something kind of new, and only used rarely in cases where no base URL is around at the time the CSS value was parsed. I would be surprised if any real world test case relies on these subtle differences between the two, it's just that it needs to happen at a time where we don't have a CSS parsing context. I think we'd probably want it to use the CSS completion algorithm, not the general purpose document one. For now, I would do that by making a cover that takes a document.

> A single completeCSSImageURL function would need to take either a
> CSSParserContext or Document, and pick which completeURL function to call
> based on which was provided at runtime (or I could template it, since the
> args as the same).

Two separate functions would be fine.

I would likely come look at this later and refactor to try to make this have less repeated code, so what matters most is the test coverage, even more than exactly how we right the code.

[Matt Woodrow]

(In reply to Darin Adler from comment #21)
 
> Do we have test coverage for that case in WPT, or is it just code inspection
> that gives us the confidence that we handle the cursor image case correctly?

The changes to invalid-cursor-property-crash.html test the empty URL piece for `cursor`, I don't think there's any current coverage for the fragment-only piece.

[Matt Woodrow]

Created attachment 444457 [details]
Patch

[Darin Adler]

(In reply to Matt Woodrow from comment #22)
> (In reply to Darin Adler from comment #21)
> > Do we have test coverage for that case in WPT, or is it just code inspection
> > that gives us the confidence that we handle the cursor image case correctly?
> 
> The changes to invalid-cursor-property-crash.html test the empty URL piece
> for `cursor`, I don't think there's any current coverage for the
> fragment-only piece.

Let’s add that. Doesn’t have to be in this patch, but let’s follow through and do it.

[Matt Woodrow]

Created https://github.com/web-platform-tests/wpt/pull/31655 for the additional testing.

[Darin Adler]

Comment on attachment 444457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444457&action=review

> Source/WebCore/css/CSSImageValue.cpp:87
> +    if (CSSValue::isCSSLocalURL(m_location.resolvedURL.string()))

This can just be:

   if (isCSSLocalURL(...

Since CSSImageValue derives from CSSValue.

> Source/WebCore/css/CSSImageValue.cpp:88
> +        return URL(URL(), m_location.resolvedURL.string());

This can just be:

    return m_location.resolvedURL;

No need to re-create the URL from a string.

> Source/WebCore/css/CSSValue.h:157
> +    // Empty URLs and fragment-only URLs should not be resolved relative to the base
> +    // URL.

Let’s not break the word URL onto a separate line.

> Source/WebCore/css/CSSValue.h:158
> +    static bool isCSSLocalURL(const String& url)

I might name this argument "relativeURL". I suggest this take a StringView, not a const String&.

If it was me, I would also move this function body out of the class definition.

[Darin Adler]

Comment on attachment 444457 [details]
Patch

This is OK, as is, but I have a few suggestions for further improvement.

[Matt Woodrow]

Created attachment 444620 [details]
Patch

[Darin Adler]

Comment on attachment 444620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444620&action=review

> Source/WebCore/css/parser/CSSParserContext.cpp:257
> +        if (CSSValue::isCSSLocalURL(string))
> +            return { string, { URL(), string } };
> +
>          if (charset.isEmpty())
>              return { string, { baseURL, string } };
>          auto encodingForURLParsing = TextEncoding { charset }.encodingForFormSubmissionOrURLParsing();

I think this might do the wrong thing when the fragment URLs contain non-ASCII characters since we are not taking the charset into account. We should make sure we have test cases for that.

I am not insisting that we add such test cases *before* landing this patch, but I strongly suspect we don’t have tests where the charset would make a difference, and we need to add them.

[Darin Adler]

Let’s rebase this and get it landed!

[Matt Woodrow]

Created attachment 444717 [details]
Patch

[Matt Woodrow]

Created attachment 444768 [details]
Patch

[EWS]

Committed r286061 (244448@main): <https://commits.webkit.org/244448@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444768 [details].

[Darin Adler]

Comment on attachment 444768 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444768&action=review

> Source/WebCore/css/parser/CSSParserContext.cpp:253
> +        if (CSSValue::isCSSLocalURL(string))
> +            return { string, { URL(), string } };

I suspect think this is going to handle non-ASCII characters incorrectly when charset is not UTF-8. Can we make some test cases to check that in other browsers?