Modify

Opened 5 years ago

Closed 5 years ago

#5760 closed defect (worksforme)

CustomFieldAdminPlugin should validate input parameters

Reported by: eblot Owned by: osimons
Priority: highest Component: CustomFieldAdminPlugin
Severity: normal Keywords:
Cc: Trac Release: 0.11

Description

The plugin does not check for non-ASCII characters in the custom field values ("select box", at least)

It then updates the Trac configuration values with unsupported character and asks Trac to save the new configuration to the configuration file.

Trac does not handle such characters, and destroy the configuration file as an exception is raised while overwriting the existing configuration file.

I'll update this ticket with the trac related ticket when/if trac.edgewall.org comes back to life.

Attachments (0)

Change History (18)

comment:1 Changed 5 years ago by eblot

comment:2 follow-up: Changed 5 years ago by osimons

Well, actually it does verify - the plugin ensures that all items can be encoded to UTF-8. This was done as [5161] from similar report in #4524.

You are of course running latest 0.11 version of the plugin?

comment:3 in reply to: ↑ 2 Changed 5 years ago by eblot

Replying to osimons:

You are of course running latest 0.11 version of the plugin?

I'm running TracCustomFieldAdmin-0.2-py2.5.egg, which seems to match (0.2) the version in the setup.py file.

comment:4 Changed 5 years ago by eblot

Wow, the setup.py file has not been updated for 2 years, while the plugin code is 7 month old, so it's hard to tell which version I actually run.

comment:5 Changed 5 years ago by osimons

I've bumped the version for you - and version should now also appear in 'About Trac'. See [6469].

Could you try verifying your version again?

comment:6 follow-up: Changed 5 years ago by cboos

[5161] produces UTF-8 encoded str, and this seems to be fed into the Config API which expects unicode objects (as nearly any other Trac API). This is quite wrong.

comment:7 in reply to: ↑ 6 ; follow-ups: Changed 5 years ago by osimons

Replying to cboos:

[5161] produces UTF-8 encoded str, and this seems to be fed into the Config API which expects unicode objects (as nearly any other Trac API). This is quite wrong.

Well, that is a matter for discussion as configuration has until a few changesets ago (in 0.11.6dev) not handled unicode well at all. Passing unicode would result in the mentioned error when config would write them to disk - if the unicode contained anything but plain ascii. I pass in utf-8, which gets saved, causing config to reload with unicode values. Remember custom fields also affects the options in the configs (keys), and not just their values.

comment:8 Changed 5 years ago by osimons

Actually, I have other safety mechanisms in place as well to ensure that custom field names does not contain non-ascii characters... Hmm... Don't remember all the details anymore :-)

comment:9 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by eblot

Replying to osimons:

Well, that is a matter for discussion as configuration has until a few changesets ago (in 0.11.6dev) not handled unicode well at all. Passing unicode would result in the mentioned error when config would write them to disk - if the unicode contained anything but plain ascii. I pass in utf-8, which gets saved, causing config to reload with unicode values. Remember custom fields also affects the options in the configs (keys), and not just their values.

AFAIR, it was decided a long time ago that all internal strings in Trac should be unicode only.

IMHO, the Configuration module should load and store trac.ini file with UTF-8 encoding. I think there is an old ticket related with this topic: how to specify the encoding of trac.ini file, I can't remember which one.

comment:10 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by cboos

Replying to osimons:

Remember custom fields also affects the options in the configs (keys), and not just their values.

Ah yes, that's indeed a problem. But the fact that converting both keys and values to utf-8 just before saving the configuration has worked at all was just luck.

I understand that changing back to unicode wouldn't work with earlier 0.11 versions, but neither will the current code work with 0.11.6. That's a bit unfortunate, we should maybe have fixed #8162 for trunk only. Would it be possible to check for the Trac version?

comment:11 in reply to: ↑ 9 ; follow-up: Changed 5 years ago by cboos

Replying to eblot:

AFAIR, it was decided a long time ago that all internal strings in Trac should be unicode only.

... which doesn't mean that unicode related bugs still surface from time to time. #T8162 was one of them.

IMHO, the Configuration module should load and store trac.ini file with UTF-8 encoding.

That's already what it does.

I think there is an old ticket related with this topic: how to specify the encoding of trac.ini file, I can't remember which one.

I don't remember having seen this.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by eblot

Replying to cboos:

#T8162 was one of them.

Are you sure about this ticket ID?

IMHO, the Configuration module should load and store trac.ini file with UTF-8 encoding.

That's already what it does.

Ok, I got confused.

I don't remember having seen this.

I'm unable to find the reference either, but I'm pretty sure it was left as an open question ;-)

comment:13 in reply to: ↑ 12 Changed 5 years ago by cboos

Replying to eblot:

Replying to cboos:

#T8162 was one of them.

Are you sure about this ticket ID?

Bad day for ticket references today ... I meant #T8276.

comment:14 in reply to: ↑ 10 ; follow-ups: Changed 5 years ago by osimons

Replying to cboos:

I understand that changing back to unicode wouldn't work with earlier 0.11 versions, but neither will the current code work with 0.11.6. That's a bit unfortunate, we should maybe have fixed #8162 for trunk only. Would it be possible to check for the Trac version?

Not quite true - I'm running latest plugin (unchanged for a long time) with very latest 0.11.6dev. Ascii or non-ascii for values, and nothing like eblots problem is seen at my end. All works fine for me.

eblot, you are on latest plugin version now right? And you can reproduce the problem? Remember to backup configuration file... :-)

comment:15 in reply to: ↑ 14 Changed 5 years ago by eblot

Replying to osimons:

eblot, you are on latest plugin version now right? And you can reproduce the problem? Remember to backup configuration file... :-)

Very, very busy right now, will try soon. Hopefully, my configuration files are backed up once a day ;-)

comment:16 in reply to: ↑ 14 Changed 5 years ago by anonymous

Replying to osimons:

Replying to cboos:

I understand that changing back to unicode wouldn't work with earlier 0.11 versions, but neither will the current code work with 0.11.6. That's a bit unfortunate, we should maybe have fixed #8162 for trunk only. Would it be possible to check for the Trac version?

Not quite true - I'm running latest plugin (unchanged for a long time) with very latest 0.11.6dev. Ascii or non-ascii for values, and nothing like eblots problem is seen at my end. All works fine for me.

Ok, that's probably because there are a bunch of to_unicode calls in various places in config.py ;-)
Still, maybe the 0.12 version of the plugin could try to use unicode only, then.

comment:17 Changed 5 years ago by eblot

Replying to osimons:

eblot, you are on latest plugin version now right? And you can reproduce the problem? Remember to backup configuration file... :-)

Ok, I've updated to the 0.2.2 version, and I cannot reproduce this issue anymore. Thanks for your help.

comment:18 Changed 5 years ago by eblot

  • Resolution set to worksforme
  • Status changed from new to closed

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.