Merge pull request #3560 from DanielMorsing/quoteidents

be more strict about identifier printing

cmd/influxd/run/server_test.go

@@ -3066,3 +3066,45 @@ func TestServer_Query_CreateContinuousQuery(t *testing.T) {
 		}
 	}
 }
+
+func TestServer_Query_EvilIdentifiers(t *testing.T) {
+	t.Parallel()
+	s := OpenServer(NewConfig(), "")
+	defer s.Close()
+
+	if err := s.CreateDatabaseAndRetentionPolicy("db0", newRetentionPolicyInfo("rp0", 1, 0)); err != nil {
+		t.Fatal(err)
+	}
+	if err := s.MetaStore.SetDefaultRetentionPolicy("db0", "rp0"); err != nil {
+		t.Fatal(err)
+	}
+
+	test := NewTest("db0", "rp0")
+	test.write = fmt.Sprintf("cpu select=1,in-bytes=2 %d", mustParseTime(time.RFC3339Nano, "2000-01-01T00:00:00Z").UnixNano())
+
+	test.addQueries([]*Query{
+		&Query{
+			name:    `query evil identifiers`,
+			command: `SELECT "select", "in-bytes" FROM cpu`,
+			exp:     `{"results":[{"series":[{"name":"cpu","columns":["time","in-bytes","select"],"values":[["2000-01-01T00:00:00Z",2,1]]}]}]}`,
+			params:  url.Values{"db": []string{"db0"}},
+		},
+	}...)
+
+	for i, query := range test.queries {
+		if i == 0 {
+			if err := test.init(s); err != nil {
+				t.Fatalf("test init failed: %s", err)
+			}
+		}
+		if query.skip {
+			t.Logf("SKIP:: %s", query.name)
+			continue
+		}
+		if err := query.Execute(s); err != nil {
+			t.Error(query.Error(err))
+		} else if !query.success() {
+			t.Error(query.failureMessage())
+		}
+	}
+}

influxql/ast.go

@@ -1992,26 +1992,6 @@ func (f *Field) Name() string {
 func (f *Field) String() string {
 	str := f.Expr.String()
 
-	switch f.Expr.(type) {
-	case *VarRef:
-		quoted := false
-		// Escape any double-quotes in the field
-		if strings.Contains(str, `"`) {
-			str = strings.Replace(str, `"`, `\"`, -1)
-			quoted = true
-		}
-
-		// Escape any single-quotes in the field
-		if strings.Contains(str, `'`) {
-			quoted = true
-		}
-
-		// Double-quote field names with spaces or that were previously escaped
-		if strings.Contains(str, " ") || quoted {
-			str = fmt.Sprintf("\"%s\"", str)
-		}
-	}
-
 	if f.Alias == "" {
 		return str
 	}
@@ -2132,7 +2112,9 @@ type VarRef struct {
 }
 
 // String returns a string representation of the variable reference.
-func (r *VarRef) String() string { return r.Val }
+func (r *VarRef) String() string {
+	return QuoteIdent(r.Val)
+}
 
 // Call represents a function call.
 type Call struct {

influxql/ast_test.go

@@ -44,35 +44,35 @@ func TestSelectStatement_Substatement(t *testing.T) {
 		{
 			stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb`,
 			expr: &influxql.VarRef{Val: "aa.value"},
-			sub:  `SELECT aa.value FROM aa`,
+			sub:  `SELECT "aa.value" FROM aa`,
 		},
 
 		// 2. Simple merge
 		{
 			stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb`,
 			expr: &influxql.VarRef{Val: "bb.value"},
-			sub:  `SELECT bb.value FROM bb`,
+			sub:  `SELECT "bb.value" FROM bb`,
 		},
 
 		// 3. Join with condition
 		{
 			stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb WHERE aa.host = 'servera' AND bb.host = 'serverb'`,
 			expr: &influxql.VarRef{Val: "bb.value"},
-			sub:  `SELECT bb.value FROM bb WHERE bb.host = 'serverb'`,
+			sub:  `SELECT "bb.value" FROM bb WHERE "bb.host" = 'serverb'`,
 		},
 
 		// 4. Join with complex condition
 		{
 			stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb WHERE aa.host = 'servera' AND (bb.host = 'serverb' OR bb.host = 'serverc') AND 1 = 2`,
 			expr: &influxql.VarRef{Val: "bb.value"},
-			sub:  `SELECT bb.value FROM bb WHERE (bb.host = 'serverb' OR bb.host = 'serverc') AND 1.000 = 2.000`,
+			sub:  `SELECT "bb.value" FROM bb WHERE ("bb.host" = 'serverb' OR "bb.host" = 'serverc') AND 1.000 = 2.000`,
 		},
 
 		// 5. 4 with different condition order
 		{
 			stmt: `SELECT sum(aa.value) + sum(bb.value) FROM aa, bb WHERE ((bb.host = 'serverb' OR bb.host = 'serverc') AND aa.host = 'servera') AND 1 = 2`,
 			expr: &influxql.VarRef{Val: "bb.value"},
-			sub:  `SELECT bb.value FROM bb WHERE ((bb.host = 'serverb' OR bb.host = 'serverc')) AND 1.000 = 2.000`,
+			sub:  `SELECT "bb.value" FROM bb WHERE (("bb.host" = 'serverb' OR "bb.host" = 'serverc')) AND 1.000 = 2.000`,
 		},
 	}
 

influxql/parser.go

@@ -2217,6 +2217,11 @@ func QuoteIdent(segments ...string) string {
 
 // IdentNeedsQuotes returns true if the ident string given would require quotes.
 func IdentNeedsQuotes(ident string) bool {
+	// check if this identifier is a keyword
+	tok := Lookup(ident)
+	if tok != IDENT {
+		return true
+	}
 	for i, r := range ident {
 		if i == 0 && !isIdentFirstChar(r) {
 			return true

influxql/parser_test.go

@@ -1664,6 +1664,8 @@ func TestQuoteIdent(t *testing.T) {
 		s     string
 	}{
 		{[]string{``}, ``},
+		{[]string{`select`}, `"select"`},
+		{[]string{`in-bytes`}, `"in-bytes"`},
 		{[]string{`foo`, `bar`}, `"foo".bar`},
 		{[]string{`foo`, ``, `bar`}, `"foo"..bar`},
 		{[]string{`foo bar`, `baz`}, `"foo bar".baz`},