Opened 4 years ago

Closed 4 years ago

#17790 closed defect (fixed)

unit tests for keyboard defenses

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201605R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Our patches for #15646 and #17009 should have unit tests.

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by arthuredelstein

Keywords: TorBrowserTeam201512R added

Here is a patch for review. Please note there are two patches -- regression tests for 15646, and regression tests for 17009:

https://github.com/arthuredelstein/tor-browser/commits/17790.

comment:2 Changed 4 years ago by arthuredelstein

Status: newneeds_review

comment:3 Changed 4 years ago by gk

Keywords: TorBrowserTeam201601R added; TorBrowserTeam201512R removed

Carrying over reviews.

comment:4 Changed 4 years ago by gk

Status: needs_reviewneeds_revision

e58782ab2034cf30ea548b0f87de122ac4805118

+        // We should only see the modifier key in content if suppression is
+        // active; otherwise we expect to see the "x" key instead.
+        let expectedContentKey = suppressModifiers ? "x" : modifierKey;

It seems the comment does not match the code? We get "x" if the modifiers are suppressed and the modifier key otherwise (which is intended). So, s/active/not active/ ?

More importantly, the behavior of privacy.suppressModifierKeyEvents is dependent on the value for privacy.resistFingerprinting but the test does not take that into account. I think we should at least assume explicitly that the latter is true. It might be good, though, to test as well with the latter being false to make sure the code for #17009 is not kicking in even if privacy.suppressModifierKeyEvents is true.

d5481537329a7a77ab597f40892c7df83d0ffcc2

+  // Return a promise that resolves to the event when]

s/]

#15646 takes care of more things than keyCode and shiftKey, e.g: code and loaction as well and Alt-key handling. What is with those?

comment:5 in reply to:  4 ; Changed 4 years ago by arthuredelstein

Replying to gk:

[...]

All good comments and suggestions. I have made fixes to both patches to try to address all of these. Also, in writing the additional tests, I found a mistake in the KeyboardEvent patch, so I am including a fixup patch in this new branch:

https://github.com/arthuredelstein/tor-browser/commits/17790+1

There are three patches in total.

comment:6 in reply to:  5 Changed 4 years ago by gk

Status: needs_revisionneeds_review

Replying to arthuredelstein:

Replying to gk:

[...]

All good comments and suggestions. I have made fixes to both patches to try to address all of these. Also, in writing the additional tests, I found a mistake in the KeyboardEvent patch, so I am including a fixup patch in this new branch:

https://github.com/arthuredelstein/tor-browser/commits/17790+1

There are three patches in total.

I've taken the fixup for tbb-5.5. The tests need a closer look for which I don't have the time now.

comment:7 Changed 4 years ago by gk

Keywords: TorBrowserTeam201602R added; TorBrowserTeam201601R removed

Carrying over review tickets.

comment:8 Changed 4 years ago by gk

Keywords: TorBrowserTeam201603R added; TorBrowserTeam201602R removed

comment:9 Changed 4 years ago by gk

Keywords: TorBrowserTeam201604R added; TorBrowserTeam201603R removed

No easy way to do reviews in March anymore.

comment:10 Changed 4 years ago by gk

Keywords: TorBrowserTeam201605R added; TorBrowserTeam201604R removed

Moving reviews over to May 2016

comment:11 Changed 4 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

I finally got to look at the tests again. I cherry-picked both commits to tor-browser-45.1.0esr-6.0-1 (commit b815400e8aae371dcc55141d202ae96155c4c78d and de5346adae7452c79820958873cf6eca473f537f), thanks.

Note: See TracTickets for help on using tickets.