#2128 closed defect (fixed)

comments in tahoe.cfg cause introducers to appear down

Reported by: amiller Owned by: daira
Priority: normal Milestone: 1.10.1
Component: code-nodeadmin Version: 1.10.0
Keywords: review-needed usability config Cc:
Launchpad Bug:

Description

This causes introducer to appear disconnected in webview

[client] introducer.furl = pb://censored@censored/introducer # thanks zancas!

This works fine

[client] introducer.furl = pb://censored@censored/introducer

Change History (25)

comment:1 Changed at 2013-12-05T16:10:26Z by zooko

related or possible duplicate issues: #371, #649

comment:2 Changed at 2013-12-05T16:47:34Z by amiller

So this seems to be due to the file format of ConfigParser.

http://docs.python.org/2/library/configparser.html (For backwards compatibility, only ; starts an inline comment, while # does not.)

I think a solution would be to simply throw an error if the furl contains whitespace, since furls shouldn't contain whitespace.

Last edited at 2013-12-05T21:43:08Z by zooko (previous) (diff)

comment:3 Changed at 2013-12-05T16:52:57Z by amiller

  • Keywords design-review-needed config added

comment:4 Changed at 2013-12-05T21:43:42Z by zooko

Shouldn't we also raise an exception if the furl contains a "#" ?

comment:5 Changed at 2013-12-05T22:44:25Z by daira

  • Keywords usability added

comment:7 Changed at 2013-12-06T20:51:04Z by daira

I don't think we should treat furls as a special case, since the same issue occurs for any line of the config file that has # after a field entry, if I understand correctly. We could preprocess the file to either treat such comments as an error, or remove them.

comment:8 follow-up: Changed at 2013-12-06T21:03:58Z by zooko

I propose that we modify our "config getter/setter" code to raise an exception any time a field name or value has a "#" character in it, both during read and during write.

comment:9 in reply to: ↑ 8 ; follow-up: Changed at 2013-12-07T14:59:59Z by daira

Replying to zooko:

I propose that we modify our "config getter/setter" code to raise an exception any time a field name or value has a "#" character in it, both during read and during write.

That seems reasonable. Are we sure that no field values need to contain #s? (I'm particularly thinking of fields that include syntax we haven't defined, like Twisted endpoint strings.)

comment:10 in reply to: ↑ 9 ; follow-up: Changed at 2013-12-07T15:33:16Z by zooko

Replying to daira:

Replying to zooko:

I propose that we modify our "config getter/setter" code to raise an exception any time a field name or value has a "#" character in it, both during read and during write.

That seems reasonable. Are we sure that no field values need to contain #s? (I'm particularly thinking of fields that include syntax we haven't defined, like Twisted endpoint strings.)

I'm not sure! What should we do?

comment:11 in reply to: ↑ 10 Changed at 2013-12-07T19:06:50Z by daira

Replying to zooko:

Replying to daira:

Replying to zooko: [...] Are we sure that no field values need to contain #s? (I'm particularly thinking of fields that include syntax we haven't defined, like Twisted endpoint strings.)

I'm not sure! What should we do?

I guess we first read docs/configuration.rst looking for fields that might contain #s.

comment:12 Changed at 2013-12-07T19:15:00Z by daira

Endpoint strings use \-escaping, so if we allow \#, then we're not preventing any endpoint from being expressed.

comment:13 follow-up: Changed at 2013-12-19T16:40:31Z by amiller

  • Keywords review-needed added; design-review-needed removed

I made a quick fix (that passes my test), that raise an exception whenever the introducer furl contains a # https://github.com/amiller/tahoe-lafs/commit/73eeb826bd986245e105abadc0ab50550e94e06f

Version 0, edited at 2013-12-19T16:40:31Z by amiller (next)

comment:14 in reply to: ↑ 13 Changed at 2013-12-19T16:41:22Z by amiller

Replying to amiller:

I made a quick fix (that passes my test), that raise an exception whenever the introducer furl contains a #

https://github.com/amiller/tahoe-lafs/commit/73eeb826bd986245e105abadc0ab50550e94e06f

