Page MenuHomePhabricator

CSSMin::minify should not interfere with string values
Closed, DeclinedPublic

Description

Looking at a syntaxhighlighter plugin somewhere, I found the following sample (simplified here):

pre.code-json {
    position: relative;
    padding: 12px 7px;
}
pre.code-json::after {
    content: '{ "JSON": "code" };';
    position: absolute;
    bottom: 2px;
    right: 2px;
    ....
}

(which positions a label over the <pre>)

The CSSMin:minify function[1] currently does not respect string property values (either single or double quotes) and causes the following bug:

return CSSMin::minify( 'foo::after { content: \'{ "JSON": "code" };\'; }' );

foo::after{content:'{"JSON":"code" };'}

Note that the value changed:

content: '{ "JSON": "code" };'
content: '{"JSON":"code" };'

In this case it's just a little whitespace, but I can come up with other examples where characters are removed and what not.

[1] https://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/includes/libs/CSSMin.php?revision=110557&view=markup#l210

Details

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:16 AM
bzimport set Reference to bz35492.
bzimport added a subscriber: Unknown Object (MLST).

Re-opening. Bugs are marked fixed when they are fixed in the software. Gerrit merge requests are not in the software yet, they're proposals in in gerrit which, if and when they pass review, will be submitted to the repository.

(In reply to comment #2)

Re-opening. Bugs are marked fixed when they are fixed in the software. Gerrit
merge requests are not in the software yet, they're proposals in in gerrit
which, if and when they pass review, will be submitted to the repository.

Sorry; I even used a similar case in bug 17322 comment 12 as a bad example.

I'd like to say I'm going to ask for the patch to be merged to 1.20, but it looks like Tim Starling has some substantive objections. Pushing to future release.

Yeah, this bug has been around for 2 years. While we know how to trigger it, we haven't hit the bug in any of the real plugins and extensions we've come across. It is only a matter of time before we will, though.

Matma Rex commented this bug on Gerrit:

"Before doing it properly, couldn't we extract all the strings and replace them with unique identifiers (using regexes, of course), apply regex whitespace magic, and re-insert the strings? This should be easy to do, the regex for strings would just have to be crafted carefully to handle escape sequences. (I sort of have a feeling this comment should go on the bug instead. Eh.)"

[ Bug assigned to code submitter. ]

Commit listed above was abandonned; resetting to "NEW".

Krinkle renamed this task from CSSMin::minify should not interfere value contents of string values to CSSMin::minify should not interfere with string values.Aug 5 2016, 2:06 AM
Krinkle moved this task from Confirmed Problem to Backlog on the MediaWiki-ResourceLoader board.
Krinkle removed a project: Future-Release.
Krinkle set Security to None.
Krinkle moved this task from Backlog to Confirmed Problem on the MediaWiki-ResourceLoader board.
Krinkle removed a subscriber: wikibugs-l-list.
Krinkle lowered the priority of this task from Medium to Low.Aug 18 2018, 6:17 AM

Change 711190 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/libs/Minify@master] docs: Write down known limitations

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

Krinkle claimed this task.

Change 711190 merged by jenkins-bot:

[mediawiki/libs/Minify@master] docs: Write down known limitations

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