Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wrapped ${mailbox.name} in "s to allow for space in mailbox names. #34629

Merged
merged 1 commit into from Feb 5, 2018
Merged

Wrapped ${mailbox.name} in "s to allow for space in mailbox names. #34629

merged 1 commit into from Feb 5, 2018

Conversation

phdoerfler
Copy link
Contributor

Motivation for this change

Without this fix the following nix expression:

{
  name = "Trashy McTrashface";
  auto = "subscribe";
  specialUse = "Trash";
}

becomes

mailbox Trashy McTrashface {
  auto = subscribe
  special_use = \Trash
}

but it should be

mailbox "Trashy McTrashface" {
  auto = subscribe
  special_use = \Trash
}

or else dovecot2 won't start

Feb 05 16:00:28 nixos dovecot2-pre-start[20738]: doveconf: Fatal: Error in configuration file /etc/dovecot/dovecot.conf line 36: Expecting '{'

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@phdoerfler phdoerfler mentioned this pull request Feb 5, 2018
Merged
@FRidh FRidh merged commit 5083c1d into NixOS:master Feb 5, 2018
@FRidh
Copy link
Member

FRidh commented Feb 5, 2018

Thanks. Also backported to stable as 5e35f55.

dotlambda
dotlambda reviewed Feb 5, 2018
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't an additional escape [ ''"'' ] be applied?

@phdoerfler
Copy link
Contributor Author

phdoerfler commented Feb 6, 2018 •
edited

@dotlambda Turns out dovecot does not specify how to escape these. Escaping via \" does not work. The documentation only states (AFAIK):

The mailbox section name specifies the mailbox name. If it has spaces, you can put it in "quotes".

From https://wiki.dovecot.org/MailboxSettings.

@FRidh
Copy link
Member

FRidh commented Feb 6, 2018

Has a fix been submitted? Don't forget to also backport.

@FRidh FRidh self-assigned this Feb 6, 2018
@dotlambda
Copy link
Member

How about asserting mailbox.name doesn't contain "?

@phdoerfler
Copy link
Contributor Author

@dotlambda @FRidh Unfortunately I don't have the time to work on this any more for the time being. This being said I think the check for " is a good idea.

@dotlambda
Copy link
Member

Okay, I'll implement the check and make a PR

@dotlambda
Copy link
Member

I think this should be the easiest way:

diff --git a/nixos/modules/services/mail/dovecot.nix b/nixos/modules/services/mail/dovecot.nix
index c5cb06cf54e..1408de4e162 100644
--- a/nixos/modules/services/mail/dovecot.nix
+++ b/nixos/modules/services/mail/dovecot.nix
@@ -113,7 +113,7 @@ let
   mailboxes = { lib, pkgs, ... }: {
     options = {
       name = mkOption {
-        type = types.str;
+        type = types.strMatching ''[^"]+'';
         example = "Spam";
         description = "The name of the mailbox.";
       };

What do you think?

@dotlambda dotlambda mentioned this pull request Feb 9, 2018
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda dotlambda left review comments

Assignees

@FRidh FRidh

Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@phdoerfler @FRidh @dotlambda @GrahamcOfBorg