fix(driveprobe): only count listing entries below the collection
hasChild treated any href that was not the collection as a share, so a peer answering about an unrelated collection looked like it had shares. Follow RFC 4918 §9.1 instead: the collection comes first and anything after it is a member, with the first href counted only if it is itself below the root. Also accumulate href text across tokens; the XML decoder may split character data, which the previous token-at-a-time check miscounted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -83,12 +83,24 @@ func HasShares(ctx context.Context, c *http.Client, peerAPIURL string) (bool, er
|
|||||||
return hasChild(io.LimitReader(resp.Body, maxResponseBytes), drivePath)
|
return hasChild(io.LimitReader(resp.Body, maxResponseBytes), drivePath)
|
||||||
}
|
}
|
||||||
|
|
||||||
// hasChild reports whether a multistatus body contains a response for anything
|
// hasChild reports whether a multistatus body lists anything besides the
|
||||||
// below root. It decodes as a stream and stops at the first child, so a peer
|
// collection we asked about. It decodes as a stream and stops at the first
|
||||||
// with many shares costs no more than a peer with one.
|
// 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) {
|
func hasChild(body io.Reader, root string) (bool, error) {
|
||||||
dec := xml.NewDecoder(body)
|
dec := xml.NewDecoder(body)
|
||||||
inHref := false
|
var href string
|
||||||
|
inHref, first := false, true
|
||||||
for {
|
for {
|
||||||
tok, err := dec.Token()
|
tok, err := dec.Token()
|
||||||
if err == io.EOF {
|
if err == io.EOF {
|
||||||
@@ -99,11 +111,24 @@ func hasChild(body io.Reader, root string) (bool, error) {
|
|||||||
}
|
}
|
||||||
switch t := tok.(type) {
|
switch t := tok.(type) {
|
||||||
case xml.StartElement:
|
case xml.StartElement:
|
||||||
inHref = t.Name.Space == "DAV:" && t.Name.Local == "href"
|
if t.Name.Space == "DAV:" && t.Name.Local == "href" {
|
||||||
case xml.EndElement:
|
inHref, href = true, ""
|
||||||
inHref = false
|
}
|
||||||
case xml.CharData:
|
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
|
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
|
// 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,
|
// absolute URL or a path, percent-encoded and with or without a trailing
|
||||||
// and may or may not include the taildrive prefix we asked under, so compare
|
// slash, so compare cleaned paths rather than strings.
|
||||||
// cleaned path segments rather than strings.
|
|
||||||
func isBelow(href, root string) bool {
|
func isBelow(href, root string) bool {
|
||||||
u, err := url.Parse(strings.TrimSpace(href))
|
u, err := url.Parse(strings.TrimSpace(href))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -124,9 +148,7 @@ func isBelow(href, root string) bool {
|
|||||||
if p == r || p == "/" {
|
if p == r || p == "/" {
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
// A peer that strips the prefix answers "/share"; one that keeps it
|
return r == "/" || strings.HasPrefix(p, r+"/")
|
||||||
// answers "/v0/drive/share". Both are a share.
|
|
||||||
return true
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// HasSharesMulti probes every URL and returns one result per input, in input
|
// HasSharesMulti probes every URL and returns one result per input, in input
|
||||||
|
|||||||
@@ -40,6 +40,18 @@ func TestHasChild(t *testing.T) {
|
|||||||
{"percent-encoded share name", multistatus("/v0/drive/", "/v0/drive/my%20share"), true},
|
{"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},
|
{"unicode share name", multistatus("/v0/drive/", "/v0/drive/%E6%97%A5%E6%9C%AC"), true},
|
||||||
{"empty multistatus", multistatus(), false},
|
{"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 {
|
for _, tt := range tests {
|
||||||
t.Run(tt.name, func(t *testing.T) {
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
|||||||
Reference in New Issue
Block a user