Er, I just realized I missed the entire point of comment 12, will fix

comment:15 Changed at 2013-12-19T17:00:32Z by amiller

I now have a regular expression that tests for an unescaped # in the furl. However I don't think it's complete. The regexp is ([\^\\\\]|^)# which matches any # that is not preceded by a backslash.

My test includes the following:

        assert re.search(regexp, "test#test") is not None
        assert re.search(regexp, "#testtest") is not None
        assert re.search(regexp, "test\#test") is None
        assert re.search(regexp, "testtest") is None

Unfortunately I think the string test\\#test should probably be matched, but it isn't. I'd need to look at how the \-escaping works to be sure.

https://github.com/amiller/tahoe-lafs/commit/47694c89d0e40ca869f74a53fee9703349dca474

comment:16 follow-up: Changed at 2013-12-26T20:11:38Z by daira

I continue to think we should not treat furls as a special case (comment:7).

comment:17 in reply to: ↑ 16 ; follow-up: Changed at 2013-12-26T21:13:25Z by zooko

Replying to daira:

I continue to think we should not treat furls as a special case (comment:7).

So what if Andrew's patch were applied to all values from the config. Would that be better?

Also, daira, what should we do about a string like "test\\#test" in a value from the config?

comment:18 in reply to: ↑ 17 Changed at 2013-12-26T21:31:54Z by daira

Replying to zooko:

Replying to daira:

I continue to think we should not treat furls as a special case (comment:7).

So what if Andrew's patch were applied to all values from the config. Would that be better?

Yes, the approach in comment:8 seems like a reasonable way of doing that. (It only checks values that are used, rather than all lines of the file, but that should be fine.)

Also, daira, what should we do about a string like "test\\#test" in a value from the config?

That string doesn't contain an escaped # (only an escaped \), so it contains a #-comment and should be rejected. I don't understand the regexp in comment:15 though. (BTW, I suggest using raw r"..." strings for regexps, to avoid the extra level of \-escaping. Triple-quoted strings aren't necessary.)

comment:19 Changed at 2014-01-14T20:28:35Z by amiller

I was distracted previously by the config parser, but it's actually the twisted endpoint parser that matters. My approach in this latest patch is to emulate this function except for rejecting unescaped # - the case of r"test\\#test" is correctly rejected, for example.

Now to address comment:8, about applying this check uniformly to every endpoint string in the config... it appears that every furl in the config has a label of the form "blah.furl". Should I rely on this and put the check in node.get_config?

comment:20 Changed at 2014-02-12T00:50:46Z by amiller

I took the test that makes sure furl endpoint strings don't contain unescaped hashes, and added it to node.get_config where it will be applied uniformly to every config entry of the form something.furl. I added a new test to guarantee that it's still okay to have a hash in a nickname, for example "Hash#Bang!" is still OK to use. https://github.com/amiller/tahoe-lafs/commit/64ddd08354822d9789f15ad57b4b769ea74b328f

comment:21 Changed at 2014-03-27T22:20:34Z by daira

  • Milestone changed from undecided to 1.11.0
  • Status changed from new to assigned

comment:22 Changed at 2014-05-05T22:04:11Z by Daira Hopwood <daira@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 883dd9795bcf4f309222f4f62038537c24d0791f/trunk:

Error if a .furl config entry contains an unescaped '#'. fixes #2128

Author: Andrew Miller <amiller@…>
Signed-off-by: Daira Hopwood <daira@…>

comment:23 Changed at 2014-05-05T22:07:47Z by daira

... and [87df9d218d9b6435dbb0f93925be91a855d700e8/trunk], which was supposed to be in the same changeset but I forgot to commit it.

comment:24 Changed at 2014-05-05T22:15:01Z by Daira Hopwood <daira@…>

In efc223ad1fdefd39827b5516a37b7b5822532bbc/trunk:

Improve error reporting for '#' characters in config entries. refs #2128

Signed-off-by: Daira Hopwood <daira@…>

comment:25 Changed at 2015-04-12T23:06:57Z by daira

  • Component changed from unknown to code-nodeadmin
Note: See TracTickets for help on using tickets.