Flash exceptions list does not accept input of full URLs ("Add" button stays greyed out)

ASSIGNED

Status

Camino Graveyard
Preferences
ASSIGNED
8 years ago
7 years ago

People

(Reporter: kelsaa, Assigned: Christopher Henderson)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.0.15) Gecko/2009102617 Camino/2.0 (like Firefox/3.0.15)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.0.15) Gecko/2009102617 Camino/2.0 (like Firefox/3.0.15)

I can’t add anything to the Flash exceptions list in Preferences in Camino. Upon entering a URL (i. e. "http://www.radcollector.com/") in the input field, expected behavior would be that the button labeled "Add" became active. Nothing happens.

As a result, it’s either “All on” or “All off” as far as Flash goes.

Reproducible: Always

Steps to Reproduce:
1. Open Preferences, go to "Web Features", click the button labeled "Edit Flash exceptions list"
2. Enter a URL
3. Notice that there is no way to actually add the URL to the list
The validation code is designed to work only with hosts, not with full URLs; entering www.radiocollector.com will work (there's placeholder hint text in the field, but you only see it if the field isn't focused, which it is by default).

I think maybe we need to allow protocols to be entered (and, if a protocol is present, validate it that way) and then stip them before committing to Flashblock's exceptions list.
Blocks: 475201
Status: UNCONFIRMED → NEW
Component: Plug-ins → Preferences
Ever confirmed: true
Flags: camino2.0.2?
QA Contact: plugins → preferences
Summary: Flash exceptions list does not accept input ("Add" button stays greyed out) → Flash exceptions list does not accept input of full URLs ("Add" button stays greyed out)
Also, kelsaa@gmail.com, you can use the context menu item on any blocked Flash to automatically add the site to the exceptions list, with no need to enter any data manually: http://caminobrowser.org/documentation/annoyances/#whitelist_flash
(Reporter)

Comment 3

8 years ago
Did try removing "http://". Same result.
(In reply to comment #3)
> Did try removing "http://". Same result.

You also have to remove the trailing slash shown in your original comment, since it's an invalid character in a hostname.
In the channel, Stuart suggested what we should do here is: 

Add a formatter that

1) Stript paths on paste ("/" or "/baz")
2) Prevent users from typing a path ("/", etc.) once there's a host or scheme+host entered

Then, on commit

1) Strip the protocol scheme from what we pass to Flashblock's internal list

As an added bonus, the same formatter+commit logic should work for popups (bug 370722), too.

Comment 6

8 years ago
I'll have a patch for this shortly.
Assignee: nobody → cl-bugs-new2
Status: NEW → ASSIGNED

Updated

8 years ago
Severity: major → normal
Hardware: x86 → All

Comment 7

8 years ago
Philip, does Flashblock accept ".foo.tld" domains, or treat "*.foo.tld" any differently from "foo.tld" itself?

Comment 8

8 years ago
I'm really bad at regexp but here is our current code:

var expr = gFlashblockWhitelist[i];
expr = expr.replace(/\./g, "\\.");
expr = expr.replace(/\-/g, "\\-");
expr = expr.replace(/\?/g, "\\?");
expr = expr.replace(/\+/g, "\\+");
//expr = expr.replace(/\*/g, "[A-Za-z0-9_\\-\\.]*")
//expr = expr.replace(/\*/g, "[^ \t\v\n\r\f]*")
expr = expr.replace(/\*/g, ".*")
if (expr.slice(-2) != ".*")
	expr = expr + ".*"
expr = expr + "$"; // "^" + 
	var re = new RegExp(expr);
if(re.test(host))
	return true;

So since I removed the "^" from the regexp some time back I would expect "foo.tld" to be the same as "*foo.tld". Of course I could be totally wrong since I've never been able to wrap my head around regexp.

Comment 9

8 years ago
I tried adding "*.foo.tld" and "foo.tld" to the exceptions list in a nightly build this evening. Both were written to the pref as "foo.tld", and both show up in the exceptions list as "foo.tld" (so we end up with duplicate foo.tld entries, *both* of which disappear when you delete one of them -- that's a bug in our current code, which aims to prevent duplicate entries). Based on that current behaviour and comment 8, then, it seems safe to ignore (prevent) asterisks entirely.

If we prevent them and then find we get bugs filed about the inability to type wildcard characters (which Flashblock then proceeds to ignore anyway, just as it ignores schemes, so such an ability is of rather dubious utility), we could re-consider that decision. I *think* this is the first bug we've ever had filed on that input field, so typing in this field seems unlikely to be a very common user action to begin with, and wanting to type wildcard characters at the beginning of a hostname seems likely to be even less common.

cl

Comment 10

8 years ago
Created attachment 418528 [details] [diff] [review]
fix v1.0

A few comments/questions about the patch, which I'm sending to hendy for review...

1) Would it be better to store the |illegalHostnameCharacters| character set as a member variable on the WhitelistFormatter class? That way we only do that work once each time a formatter is init-ed, rather than every time a character is typed via isPartialStringValid:. My gut says this is a good idea, but I'd like someone to confirm that.

