Can not remove permission for colon-using/URL subjects

[chadf@…]

If a group has been added for a permission subject containing a colon (such as a URL) then it can not be removed. Neither the admin web manager nor trac-admin will remove it. I had to delete the row directly from the permission table in the database.

To recreate, add:

Subject: https://profiles.google.com/someone@example.com Group: foo

Then attempt to delete.

It also breaks for 'http:abc' and 'abc:xyz', but not '/abc', suggesting the colon is the issue.

[Remy Blank]

Thanks for the report. This shouldn't be too difficult to fix.

[osimons]

I've seen this too, with an even more likely use-case; With email as username, someone had copied the email address for permission assignment but the pasted value actually ended up as: mailto:user@domain.com

I'll look into it.

[osimons]

Actually, the ticket description does not quite match my findings: The trac-admin command permission remove mailto:user@domain.com group will remove the assigned action or group. Can anyone reproduce the problem for trac-admin?

As far as I can tell, the problem is restricted to web admin 'Permissions' panel behavior. Which uses subject:action as the underlying value of checkboxes posted back to server, so no doubt it gets confused by additional commas and splits them the wrong way.

[chadf@…]

Looks like the trac-admin remove may be a different issue (probably just) on my side. After the web remove attempt failed, I tried trac-admin next and assumed it was the same problem. Turns out I can't remove any permission that way. So 'admin/web' is the only appropriate component for this bug.

[osimons]

Heh. If we do this it works fine:

trac/admin/web_ui.py

@@ -378 +378 @@
                 sel = req.args.get('sel')
                 sel = isinstance(sel, list) and sel or [sel]
                 for key in sel:
-                    subject, action = key.split(':', 1)
+                    subject, action = key.rsplit(':', 1)
                     if (subject, action) in perm.get_all_permissions():
                         perm.revoke_permission(subject, action)
                 add_notice(req, _('The selected permissions have been '

However, anyone that creates a group looking like 'group:one' will be unable to remove users from the group. So we cannot have both users and groups with colon, we need to decide on one - or replace the colon with some other character less likely to conflict. Suggestions for a better split character?

[Remy Blank]

Whatever we choose, the split character should be forbidden in both user and group names. Do we already have validity checks on the names? If yes, we could simply choose a forbidden character.

[osimons]

Actually, with pluggable authentication backends and group providers the users may arrive in all kinds of shapes and sizes. I doubt picking a printable character from the plain ASCII table will be fail-safe.

[chadf@…]

I was attempting to add a permission for an OpenID based login. This might suggest that the subject part would need to flexible to external authentication mechanisms (but then again maybe there are externally managed groups just as messy).

Less likely to be used in an identifier is a pipe, '|', but still a risk. If this only exists in memory maybe using an array of two with no string joins/splits at all. Another option would be to escape both parts (URL encoding maybe), then no characters would/should ever be a problem.

[osimons]

Using pipe ('|') for splitting subject and action.

[osimons]

Certainly better, but perhaps not ideal. This is just an admin web form issue though, so very easy to switch and try something new out at any time. Perhaps try a pipe now, and reconsider if new corner-cases surface?

[Remy Blank]

How about base64- (or hex-)encoding the value? This would avoid all issues, including issues due to characters not allowed in id= attributes.

[osimons]

Using base64 instead.

[osimons]

Replying to rblank:

How about base64-encoding the values, and using a separator not present in the the set of encoded characters? This would avoid all issues, including those due to characters in values not allowed in id= attributes.

Yeah, lets do it properly while we are at it. We have other places doing the same that may be prone to issues, so added attachment:t10526-alt_perm_sel_base64-r10911-012.diff​ that adds safe conversion functions to and from base64 using utf-8 as intermediate representation for safe unicode handling.

OK?

[Remy Blank]

Looks almost perfect. There's one small issue, though: .encode('base64') will insert newlines regularly in the output (with a large enough input). One possible solution could be to do .replace('\n', '') instead of .rstrip('\n'), but it's a bit ugly (the decoding still works). I'm not sure it's possible to encode directly without newlines (​http://docs.python.org is down, so I can't check ATM).

[osimons]

Ah. It is sometimes useful, but usually not for passing strings back and forth for a web application. What if we used the .replace() strategy and changed the signature to:

def unicode_to_base64(text, strip_newlines=True):
    ...

Optional makes sense to me.

Also, for trunk merge I think we should add these utility functions when we populate main data for templates - making the functions always available in rendering context.

[Christian Boos]

Two nit-picks from my side:

• in unicode_to_base64, no need for the isinstance check, it's the first thing done in to_unicode
• a comment in the template would be useful to explain why we do this encoding (e.g. <!--! base64 make it safe to use ':' as separator when passing both subject and action as one query parameter -->)

[osimons]

Minor tweaks based on feedback.

[osimons]

Thanks for feedback. attachment:t10526-alt_perm_sel_base64_2-r10911-012.diff​ makes minor tweaks to account for most recent suggestions.

[osimons]

Might as well just get this in for the upcoming release. OK? Say the word, and I'll commit.

[Christian Boos]

"Please" ;-)

[osimons]

Heh, I would have settled for "OK" but "please" works even better :-)

Committed in [10929] and merged to trunk in [10930].

[Christian Boos]

Well, when I said "all tests pass" on the other ticket, I lied…

There are a bunch of XHTML validation errors due to this one:

======================================================================
FAIL: runTest (trac.admin.tests.functional.TestCreatePermissionGroup)
Create a permissions group
----------------------------------------------------------------------
...
# Syntax of value for attribute id of input is not valid
# URL: http://127.0.0.1:8328/admin/general/perm
# Line 176, column 44

                  <input type="checkbox" id="YW5vbnltb3Vz:QlJPV1NFUl9WSUVX" name="sel" value="YW5vbnltb3Vz:QlJPV1NFUl9WSUVX" />
                  <label for="YW5vbnltb3Vz:QlJPV1NFUl9WSUVX">BROWSER_VIEW</label>
                </div>
                <div>
                  <input type="checkbox" id="YW5vbnltb3Vz:Q0hBTkdFU0VUX1ZJRVc=" name="sel" value="YW5vbnltb3Vz:Q0hBTkdFU0VUX1ZJRVc=" />
                  <label for="YW5vbnltb3Vz:Q0hBTkdFU0VUX1ZJRVc=">CHANGESET_VIEW</label>
                </div>
                <div>
                  <input type="checkbox" id="YW5vbnltb3Vz:RklMRV9WSUVX" name="sel" value="YW5vbnltb3Vz:RklMRV9WSUVX" />
                  <label for="YW5vbnltb3Vz:RklMRV9WSUVX">FILE_VIEW</label>
                </div>

4 failures on the functional tests, all the same problem.

[Christian Boos]

I was first suspicious about ":", but the W3C validator was a bit more explicit about the problem:

character "=" is not allowed in the value of attribute "id"

[Christian Boos]

Damn…

Line 169, Column 78: character "%" is not allowed in the value of attribute "id"
…" id="YW5vbnltb3Vz:Q0hBTkdFU0VUX1ZJRVc%3D" name="sel" value="YW5vbnltb3Vz:Q0hB…

Next attempt…

[osimons]

I see. My lxml had disappeared, and I hadn't noticed. Installed it again, and I see the errors too. From tests I get the results directly:

Syntax of value for attribute for of label is not valid

I just added the id at the suggestion of rblank, and it was after all an unrelated issue. The id just needs to be unique, but its content is of no consequence AFAICT? So we can perhaps just use;

id="$subject_enc.replace('=',''):$action_enc.replace('=','')"

[Christian Boos]

follow-up to r10929 ('=' in id attributes was not valid XHTML)

[Christian Boos]

At last… I really want to get 0.12.3rc1 out tonight ;-)

