Skip to content

fix(webdav): normalize percent-encoding case so MKCOL matches an existing PUT - #421

Merged
davidwkeith merged 2 commits into
mainfrom
claude/next-priorities-lsbmae
Jul 24, 2026
Merged

fix(webdav): normalize percent-encoding case so MKCOL matches an existing PUT#421
davidwkeith merged 2 commits into
mainfrom
claude/next-priorities-lsbmae

Conversation

@davidwkeith

Copy link
Copy Markdown
Owner

Summary

The 2026-07-23 hosted litmus run left @dwk/webdav's conformance status at
failing (conformance/status.json, conformance/webdav-qa.md): the
basic group failed on mkcol_over_plain — an MKCOL naming the same
UTF-8 segment put_get_utf8_segment had just created as a plain resource
succeeded instead of refusing with 405. Because the packaged litmus binary
stops after the first failing group, copymove/props/locks never ran
at all as a result.

Root cause: pathOf resolved every request's path straight from
URL#pathname, which copies an already-percent-encoded triplet through
verbatim rather than normalizing its case. put_get_utf8_segment and
mkcol_over_plain name the same UTF-8 segment but litmus's own request
construction gives the two requests different percent-encoding hex case for
it (e.g. %e2%82%ac vs %E2%82%AC) — RFC 3986 §2.1 says these are the same
octets, but the backend's exact-string-match stat() lookup in mkcol()
didn't treat them that way, so it missed the existing resource and let the
MKCOL through.

Fix: pathOf now uppercases every percent-encoded triplet before the
resolved path is used anywhere downstream (backend calls, authorization,
lock/precondition checks), so encoding-case no longer affects resource
identity.

This is not yet re-verified against the hosted targetstatus.json
and the QA doc's Result table intentionally stay at failing/pending
until a fresh litmus dispatch confirms basic passes and lets
copymove/props/locks run for the first time. conformance/webdav-qa.md
has a new "Follow-up" section documenting the fix and that next step.

Packages affected

@dwk/webdav

Checklist

  • Read the relevant spec(s) under spec/packages/ and updated them if
    behaviour changed — not applicable, no behaviour change to the
    documented contract, just a conformance bug fix
  • Added/updated colocated tests (src/*.test.ts)
  • Ran the local CI gate: pnpm lint && pnpm format:check && pnpm typecheck && pnpm build && pnpm test
  • Added a changeset (pnpm changeset) if this touches a publishable
    package
  • Updated catalog.json / conformance/status.json if this adds a new
    mountable worker or changes conformance status — not applicable yet;
    status.json stays failing/pending until a fresh hosted litmus
    run confirms the fix (see conformance/webdav-qa.md's new Follow-up
    section)

Generated by Claude Code

… that created a resource

litmus mkcol_over_plain (reusing put_get_utf8_segment's resource) sends the
same UTF-8 segment with different percent-encoding hex case across the two
requests. pathOf resolved paths straight from URL#pathname, which copies an
already-encoded triplet through verbatim, so the two requests produced
different path strings and missed each other in the backend's exact-match
lookup, letting MKCOL succeed over an existing plain resource instead of
405ing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQrUELXYwiEKoRv3vnfFdX

Copy link
Copy Markdown
Owner Author

The github-advanced-security check failure isn't related to this diff — its job log shows GitHub's own Copilot Autofind security-review bot erroring out server-side before it analyzed anything:

Error creating PR review request: SessionModelError: Execution failed: CAPIError: 400
{"error":{"message":"The requested model is not supported.","code":"model_not_supported", ...}}

That's the Autofind runtime failing to start a Copilot session with sweagent-capi:claude-opus-4.6 — an infrastructure/config issue on GitHub's side, not a finding about packages/webdav/src/webdav.ts. The real code-scanning analysis (CodeQL / Analyze (javascript-typescript)) is separate and unaffected. No action to take here; will keep an eye on whether it clears on its own or needs a manual rerun.


Generated by Claude Code

@davidwkeith davidwkeith left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the diff and verified locally on claude/next-priorities-lsbmae (ba42259):

  • pnpm --filter @dwk/webdav build, typecheck, prettier --check, eslint, and pnpm test --project @dwk/webdav (127 tests) all pass.
  • The root cause and fix are correct: URL#pathname copies percent-encoded triplets through verbatim without case-normalizing them, so PUT /res-%e2%82%ac and MKCOL /res-%E2%82%AC resolved to different path strings even though RFC 3986 §2.1 treats them as the same octets. Uppercasing every triplet in pathOf before any downstream use (backend calls, authorize, lock checks, destinationOf) fixes this consistently for every verb, not just MKCOL/PUT. The new colocated test reproduces the exact litmus scenario.

One minor nit, non-blocking: normalizePercentEncoding is applied to the request pathname in pathOf, but resolved.mountPrefix/resolved.storageRoot (derived from config.mountPath/config.baseUrl in resolve()) are never normalized the same way. If a deployment ever configured a mount path containing a percent-encoded segment, the prefix-match against the now-normalized pathname could silently fail. In practice mount paths are plain-ASCII developer config, so this is very unlikely to bite — just flagging for completeness rather than asking for a change.

CONTRIBUTING.md conformance: PR title (fix(webdav): normalize percent-encoding case so MKCOL matches an existing PUT) matches Conventional Commits style with correct scope; colocated test added; changeset added (patch for @dwk/webdav); PR body keeps the Summary/Packages affected/Checklist headings verbatim from the template, and the two unchecked boxes (spec update, status.json/catalog) each carry a one-line reason rather than being silently deleted, consistent with the "leave unchecked with a reason" rule. Looks good to merge once the CI checks (still in progress as of this review) go green.


Generated by Claude Code

…path too

resolve() left config.mountPath/baseUrl's percent-encoding case as given
while pathOf normalized every request path, so a percent-encoded mount
segment could itself drift out of sync with a differently-cased request and
404 spuriously. Apply the same normalization in resolve().

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQrUELXYwiEKoRv3vnfFdX

Copy link
Copy Markdown
Owner Author

Thanks for the review. Fixed the mount-path nit in 3a10cef rather than leaving it — it was cheap and closes a real (if unlikely) gap: resolve() now runs config.mountPath/baseUrl through the same normalizePercentEncoding as pathOf, so a percent-encoded mount segment can't itself drift out of sync with a differently-cased request. Added a colocated test (webdav.test.ts) that configures a percent-encoded mount path and PUTs against it with different hex case, confirming it resolves instead of 404ing. pnpm test --project @dwk/webdav (128 tests), typecheck, build, lint, and format all pass; re-ran @dwk/solid-pod's suite too (185 tests) since it consumes this package.

On the github-advanced-security failure: still unrelated to this diff, per my earlier comment — it's GitHub's Copilot Autofind bot hitting a server-side model_not_supported error before it ever analyzed anything.


Generated by Claude Code

@davidwkeith
davidwkeith merged commit e6eab17 into main Jul 24, 2026
8 of 9 checks passed
@davidwkeith
davidwkeith deleted the claude/next-priorities-lsbmae branch July 24, 2026 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants