wrappers: move to regexp for desktop entry validation

canonical · Dec 13, 2016 · f83fc5c · f83fc5c
1 parent 417e629
commit f83fc5c
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 109 deletions.
diff --git a/wrappers/desktop.go b/wrappers/desktop.go
@@ -27,6 +27,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"github.com/snapcore/snapd/dirs"
@@ -35,98 +36,71 @@ import (
 	"github.com/snapcore/snapd/snap"
 )
 
-// valid simple prefixes
-var validDesktopFilePrefixes = []string{
+// From the freedesktop Desktop Entry Specification¹,
+//
+//    Keys with type localestring may be postfixed by [LOCALE], where
+//    LOCALE is the locale type of the entry. LOCALE must be of the form
+//    lang_COUNTRY.ENCODING@MODIFIER, where _COUNTRY, .ENCODING, and
+//    @MODIFIER may be omitted. If a postfixed key occurs, the same key
+//    must be also present without the postfix.
+//
+//    When reading in the desktop entry file, the value of the key is
+//    selected by matching the current POSIX locale for the LC_MESSAGES
+//    category against the LOCALE postfixes of all occurrences of the
+//    key, with the .ENCODING part stripped.
+//
+// sadly POSIX doesn't mention what values are valid for LC_MESSAGES,
+// beyond mentioning² that it's implementation-defined (and can be of
+// the form [language[_territory][.codeset][@modifier]])
+//
+// 1. https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s04.html
+// 2. http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_02
+//
+// So! The following is simplistic, and based on the contents of
+// PROVIDED_LOCALES in locales.config, and does not cover all of
+// "locales -m" (and ignores XSetLocaleModifiers(3), which may or may
+// not be related). Patches welcome, as long as it's readable.
+//
+// REVIEWERS: this could also be left as `(?:\[[@_.A-Za-z-]\])?=` if even
+// the following is hard to read:
+const localizedSuffix = `(?:\[[a-z]+(?:_[A-Z]+)?(?:\.[0-9A-Z-]+)?(?:@[a-z]+)?\])?=`
+
+var isValidDesktopFileLine = regexp.MustCompile(strings.Join([]string{
+	// NOTE (mostly to self): as much as possible keep the
+	// individual regexp simple, optimize for legibility
+	//
+	// empty lines and comments
+	`^\s*$`,
+	`^\s*#`,
 	// headers
-	"[Desktop Entry]",
-	"[Desktop Action ",
+	`^\[Desktop Entry\]$`,
+	`^\[Desktop Action [0-9A-Za-z-]+\]$`,
+	`^\[[A-Za-z0-9-]+ Shortcut Group\]$`,
 	// https://specifications.freedesktop.org/desktop-entry-spec/latest/ar01s05.html
-	"Type=",
-	"Version=",
-	"Name=",
-	"GenericName=",
-	"NoDisplay=",
-	"Comment=",
-	"Icon=",
-	"Hidden=",
-	"OnlyShowIn=",
-	"NotShowIn=",
-	"Exec=",
+	"^Type=",
+	"^Version=",
+	"^Name" + localizedSuffix,
+	"^GenericName" + localizedSuffix,
+	"^NoDisplay=",
+	"^Comment" + localizedSuffix,
+	"^Icon=",
+	"^Hidden=",
+	"^OnlyShowIn=",
+	"^NotShowIn=",
+	"^Exec=",
 	// Note that we do not support TryExec, it does not make sense
 	// in the snap context
-	"Terminal=",
-	"Actions=",
-	"MimeType=",
-	"Categories=",
-	"Keywords=",
-	"StartupNotify=",
-	"StartupWMClass=",
+	"^Terminal=",
+	"^Actions=",
+	"^MimeType=",
+	"^Categories=",
+	"^Keywords" + localizedSuffix,
+	"^StartupNotify=",
+	"^StartupWMClass=",
 	// unity extension
-	"X-Ayatana-Desktop-Shortcuts=",
-	"TargetEnvironment=",
-}
-
-var validDesktopFileSuffixes = []string{
-	"Shortcut Group]",
-}
-
-// name desktop file keys are localized as key[LOCALE]=:
-//   lang_COUNTRY@MODIFIER
-//   lang_COUNTRY
-//   lang@MODIFIER
-//   lang
-var validLocalizedDesktopFilePrefixes = []string{
-	"Name",
-	"GenericName",
-	"Comment",
-	"Keywords",
-}
-
-func isValidDesktopFilePrefix(line string) bool {
-	for _, prefix := range validDesktopFilePrefixes {
-		if strings.HasPrefix(line, prefix) {
-			return true
-		}
-	}
-
-	return false
-}
-
-func trimLang(s string) string {
-	const langChars = "@_ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
-
-	if s == "" || s[0] != '[' {
-		return s
-	}
-	t := strings.TrimLeft(s[1:], langChars)
-	if t != "" && t[0] == ']' {
-		return t[1:]
-	}
-	return s
-}
-
-func isValidDesktopFileSuffix(line string) bool {
-	for _, prefix := range validDesktopFileSuffixes {
-		if strings.HasSuffix(line, prefix) {
-			return true
-		}
-	}
-
-	return false
-}
-
-func isValidLocalizedDesktopFilePrefix(line string) bool {
-	for _, prefix := range validLocalizedDesktopFilePrefixes {
-		s := strings.TrimPrefix(line, prefix)
-		if s == line {
-			continue
-		}
-		if strings.HasPrefix(trimLang(s), "=") {
-			return true
-		}
-	}
-	return false
-}
+	"^X-Ayatana-Desktop-Shortcuts=",
+	"^TargetEnvironment=",
+}, "|")).MatchString
 
 // rewriteExecLine rewrites a "Exec=" line to use the wrapper path for snap application.
 func rewriteExecLine(s *snap.Info, desktopFile, line string) (string, error) {
@@ -155,16 +129,10 @@ func sanitizeDesktopFile(s *snap.Info, desktopFile string, rawcontent []byte) []
 	for scanner.Scan() {
 		line := scanner.Text()
 
-		// whitespace/comments are just copied
-		if strings.TrimSpace(line) == "" || strings.HasPrefix(strings.TrimSpace(line), "#") {
-			newContent = append(newContent, line)
+		if !isValidDesktopFileLine(line) {
 			continue
 		}
 
-		// ignore everything we have not whitelisted
-		if !isValidDesktopFilePrefix(line) && !isValidLocalizedDesktopFilePrefix(line) && !isValidDesktopFileSuffix(line) {
-			continue
-		}
 		// rewrite exec lines to an absolute path for the binary
 		if strings.HasPrefix(line, "Exec=") {
 			var err error

diff --git a/wrappers/desktop_test.go b/wrappers/desktop_test.go
@@ -278,23 +278,30 @@ apps:
 	c.Assert(newl, Equals, fmt.Sprintf("Exec=env BAMF_DESKTOP_FILE_HINT=foo.desktop %s/bin/snap.app", dirs.SnapMountDir))
 }
 
-func (s *sanitizeDesktopFileSuite) TestTrimLang(c *C) {
+func (s *sanitizeDesktopFileSuite) TestLangLang(c *C) {
 	langs := []struct {
 		in  string
-		out string
+		out bool
 	}{
 		// langCodes
-		{"[lang_COUNTRY@MODIFIER]=foo", "=foo"},
-		{"[lang_COUNTRY]=bar", "=bar"},
-		{"[lang_COUNTRY]=baz", "=baz"},
-		{"[lang]=foobar", "=foobar"},
-		// non-langCodes, should be ignored
-		{"", ""},
-		{"Name=foobar", "Name=foobar"},
-		// corner case
-		{"[foo=bar", "[foo=bar"},
+		{"Name[lang]=lang-alone", true},
+		{"Name[_COUNTRY]=country-alone", false},
+		{"Name[.ENC-0DING]=encoding-alone", false},
+		{"Name[@modifier]=modifier-alone", false},
+		{"Name[lang_COUNTRY]=lang+country", true},
+		{"Name[lang.ENC-0DING]=lang+encoding", true},
+		{"Name[lang@modifier]=lang+modifier", true},
+		// could also test all bad combos of 2, and all combos of 3...
+		{"Name[lang_COUNTRY.ENC-0DING@modifier]=all", true},
+		// other localised entries
+		{"GenericName[xx]=a", true},
+		{"Comment[xx]=b", true},
+		{"Keywords[xx]=b", true},
+		// bad ones
+		{"Name[foo=bar", false},
+		{"Icon[xx]=bar", false},
 	}
 	for _, t := range langs {
-		c.Assert(wrappers.TrimLang(t.in), Equals, t.out)
+		c.Assert(wrappers.IsValidDesktopFileLine(t.in), Equals, t.out)
 	}
 }
diff --git a/wrappers/export_test.go b/wrappers/export_test.go
@@ -30,9 +30,9 @@ var (
 	GenerateSnapSocketFile  = generateSnapSocketFile
 
 	// desktop
-	SanitizeDesktopFile = sanitizeDesktopFile
-	RewriteExecLine     = rewriteExecLine
-	TrimLang            = trimLang
+	SanitizeDesktopFile    = sanitizeDesktopFile
+	RewriteExecLine        = rewriteExecLine
+	IsValidDesktopFileLine = isValidDesktopFileLine
 )
 
 func MockKillWait(wait time.Duration) (restore func()) {