[Christian Boos]

Oh, didn't see you suggestion. Might be simpler indeed.

[osimons]

Could even just do .rstrip('=') as the '=' is just padding. Your suggestion is more generic and elaborate, but it introduces a custom base64 that is no longer compatible with other implementations. The newlines we strip is after all optional in the content, while the '=' is not. I think keeping it compatible has a higher value, and instead just make exception in the code where we need to tweak this (like rstrip()).

[Christian Boos]

… but as optional as it may be, just stripping the '=' leads to an Error: Incorrect padding…

Example:

>>> 'bWU'.decode('base64')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\Dev\Python272\lib\encodings\base64_codec.py", line 42, in base64_decode
    output = base64.decodestring(input)
  File "c:\Dev\Python272\lib\base64.py", line 321, in decodestring
    return binascii.a2b_base64(s)
binascii.Error: Incorrect padding

So probably unicode_from_base64 should compensate for it.

[osimons]

Sorry, seems my meaning wan't clear enough; I have no problem making an exception when we won't be decoding it again (which we won't for id). But adding incompatible behavior to library routines is asking for trouble later on, isn't it? We don't know who will be using these util functions and for what purpose.

trac/admin/templates/admin_perms.html

@@ -78 +78 @@
                       py:with="subject_enc=unicode_to_base64(subject);
                                action_enc=unicode_to_base64(action)">
                 <!--! base64 make it safe to use ':' as separator when passing
-                      both subject and action as one query parameter -->
+                      both subject and action as one query parameter
+                      '=' not allowed in id attribute, so strip it there -->
                 <div>
                   <input py:if="revoke_perm" type="checkbox"
-                         id="$subject_enc:$action_enc"
+                         id="$subject_enc.rstrip('='):$action_enc.rstrip('=')"
                          name="sel" value="$subject_enc:$action_enc" />
                   <label for="$subject_enc:$action_enc">$action</label>
                 </div>

[Christian Boos]

Ah yes, only for the id… but also for the for attribute, then, just not for the value.

I'd suggest the following:

trac/admin/templates/admin_perms.html

@@ -75 +75 @@
             <td>$subject</td>
             <td>
               <py:for each="(subject,action) in perm_group"
-                      py:with="subject_enc=unicode_to_base64(subject);
-                               action_enc=unicode_to_base64(action)">
+                      py:with="subject_action='%s:%s' % (unicode_to_base64(subject),
+                                                         unicode_to_base64(action));
+                               subject_action_id=subject_action.replace('=', '')">
                 <!--! base64 make it safe to use ':' as separator when passing
