Skip to content

Store objects for an upstream key with a dot in it, such as gitlab.com [patch] - #109

Open
matt-edmondson wants to merge 1 commit into
mainfrom
fix/56-dotted-upstream-keys
Open

matt-edmondson wants to merge 1 commit into
mainfrom
fix/56-dotted-upstream-keys

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

Fixes #56

What was wrong

ObjectStore only accepted upstream keys made of [A-Za-z0-9_-] (IsValidUpstream / IsUpstreamCharacter) and threw ArgumentException for anything else. Startup validation, UpstreamRegistry and LfsRouteParser all accept such a key. So with an upstream named gitlab.com, the batch call succeeded and then every object download and every upload returned 500.

Change

This follows the maintainer decision on the issue: map the key to a safe directory name in ObjectStore, and don't reject dotted keys at startup.

  • ObjectStore.DirectoryNameFor maps a key to its directory name:
    • A key made only of ASCII letters, digits, - and _ is its own name. That is the layout existing stores already have, so they stay readable.
    • Any other key has each UTF-8 byte outside that set written as %XX, so gitlab.com becomes gitlab%2Ecom. The mapping is deterministic and reversible, so it is collision-free. An escaped name always contains a %, which a plain name never does. It leaves no separator or .. that could reach outside the store root.
  • ObjectPath and StagingDirectory use the mapped name. That covers OpenStaging, OpenRead, PublishAsync, Touch and Exists. Enumerate maps the name back to the key (UpstreamFor).
  • Only an empty key is still refused.
  • The storage section of the design spec describes the mapping.

Tests

  • ProxyFlowTests.DottedUpstreamKey_DownloadsAndUploadsAreStored runs a ProxyFixture with a gitlab.com upstream. It checks that a download and an upload both return 200 and both land in the store, which is the test the decision asked for.
  • ObjectStoreTests:
    • AnyNonEmptyUpstream_IsStoredInOneDirectoryUnderTheRootAndReadBack covers gitlab.com, ../escape, with/slash, a backslash, .. and a non-ASCII key. Each must store and read back, sit in exactly one directory directly under the root, and enumerate under its original key.
    • DirectoryNameFor_PlainKey_IsTheKeyItself checks that existing layouts are unchanged.
    • DirectoryNameFor_DottedKey_EscapesOnlyTheDot and DirectoryNameFor_KeysThatLookAlikeOnceEscaped_StayDistinct cover gitlab.com, gitlab%2Ecom, gitlab%252Ecom, gitlab_com and gitlab-com.
  • OpenStaging_MalformedUpstream_Throws becomes OpenStaging_EmptyUpstream_Throws. Its ../escape and with/slash rows are now stored safely, which the test above checks, rather than refused.

With the fix reverted, 8 cases fail, including the ProxyFixture test. The full suite passes locally: 363 tests, 0 failed. The branch merges cleanly with #101, #102, #103, #104, #107 and #108.

🤖 Generated with Claude Code

https://claude.ai/code/session_01WY7QGzbH6kE2oceF1hP8fh


Generated by Claude Code

…m [patch]

ObjectStore accepted only upstream keys made of [A-Za-z0-9_-] and threw
ArgumentException for anything else, though startup validation, the
registry and the route parser all accept such a key. With an upstream
named gitlab.com the batch call worked and then every object download and
upload returned 500.

The store now maps a key to its directory name instead of rejecting it. A
plain key is its own name, so existing stores stay readable. Any other key
has each UTF-8 byte outside that set written as %XX, which is deterministic
and reversible, cannot collide with a plain name since it always contains a
%, and leaves no separator or parent reference that could reach outside the
root. Enumerate maps the name back to the key. Only an empty key is still
refused.

Fixes #56

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WY7QGzbH6kE2oceF1hP8fh
@sonarqubecloud

sonarqubecloud Bot commented Oct 8, 2026

Copy link
Copy Markdown

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.

Every object download and upload returns 500 when the upstream key contains a dot (e.g. gitlab.com), though startup validation and batch calls accept it

2 participants