2) I removed the whitespace stripping on the popup exceptions list commit code because the formatter takes care of that for us. I didn't bother commenting to that effect in the code because it's kind of difficult to comment on code that isn't there any more ;)

3) I'm not totally tied to the idea of preventing someone from entering "com", for example, in the popup exceptions list, but I think it's very unlikely that anyone besides dwitte, mvl, myself, and a handful of other people know how to use that permission properly. I think it's a bigger win to keep most users from entering something that drastic by accident, since the handful of people who fully understand what a "com" entry will do are also aware of other ways to create such a permission if they wish. If other people think we should give users enough rope to hang themselves, I'm OK with loosening that validation a bit.

Regardless of the outcome of the discussion of point 3, I think we should prevent them from entering ".", which we currently allow. dwitte or mvl are probably the only two people who can say for sure, but I believe that entry creates a permission for all hosts in all domains in all TLDs; i.e., you could globally override the popup blocker by accident by typing a period in the whitelist field and hitting enter. If the consequences aren't that serious, it's likely a meaningless thing to add.
Attachment #418528 - Flags: review?(trendyhendy2000)

Comment 11

8 years ago
Created attachment 420964 [details] [diff] [review]
fix v1.1, strips whitespace on paste

hendy has some tests he's gonna run this through, but I figured I'd put the latest version of this up since it's probably what's going to be sent to sr.
Attachment #418528 - Attachment is obsolete: true
Attachment #420964 - Flags: review?(trendyhendy2000)
Attachment #418528 - Flags: review?(trendyhendy2000)
(Assignee)

Updated

8 years ago
Attachment #420964 - Flags: review?(trendyhendy2000) → review-
(Assignee)

Comment 12

8 years ago
Comment on attachment 420964 [details] [diff] [review]
fix v1.1, strips whitespace on paste

Validation passes all the tests I gave it. Adding unit tests to a prefpane bundle is beyond my current capabilities, however.

Need WebFeatures.h with WhitelistFormatter interface declaration.

Truncate comment lines to 80 chars.

The non-kosher adjacent character checks could probably be reduced to a single if block within an enumeration of an array of the various cases.

Probably is a good idea to make illegalHostnameCharacters a member variable.

Comment 13

8 years ago
Created attachment 421948 [details] [diff] [review]
fix v1.2

Addresses all the review comments, along with a few other minor issues I discovered in the process of updating it.
Attachment #420964 - Attachment is obsolete: true
Attachment #421948 - Flags: review?(trendyhendy2000)
(Assignee)

Comment 14

8 years ago
Comment on attachment 421948 [details] [diff] [review]
fix v1.2

Looks good, just needs a dealloc to release mIllegalHostnameCharacters.

r+ otherwise.
Attachment #421948 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #421948 - Flags: review?(trendyhendy2000)
Attachment #421948 - Flags: review+

Comment 15

8 years ago
Comment on attachment 421948 [details] [diff] [review]
fix v1.2

>+// A formatter for whitelist entry fields that strips and prevents paths
>+// in the host entry field and strips any scheme before passing it to the
>+// whitelist code.

Except it doesn't strip the scheme.

>+  [mAddField setFormatter:[[[WhitelistFormatter alloc] init] autorelease]];

Are we trying to keep nibs frozen for the next release? I didn't think so, in which case this should be doable in the nib.

>+    host = [host substringFromIndex:(schemeRange.location + schemeRange.length)];

Use NSMaxRange.

>+  [mAddFlashblockField setFormatter:[[[WhitelistFormatter alloc] init] autorelease]];

Same as above.

>+    // This (very basic) validation ensures the user has entered both a TLD and 
>+    // a domain. The permissions manager has the ability to block/allow popups
>+    // from all domains in a given TLD (i.e., "com" is a valid entry and will
>+    // set a permission for all dot-com domains)

Why are we preventing people from entering valid entries like "com" if they really want to? I'm not seeing the benefit.

