diff --git a/cmd/tsconnect/driveprobe/driveprobe.go b/cmd/tsconnect/driveprobe/driveprobe.go index ac80cb13e..70cc56bc7 100644 --- a/cmd/tsconnect/driveprobe/driveprobe.go +++ b/cmd/tsconnect/driveprobe/driveprobe.go @@ -83,12 +83,24 @@ func HasShares(ctx context.Context, c *http.Client, peerAPIURL string) (bool, er return hasChild(io.LimitReader(resp.Body, maxResponseBytes), drivePath) } -// hasChild reports whether a multistatus body contains a response for anything -// below root. It decodes as a stream and stops at the first child, so a peer -// with many shares costs no more than a peer with one. +// hasChild reports whether a multistatus body lists anything besides the +// collection we asked about. It decodes as a stream and stops at the first +// child, so a peer with many shares costs no more than a peer with one. +// +// RFC 4918 §9.1 says a Depth-1 PROPFIND answers with the collection itself +// followed by its members, so anything after the first href is a share. The +// first href counts only if it is itself below root, which catches a peer that +// answers about a subtree rather than the collection we asked for. +// +// A peer that omits the collection entirely is not understood: ipnlocal strips +// the taildrive prefix before handing the request to the share server, so a +// real peer's members are named "/docs" rather than "/v0/drive/docs" and a lone +// member is indistinguishable from the collection. Such a peer is reported as +// having no shares, which is the safe direction for a positive filter. func hasChild(body io.Reader, root string) (bool, error) { dec := xml.NewDecoder(body) - inHref := false + var href string + inHref, first := false, true for { tok, err := dec.Token() if err == io.EOF { @@ -99,11 +111,24 @@ func hasChild(body io.Reader, root string) (bool, error) { } switch t := tok.(type) { case xml.StartElement: - inHref = t.Name.Space == "DAV:" && t.Name.Local == "href" - case xml.EndElement: - inHref = false + if t.Name.Space == "DAV:" && t.Name.Local == "href" { + inHref, href = true, "" + } case xml.CharData: - if inHref && isBelow(string(t), root) { + // Character data can arrive in several tokens for one element. + if inHref { + href += string(t) + } + case xml.EndElement: + if !inHref { + continue + } + inHref = false + if !first { + return true, nil + } + first = false + if isBelow(href, root) { return true, nil } } @@ -111,9 +136,8 @@ func hasChild(body io.Reader, root string) (bool, error) { } // isBelow reports whether href points below root. Peers may answer with an -// absolute URL or a path, percent-encoded, with or without a trailing slash, -// and may or may not include the taildrive prefix we asked under, so compare -// cleaned path segments rather than strings. +// absolute URL or a path, percent-encoded and with or without a trailing +// slash, so compare cleaned paths rather than strings. func isBelow(href, root string) bool { u, err := url.Parse(strings.TrimSpace(href)) if err != nil { @@ -124,9 +148,7 @@ func isBelow(href, root string) bool { if p == r || p == "/" { return false } - // A peer that strips the prefix answers "/share"; one that keeps it - // answers "/v0/drive/share". Both are a share. - return true + return r == "/" || strings.HasPrefix(p, r+"/") } // HasSharesMulti probes every URL and returns one result per input, in input diff --git a/cmd/tsconnect/driveprobe/driveprobe_test.go b/cmd/tsconnect/driveprobe/driveprobe_test.go index 647a80663..4265a943e 100644 --- a/cmd/tsconnect/driveprobe/driveprobe_test.go +++ b/cmd/tsconnect/driveprobe/driveprobe_test.go @@ -40,6 +40,18 @@ func TestHasChild(t *testing.T) { {"percent-encoded share name", multistatus("/v0/drive/", "/v0/drive/my%20share"), true}, {"unicode share name", multistatus("/v0/drive/", "/v0/drive/%E6%97%A5%E6%9C%AC"), true}, {"empty multistatus", multistatus(), false}, + // The collection comes first per RFC 4918 §9.1, so anything after it + // is a share whatever the peer names it. + {"unrelated collection href, no members", multistatus("/somewhere/else/"), false}, + {"unrelated collection href with a member", multistatus("/somewhere/else/", "/somewhere/else/docs"), true}, + // A peer that omits the collection from a Depth-1 listing violates + // RFC 4918 §9.1, and once the taildrive prefix is stripped there is + // nothing left to tell its lone member apart from the collection. It + // loses the benefit of the doubt: hasShares excludes what it cannot + // confirm. + {"single member, collection omitted", multistatus("/docs"), false}, + {"href split by an entity reference", multistatus("/v0/drive/", "/v0/drive/a&b"), true}, + {"empty href", multistatus("/v0/drive/", ""), true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {