Opened 2 years ago

Closed 2 years ago

#22457 closed defect (fixed)

Line wrapping and syntax highlighting do not work on view-source pages

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-regression, tbb-6.5-issues, TorBrowserTeam201706R
Cc: yawning, mcs, brade, arthuredelstein, gapegas7uftp Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

https://blog.torproject.org/comment/241854#comment-241854 pointed out that view-source pages are broken wrt line wrapping. Additionally, it turns our that syntax highlighting is affected as well. This is a regression caused by #8725.

Child Tickets

Change History (15)

comment:1 Changed 2 years ago by gk

Cc: yawning mcs brade arthuredelstein added

Hm. So, this got introduced with #8725. I have a branch (bug_22457 (https://gitweb.torproject.org/user/gk/torbutton.git/commit/?h=bug_22457&id=cb12ea9247fae28bb10c4ad3fb4e751147adcc76) ) that fixes the problem by adding viewsource.css to the whitelist. However, the CSS file for Linux is significantly different from that for Windows/macOS allowing probably detection of Linux/non-Linux platforms.

While we are not actively protecting against platform detection it seems to me we should avoid adding additional means if we can. Thus, I am proposing to WONTFIX this issue especially as the vast majority of users should be unaffected by this bug due to e10s being enabled by default.

comment:2 Changed 2 years ago by gk

Keywords: tbb-regression tbb-6.5-issues added

comment:3 in reply to:  1 Changed 2 years ago by yawning

Replying to gk:

While we are not actively protecting against platform detection it seems to me we should avoid adding additional means if we can. Thus, I am proposing to WONTFIX this issue especially as the vast majority of users should be unaffected by this bug due to e10s being enabled by default.

I would be ok with it being whitelisted or not (I think platform detection is totally a lost cause, and will be forever). Perhaps it should be whitelisted only if e10s is disabled.

nb: The "better" solution to the resource/chrome URI situation (where all requests originating from
the local browser are whitelisted) would fix this.

comment:4 Changed 2 years ago by gk

Keywords: TorBrowserTeam201705 added

comment:5 Changed 2 years ago by gk

I think I have a better fix for that: view-source should not be available anymore in esr52, so we can allow it unconditionally I think.

Last edited 2 years ago by gk (previous) (diff)

comment:7 in reply to:  5 ; Changed 2 years ago by gk

Replying to gk:

I think I have a better fix for that: view-source should not be available anymore in esr52, so we can allow it unconditionally I think.

On second thought that might not help much as the blocked CSS is the problem.

comment:8 Changed 2 years ago by cypherpunks

You can experiment with https://bugzilla.mozilla.org/show_bug.cgi?id=863246, because they're going to land it.

comment:9 Changed 2 years ago by gk

Cc: gapegas7uftp added

Resolved #22440 as duplicate.

comment:10 in reply to:  7 ; Changed 2 years ago by gk

Description: modified (diff)
Keywords: TorBrowserTeam201705R added; TorBrowserTeam201705 removed
Status: newneeds_review
Summary: Line wrapping and syntax highlighting do not work in non-e10s-mode on view-source pagesLine wrapping and syntax highlighting do not work on view-source pages

Replying to gk:

Replying to gk:

I think I have a better fix for that: view-source should not be available anymore in esr52, so we can allow it unconditionally I think.

On second thought that might not help much as the blocked CSS is the problem.

But we can work around that using aRequestOrigin. See bug_22457_v2 () for a possible fix that avoids the plaform fingerprinting. Note: For testing purposes you need either disabled e10s or apply the patch on top of the one in #22459.

comment:11 Changed 2 years ago by gk

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201705R removed

comment:12 in reply to:  10 ; Changed 2 years ago by arthuredelstein

Replying to gk:

Replying to gk:

Replying to gk:

I think I have a better fix for that: view-source should not be available anymore in esr52, so we can allow it unconditionally I think.

On second thought that might not help much as the blocked CSS is the problem.

But we can work around that using aRequestOrigin. See bug_22457_v2 () for a possible fix that avoids the plaform fingerprinting. Note: For testing purposes you need either disabled e10s or apply the patch on top of the one in #22459.

I couldn't find the bug_22457_v2 patch. Does it need to be pushed?

I agree with Yawning that hiding differences between platforms is a hopeless cause, so I would also be happy with whitelisting the appropriate css files.

comment:13 in reply to:  12 Changed 2 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Replying to gk:

Replying to gk:

I think I have a better fix for that: view-source should not be available anymore in esr52, so we can allow it unconditionally I think.

On second thought that might not help much as the blocked CSS is the problem.

But we can work around that using aRequestOrigin. See bug_22457_v2 () for a possible fix that avoids the plaform fingerprinting. Note: For testing purposes you need either disabled e10s or apply the patch on top of the one in #22459.

I couldn't find the bug_22457_v2 patch. Does it need to be pushed?

It did indeed: https://gitweb.torproject.org/user/gk/torbutton.git/commit/?h=bug_22457_v2&id=7d65c8297fe862906bcb3d7c9c13e5c5b628943a

comment:14 Changed 2 years ago by mcs

r=brade, r=mcs
The code changes look fine and the patch seems to fix the problem (we did a quick test on OSX).

comment:15 Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks. Cherry-picked onto master (commit 137c0527b1d152c5999db53894badc54ab9e34c9).

Note: See TracTickets for help on using tickets.