Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

Last modified 3 years ago

#10526 closed defect (fixed)

Can not remove permission for colon-using/URL subjects

Reported by: chadf@… Owned by: osimons
Priority: normal Milestone: 0.12.3
Component: admin/console Version: 0.12.2
Severity: normal Keywords: bitesized
Cc: Jun Omae Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.

Attachments (7)

t10526-alt_perm_sel_split-r10911-012.diff (3.4 KB ) - added by osimons 11 years ago.
Using pipe ('|') for splitting subject and action.
t10526-alt_perm_sel_base64-r10911-012.diff (8.0 KB ) - added by osimons 11 years ago.
Using base64 instead.
t10526-alt_perm_sel_base64_2-r10911-012.diff (8.6 KB ) - added by osimons 11 years ago.
Minor tweaks based on feedback.
follow-up-r10929.patch (1.7 KB ) - added by Christian Boos 11 years ago.
follow-up to r10929 ('=' in id attributes was not valid XHTML)
t10526-unicode_base64-url_safe-r10934.patch (3.7 KB ) - added by Christian Boos 11 years ago.
another follow-up to r10929, based on Rémy's advices and code
t10526-unicode_base64-url_safe-r10934.2.patch (4.1 KB ) - added by Christian Boos 11 years ago.
new version, using a sequence for the id
t10526-unicode_base64-url_safe-r10934.3.patch (2.4 KB ) - added by Christian Boos 11 years ago.
"final" version

Download all attachments as: .zip

Change History (52)

comment:1 by Remy Blank, 11 years ago

Keywords: bitesized added
Milestone: next-minor-0.12.x

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

comment:2 by osimons, 11 years ago

Owner: set to 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.

comment:3 by osimons, 11 years ago

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.

comment:4 by chadf@…, 11 years ago

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.

comment:5 by osimons, 11 years ago

Heh. If we do this it works fine:

  • trac/admin/web_ui.py

    a b class PermissionAdminPanel(Component):  
    378378                sel = req.args.get('sel')
    379379                sel = isinstance(sel, list) and sel or [sel]
    380380                for key in sel:
    381                     subject, action = key.split(':', 1)
     381                    subject, action = key.rsplit(':', 1)
    382382                    if (subject, action) in perm.get_all_permissions():
    383383                        perm.revoke_permission(subject, action)
    384384                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?

comment:6 by Remy Blank, 11 years ago

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.

comment:7 by osimons, 11 years ago

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.

comment:8 by chadf@…, 11 years ago

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.

by osimons, 11 years ago

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

comment:9 by osimons, 11 years ago

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?

comment:10 by Remy Blank, 11 years ago

How about base64- (or hex-)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.

Last edited 11 years ago by Remy Blank (previous) (diff)

by osimons, 11 years ago

Using base64 instead.

in reply to:  10 comment:11 by osimons, 11 years ago

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?

comment:12 by Remy Blank, 11 years ago

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).

comment:13 by osimons, 11 years ago

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.

comment:14 by Christian Boos, 11 years ago

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 -->)

by osimons, 11 years ago

Minor tweaks based on feedback.

comment:15 by osimons, 11 years ago

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

comment:16 by osimons, 11 years ago

Milestone: next-minor-0.12.x0.12.3

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

comment:17 by Christian Boos, 11 years ago

"Please" ;-)

comment:18 by osimons, 11 years ago

Resolution: fixed
Status: newclosed

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

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

comment:19 by Christian Boos, 11 years ago

Resolution: fixed
Status: closedreopened

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.

comment:20 by Christian Boos, 11 years ago

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"

comment:21 by Christian Boos, 11 years ago

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…

comment:22 by osimons, 11 years ago

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('=','')"

by Christian Boos, 11 years ago

Attachment: follow-up-r10929.patch added

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

comment:23 by Christian Boos, 11 years ago

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

comment:24 by Christian Boos, 11 years ago

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

comment:25 by osimons, 11 years ago

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()).

comment:26 by Christian Boos, 11 years ago

… 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.

comment:27 by osimons, 11 years ago

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

    a b  
    7878                      py:with="subject_enc=unicode_to_base64(subject);
    7979                               action_enc=unicode_to_base64(action)">
    8080                <!--! base64 make it safe to use ':' as separator when passing
    81                       both subject and action as one query parameter -->
     81                      both subject and action as one query parameter
     82                      '=' not allowed in id attribute, so strip it there -->
    8283                <div>
    8384                  <input py:if="revoke_perm" type="checkbox"
    84                          id="$subject_enc:$action_enc"
     85                         id="$subject_enc.rstrip('='):$action_enc.rstrip('=')"
    8586                         name="sel" value="$subject_enc:$action_enc" />
    8687                  <label for="$subject_enc:$action_enc">$action</label>
    8788                </div>

comment:28 by Christian Boos, 11 years ago

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

    diff -r 4a38ae5a0f5c trac/admin/templates/admin_perms.html
    a b  
    7575            <td>$subject</td>
    7676            <td>
    7777              <py:for each="(subject,action) in perm_group"
    78                       py:with="subject_enc=unicode_to_base64(subject);
    79                                action_enc=unicode_to_base64(action)">
     78                      py:with="subject_action='%s:%s' % (unicode_to_base64(subject),
     79                                                         unicode_to_base64(action));
     80                               subject_action_id=subject_action.replace('=', '')">
    8081                <!--! base64 make it safe to use ':' as separator when passing
    81                       both subject and action as one query parameter -->
     82                      both subject and action as one query parameter;
     83                      '=' not allowed in id attribute, so strip it there -->
    8284                <div>
    8385                  <input py:if="revoke_perm" type="checkbox"
    84                          id="$subject_enc:$action_enc"
    85                          name="sel" value="$subject_enc:$action_enc" />
    86                   <label for="$subject_enc:$action_enc">$action</label>
     86                         id="$subject_action_id"
     87                         name="sel" value="$subject_action" />
     88                  <label for="$subject_action_id">$action</label>
    8789                </div>
    8890              </py:for>
    8991            </td>

comment:29 by osimons, 11 years ago

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

comment:30 by Christian Boos, 11 years ago

Resolution: fixed
Status: reopenedclosed

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

comment:31 by Remy Blank, 11 years ago

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.

by Christian Boos, 11 years ago

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

comment:32 by Remy Blank, 11 years ago

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.

in reply to:  32 ; comment:33 by Christian Boos, 11 years ago

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: IDNameNameStartChar. I didn't follow blindly ;-)

in reply to:  33 comment:34 by Remy Blank, 11 years ago

Replying to cboos:

It's fine: IDNameNameStartChar.

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…

comment:35 by Christian Boos, 11 years ago

Resolution: fixed
Status: closedreopened

comment:36 by Christian Boos, 11 years ago

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…

in reply to:  36 comment:37 by Remy Blank, 11 years ago

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.

comment:38 by Jun Omae, 11 years ago

Cc: Jun Omae added

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>

comment:39 by Remy Blank, 11 years ago

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).

comment:40 by Christian Boos, 11 years ago

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).

by Christian Boos, 11 years ago

new version, using a sequence for the id

comment:41 by Christian Boos, 11 years ago

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.

by Christian Boos, 11 years ago

"final" version

comment:42 by osimons, 11 years ago

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.

comment:43 by Remy Blank, 11 years ago

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.

comment:44 by Christian Boos, 11 years ago

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

Applied in r10941.

comment:45 by Christian Boos, 11 years ago

Resolution: fixed
Status: reopenedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain osimons.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from osimons to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.