Page MenuHomePhabricator

css sanitizer getSizingAdditions() use by multicol sanitizer allows invalid CSS
Open, Needs TriagePublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
Edit goes through.

What should have happened instead?:
I think this edit should have been rejected: according to the specification, column-width must either be an absolute width or auto. This keyword (and in fact any other keywords in the array in getSizingAdditions()) is neither.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

"This use looks funny" from code searching the function being modified in the T271958#9954936 patch.

Event Timeline

Removing this would be simple in code, only this (and one test):

		$props['column-width'] = new Alternative( array_merge(
			[ $matcherFactory->length(), $auto ],
			// Additional values from https://www.w3.org/TR/2019/WD-css-sizing-3-20190522/
			$this->getSizingAdditions( $matcherFactory )
		) );

It is mentioned in the initial spec in the code:
https://www.w3.org/TR/2019/WD-css-sizing-3-20190522/#column-sizing

What is the procedure here? Should it be kept for backward compatibility, or can we just remove it? I assume template styles using column-width: max-content; will not save correctly after removing that line. Not sure if they will render.

What is the procedure here? Should it be kept for backward compatibility, or can we just remove it? I assume template styles using column-width: max-content; will not save correctly after removing that line. Not sure if they will render.

I would expect concerns around a security-sensitive software package to imply removal in the general case.

Anyway, there is no procedure. They will not save correctly post-removal. I do not know about rendering either. I suppose that forms at least part of my motivation with tasks such as T354228 and T162379.

A quick global search looking only for and not e.g. \s for max-content/min-content in column-width didn't find anything. I didn't look in columns. So it is possible that there is no-one using these keywords in that context.

If the specs do not match, probably a bug should be submitted to the standardization committee. It's possible during implementation of css-sizing-3 that they were unable to get things to work, though max/min-content seems simple enough given the implementation note in css-sizing-3. Or that it was dropped because no browsers do support the added keywords.

Change #1074721 had a related patch set uploaded (by Eccenux; author: Eccenux):

[css-sanitizer@master] Support for CSS Box Sizing Level 4

https://gerrit.wikimedia.org/r/1074721

OK, I searched on plwiki in our template using column layout -- also no usages of (min|max|fit) in width param.

Added to the Box Sizing path (there would probably be conflicts if this was in two patches). Also this is kind of related in that:

Level 3 says:

The following features are at-risk, and may be dropped during the CR period:

  • Additions to column-width

Level 4 removed it completely (now part of CSS Multi-column Layout Module Level 1)

What is the procedure here? Should it be kept for backward compatibility, or can we just remove it? I assume template styles using column-width: max-content; will not save correctly after removing that line. Not sure if they will render.

A quick global search looking only for and not e.g. \s for max-content/min-content in column-width didn't find anything. I didn't look in columns. So it is possible that there is no-one using these keywords in that context.

Did a bit more detailed look, did not see anything in global search. Example query.

Added to the Box Sizing path (there would probably be conflicts if this was in two patches).

I think correcting this issue can be fixed before correcting the other (and should be fixed separately). In git you can set the other to depend on a patch to fix this issue. (I don't know how to do that, just am like 95% certain it can be done. :^)