>+    if (!(([partialString characterAtIndex:(slashRange.location - 1)] == ':') &&

What if the string starts with "/"?

>+      *newString = [partialString substringToIndex:(slashRange.location)];
>+      return NO;

This is repeated in the "else" clause; rather than have an else at all, you can just adjust slashRange in an "if" for the :// case, and then press on.

>+        // Strip any colons after the scheme:// is entered, too.

... unless there was also a /, in which case you already returned just above, so this never runs.

But why do we want to do this anyway? Is restricting a rule by port disallowed? If so, say that in the comment.

>+  // Re-use this character set, since it's expensive to make.

I'm unclear on why we want any of the rest of this code. We strip paths in the formatter so that people understand that they are adding a site, not a domain. Why do we care if they deliberately enter bogus domain characters? Will it break the whole whitelisting system? I assume not, and if not, what are we gaining here? We can't prevent people from accidentally typoing the domain, so going to great lengths to prevent what is probably a tiny fraction of typos doesn't seem like it really helps users.

Is there some other advantage I'm missing?

>+    NSRange schemeRange = [aSite rangeOfString:@"://"];
>+    if (schemeRange.location != NSNotFound)
>+      aSite = [aSite substringFromIndex:(schemeRange.location + schemeRange.length)];

This code is showing up a number of times in this patch; make an NSString category for it.
Attachment #421948 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
(In reply to comment #15)
> >+  [mAddField setFormatter:[[[WhitelistFormatter alloc] init] autorelease]];
> 
> Are we trying to keep nibs frozen for the next release? I didn't think so, in
> which case this should be doable in the nib.

This bug is nominated for 2.0.2 (because the current code for this new-in-2 UI makes the UI seem broken when people try to paste things that they think they should be able to paste), so for 2.0.x we need a code-only patch.
(In reply to comment #15)
> >+    // This (very basic) validation ensures the user has entered both a TLD and 
> >+    // a domain. The permissions manager has the ability to block/allow popups
> >+    // from all domains in a given TLD (i.e., "com" is a valid entry and will
> >+    // set a permission for all dot-com domains)
> 
> Why are we preventing people from entering valid entries like "com" if they
> really want to? I'm not seeing the benefit.

> I'm unclear on why we want any of the rest of this code. We strip paths in the
> formatter so that people understand that they are adding a site, not a domain.
> Why do we care if they deliberately enter bogus domain characters? Will it
> break the whole whitelisting system? I assume not, and if not, what are we
> gaining here? We can't prevent people from accidentally typoing the domain, so
> going to great lengths to prevent what is probably a tiny fraction of typos
> doesn't seem like it really helps users.
> 
> Is there some other advantage I'm missing?

I've been thinking about this off-and-on for a while now.  The question I keep asking is "what's going to surprise our users the least; what behavior are they least likely to think is broken?"

Is it going to be the fact that they get pop-ups on every .com site they visit, even though they've enabled the blocker, because they accidentally whitelisted "com" via a bad paste or fat-fingering?  Or is it going to be wondering why the button is greyed out with just "com" in the input field?

Is it going to be the fact that Flashblock appears not to work on foo-bar.com because they entered foo+bar.com, or that the button is greyed out when foo+bar.com is entered in the input field.  What about when they paste http://例子.测试/首页 ?  Do Flashblock and the permissions backend understand IDN?  Do they punycode, or are we expected to send them punycode?  (We write the Flashblock whitelist, and we write UTF-8 IDN, but whether Flashblock actually works with that, I don't know.  Permissions appears to punycode an IDN when we submit it.)

In the second case, I think it's a lot messier; with IDN, we have a lot more to think about--beyond the fact this bug was essentially filed because our users found the disabled button more confusing when pasting in a URL that otherwise seemed valid.

In the first case, though, I find it much harder to make an argument that we're causing confusion, since people aren't going to think of "com" or "*.com" as a valid URL, and there I'd prefer we err on the side of making our feature seem less broken by refusing obscure but wide-ranging-in-impact input syntaxes.

That said, we really need a patch for this for branch in the next day or so, or this will miss the boat in 2.0.2.
(In reply to comment #17)
> That said, we really need a patch for this for branch in the next day or so, or
> this will miss the boat in 2.0.2.

And it has.  We need to get this done for 2.0.3.
Flags: camino2.0.3?
Flags: camino2.0.2?
Flags: camino2.0.2-
Assignee: cl-bugs-new2 → trendyhendy2000
Flags: camino2.0.3? → camino2.0.3-
Flags: camino2.0.4? → camino2.0.4-
Flags: camino2.1?
Flags: camino2.0.5?
Flags: camino2.0.5? → camino2.0.5-
Flags: camino2.0.6? → camino2.0.6-
You need to log in before you can comment on or make changes to this bug.