-                      both subject and action as one query parameter -->
+                      both subject and action as one query parameter;
+                      '=' not allowed in id attribute, so strip it there -->
                 <div>
                   <input py:if="revoke_perm" type="checkbox"
-                         id="$subject_enc:$action_enc"
-                         name="sel" value="$subject_enc:$action_enc" />
-                  <label for="$subject_enc:$action_enc">$action</label>
+                         id="$subject_action_id"
+                         name="sel" value="$subject_action" />
+                  <label for="$subject_action_id">$action</label>
                 </div>
               </py:for>
             </td>

[osimons]

Looks good, thanks. Let's go with that. Want me to commit it?

[Christian Boos]

No, it's fine I have it there. Committed in r10933.

[Remy Blank]

Sorry to be late to the party (and for the bad initial advice), but id= attributes in XHTML must have the following property:

ID and NAME tokens must begin with a letter ([A-Za-z]) and may be followed by any number of letters, digits ([0-9]), hyphens ("-"), underscores ("_"), colons (":"), and periods (".").

So "standard" base64 is out of the question, as it doesn't make sure the first character is a letter, and the "/" character can appear. Wikipedia mentions a "modified Base64 for XML identifiers (Name)", which uses the characters "_:" instead of "+/", has no padding and no line terminations. But that still leaves the first character issue. So we should prepend e.g. "_". Which admittedly makes this much more complicated than necessary.

Maybe we should generate the identifier with a different function make_xml_name(), which could use whatever encoding makes sense to generate a unique XML name from a string (or even an MD5 hash), and keep base64 for the value, which has the type CDATA, so it works there.

[Christian Boos]

another follow-up to r10929, based on Rémy's advices and code

[Remy Blank]

I'm really bad at advice these days, and it seems I can't even understand what I write. The standard says must begin with a letter and I advise to prepend a "_" :(

I would still separate the use case "create a unique ID for the id= attribute", where decoding isn't necessary, from "encode a value so that a given character doesn't appear", in for which case we can use standard base64 encoding.

I'll provide a patch.

[Christian Boos]

Replying to rblank:

I'm really bad at advice these days, and it seems I can't even understand what I write. The standard says must begin with a letter and I advise to prepend a "_" :(

It's fine: ​ID → ​Name → ​NameStartChar. I didn't follow blindly ;-)

[Remy Blank]

Replying to cboos:

It's fine: ​ID → ​Name → ​NameStartChar.

Mmh, then that's one point where HTML differs from XHTML (I copy / pasted the section above from the HTML4 standard). I hope this won't uncover incompatibilities between browsers. Oh well…

[Christian Boos]

While I imagine we can certainly still improve upon t10526-unicode_base64-url_safe-r10934.patch​, any reason not to go with this patch? That would enable me to cut a 0.12.3rc1 release tomorrow…

[Remy Blank]

Replying to cboos:

While I imagine we can certainly still improve upon t10526-unicode_base64-url_safe-r10934.patch​, any reason not to go with this patch?

Not from my side.

[Jun Omae]

The id attribute's value is case-sensitive in XHTML specification. However, on IE 7 and 8, the value is case-insensitive. case-sensitive on IE 9.

I think we should use MD5/SHA1 hash instead of base64.

On IE 7, clicking the first label blahblah turns on the second checkbox.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>id/for case</title>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
<body>
<form action="" method="post">
  <input id="blah:blah" type="checkbox" />
  <label for="blah:blah">blah:blah</label>
  <br />
  <input id="Blah:Blah" type="checkbox" />
  <label for="Blah:Blah">Blah:Blah</label>
</form>
</body>
</html>

[Remy Blank]

That's even worse than what I expected when I wrote "incompatibilities between browsers" above :(

A hash sounds good to me (with an initial letter, just for superstition).

[Christian Boos]

Hm, so IIUC correctly Jun, we might get "collisions" with different inputs being encoded to base64 values which are different except for case? If we start to bother about this, then we might as well be cautious against a hash… and then, why not just using a sequence, which better shows the intent anyway (i.e. to simply have unique, valid, ids).

[Christian Boos]

new version, using a sequence for the id

[Christian Boos]

t10526-unicode_base64-url_safe-r10934.2.patch​ was a quick change to show what I meant with the sequence (validates & works well).

Now we could also think about simplifying unicode_to_base64, probably by going back to the initial version of Simon.

[Christian Boos]

"final" version

[osimons]

Looks good. Makes sense to control the ID values more closely.

The docstring in unicode_to_base64 should perhaps say

"... unless ``strip_newlines`` is `False`

… to better convey the meaning and action needed to disable the behavior.

[Remy Blank]

You won't see any collisions with an MD5 in your lifetime, but I agree that using the indexes is the best solution here. Don't some of the functional tests reference the id? If so, they would have to be updated as well.

[Christian Boos]

I know, this was only rhetorical ;-) The functional tests passed with this change, so I think it's fine.

Applied in r10941.