New issue
Advanced search Search tips

Issue 855719 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Escaping and linkifying error in "All changes" list

Project Member Reported by davidben@chromium.org, Jun 22 2018

Issue description

This is a totally minor error, but I figured I may as well file it:

At the bottom-right of, there is a listing of a commit message.
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/GPU%20Linux%20Builder/119407

It says:

   url = "https://example.com/"

with a link to https://example.com/&#34.

The actual commit message, at https://chromium-review.googlesource.com/c/chromium/src/+/1096083 is:

   url = "https://example.com/"

It appears something messed up escaping and linkifying somewhere.
 

Comment 1 by hinoka@chromium.org, Jun 22 2018

Relevant code is here:
https://cs.chromium.org/chromium/infra/go/src/go.chromium.org/luci/milo/frontend/middleware.go?l=95

We use \b as word boundaries, which apparently matches quotation marks in Go.  I don't have an immediate solution in mind for this, but the regex will need to be smarter about custom delimitators, like [",.]

Periods are particularly hard to reason about because they are legal in hostname, but not commonly used in path.  EG "https://example.com." is a valid URL, trailing period inclusive.
I think there's an additional issue that the regex is running after template.HTMLEscapeString, which is why there's some escaping gunk in there.
Owner: davidben@chromium.org
Status: Started (was: Untriaged)
I got bored:
https://chromium-review.googlesource.com/c/infra/luci/luci-go/+/1112575
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 25 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/17909a5ed08ccf3d8b55f37c33cfa7b376c273d8

commit 17909a5ed08ccf3d8b55f37c33cfa7b376c273d8
Author: David Benjamin <davidben@google.com>
Date: Mon Jun 25 22:49:00 2018

[milo] Fix interaction of linkifying rules with each other and escaping.

formatCommitDesc ran regexes on the HTML escaped input. This results in
a number of incorrect conversions. E.g., rewrote:

    https://crbug.com/618365

to:

    <a href="https://<a href="https://crbug.com/618365">crbug.com/618365</a>">https://<a href="https://crbug.com/618365">crbug.com/618365</a></a>

because it would first apply the URL-linking rule on the full URL, then
it would find several instances of crbug.com/618365, which it dutifully
linked as well. This particular case was even enshrined in a test
expectation.

For a truly entertaining case, see
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/GPU%20FYI%20Linux%20Builder/5847.
Here it rewrites:

    Bug:  https://crbug.com/828864 

as:

    Bug: <a href="https://<a href="https://crbug.com/<a href=" https://crbug.com/828864 ">828864</a>">crbug.com/<a href=" https://crbug.com/828864 ">828864</a></a>">https://<a href="https://crbug.com/<a href=" https://crbug.com/828864 ">828864</a>">crbug.com/<a href=" https://crbug.com/828864 ">828864</a></a></a>

There are seven hrefs in there. :-) First, it sees
 https://crbug.com/828864  and linkifies that (1). That results in two
 crbug.com/828864  instances, which matches the next rule (2). That
results in four 828864 instances, which matches the "Bug: ..." rule,
each of which gets linkified (4). 1 + 2 + 4 = 7.

Instead, use a slightly richer intermediate representation that prevents
already-processed chunks of text from being processed a second time.

Bug:  855719 
Change-Id: Ica1aec4ed10ca2547680fca8639a996cd0d529f2
Reviewed-on: https://chromium-review.googlesource.com/1112575
Reviewed-by: Nodir Turakulov <nodir@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>

[modify] https://crrev.com/17909a5ed08ccf3d8b55f37c33cfa7b376c273d8/milo/frontend/expectations/buildbot.build-Debug_page-_CrWinGoma_30608.html
[modify] https://crrev.com/17909a5ed08ccf3d8b55f37c33cfa7b376c273d8/milo/frontend/middleware.go
[modify] https://crrev.com/17909a5ed08ccf3d8b55f37c33cfa7b376c273d8/milo/frontend/middleware_test.go

Status: Fixed (was: Started)

Sign in to add a comment