encoding/xml: reject XML declaration after start of document

The XML specification requires an XML declaration, if present, to only
appear at the very beginning of the document, not even preceded by
whitespace. The parser currently accepts it at any part of the input.

Rejecting whitespace at the beginning of the file might break too many
users. This change instead only rejects an XML declaration preceded by
a non-whitespace token *and* allows the Encoder to emit whitespace
before an XML declaration. This means that a token stream produced by
the Decoder can be passed to the Encoder without error, while we still
don't emit clearly invalid XML.

This might break programs depending on Decoder allowing arbitrary XML
before the XML declaration.

Fixes #65691.

Change-Id: Ib1d4b3116aee63f40fd377f90595780b4befd1ee
Reviewed-on: https://go-review.googlesource.com/c/go/+/564035
Auto-Submit: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>

diff --git a/src/encoding/xml/marshal.go b/src/encoding/xml/marshal.go
index 05b5542dfb41626b77974b902aadc584d17f7b4d..c30f0dca1514dfb809d28213b8445478aa2fb933 100644 (file)
--- a/src/encoding/xml/marshal.go
+++ b/src/encoding/xml/marshal.go
@@ -208,7 +208,6 @@ var (
 // EncodeToken allows writing a [ProcInst] with Target set to "xml" only as the first token
 // in the stream.
 func (enc *Encoder) EncodeToken(t Token) error {
-
        p := &enc.p
        switch t := t.(type) {
        case StartElement:
@@ -228,11 +227,10 @@ func (enc *Encoder) EncodeToken(t Token) error {
                p.WriteString("<!--")
                p.Write(t)
                p.WriteString("-->")
-               return p.cachedWriteError()
        case ProcInst:
                // First token to be encoded which is also a ProcInst with target of xml
                // is the xml declaration. The only ProcInst where target of xml is allowed.
-               if t.Target == "xml" && p.w.Buffered() != 0 {
+               if t.Target == "xml" && p.wroteNonWS {
                        return fmt.Errorf("xml: EncodeToken of ProcInst xml target only valid for xml declaration, first token encoded")
                }
                if !isNameString(t.Target) {
@@ -257,9 +255,30 @@ func (enc *Encoder) EncodeToken(t Token) error {
                p.WriteString(">")
        default:
                return fmt.Errorf("xml: EncodeToken of invalid token type")
+       }
+       if err := p.cachedWriteError(); err != nil || enc.p.wroteNonWS {
+               return err
+       }
+       enc.p.wroteNonWS = !isWhitespace(t)
+       return nil
+}
 
+// isWhitespace reports whether t is a CharData token consisting entirely of
+// XML whitespace.
+func isWhitespace(t Token) bool {
+       switch t := t.(type) {
+       case CharData:
+               for _, b := range t {
+                       switch b {
+                       case ' ', '\r', '\n', '\t':
+                       default:
+                               return false
+                       }
+               }
+               return true
+       default:
+               return false
        }
-       return p.cachedWriteError()
 }
 
 // isValidDirective reports whether dir is a valid directive text,
@@ -329,6 +348,7 @@ type printer struct {
        prefixes   []string
        tags       []Name
        closed     bool
+       wroteNonWS bool
        err        error
 }
 

diff --git a/src/encoding/xml/marshal_test.go b/src/encoding/xml/marshal_test.go
index f6bcc7fd30287b0bafe68dc9c1a5e4c0438e3790..90922f549e21fd00e5fdf68604cec3164d6ab5d4 100644 (file)
--- a/src/encoding/xml/marshal_test.go
+++ b/src/encoding/xml/marshal_test.go
@@ -2356,6 +2356,9 @@ func TestProcInstEncodeToken(t *testing.T) {
        var buf bytes.Buffer
        enc := NewEncoder(&buf)
 
+       if err := enc.EncodeToken(CharData(" \n\r\t")); err != nil {
+               t.Fatal("enc.EncodeToken: expected to be able to encode whitespace as first token")
+       }
        if err := enc.EncodeToken(ProcInst{"xml", []byte("Instruction")}); err != nil {
                t.Fatalf("enc.EncodeToken: expected to be able to encode xml target ProcInst as first token, %s", err)
        }

diff --git a/src/encoding/xml/xml.go b/src/encoding/xml/xml.go
index 6b8f2e79784fa1f35799848610f4c23284dd6c6f..a1e63ed30d39db682dea2924107d3618142da052 100644 (file)
--- a/src/encoding/xml/xml.go
+++ b/src/encoding/xml/xml.go
@@ -212,6 +212,7 @@ type Decoder struct {
        line           int
        linestart      int64
        offset         int64
+       readNonWS      bool
        unmarshalDepth int
 }
 
@@ -559,6 +560,8 @@ func (d *Decoder) rawToken() (Token, error) {
                return EndElement{d.toClose}, nil
        }
 
+       readNonWS := d.readNonWS
+
        b, ok := d.getc()
        if !ok {
                return nil, d.err
@@ -571,8 +574,12 @@ func (d *Decoder) rawToken() (Token, error) {
                if data == nil {
                        return nil, d.err
                }
+               if !d.readNonWS && !isWhitespace(CharData(data)) {
+                       d.readNonWS = true
+               }
                return CharData(data), nil
        }
+       d.readNonWS = true
 
        if b, ok = d.mustgetc(); !ok {
                return nil, d.err
@@ -623,6 +630,11 @@ func (d *Decoder) rawToken() (Token, error) {
                data = data[0 : len(data)-2] // chop ?>
 
                if target == "xml" {
+                       if readNonWS {
+                               d.err = errors.New("xml: XML declaration after start of document")
+                               return nil, d.err
+                       }
+
                        content := string(data)
                        ver := procInst("version", content)
                        if ver != "" && ver != "1.0" {

diff --git a/src/encoding/xml/xml_test.go b/src/encoding/xml/xml_test.go
index 4bec4e7f1e9d49dd26b2dd505fdfd6629804253d..2c985f7c70f8327cc3500f9cb11416f53259e300 100644 (file)
--- a/src/encoding/xml/xml_test.go
+++ b/src/encoding/xml/xml_test.go
@@ -1350,10 +1350,12 @@ func TestParseErrors(t *testing.T) {
 
                // Header-related errors.
                {`<?xml version="1.1" encoding="UTF-8"?>`, `unsupported version "1.1"; only version 1.0 is supported`},
+               {`<foo><?xml version="1.0"?>`, `XML declaration after start of document`},
 
                // Cases below are for "no errors".
                {withDefaultHeader(`<?ok?>`), ``},
                {withDefaultHeader(`<?ok version="ok"?>`), ``},
+               {`  <?xml version="1.0"?>`, ``},
        }
 
        for _, test := range tests {