libstore: Fix pct-encoding issues in store references#15280
Merged
Conversation
github-actions
bot
added
new-cli
xokdvium
force-pushed
the
local-store-filesystem
branch
from
9aae32b to
523d479
Compare
Ericson2314
approved these changes
Feb 18, 2026
Contributor
Author
|
Has issues building on mingw for now |
Ericson2314
force-pushed
the
local-store-filesystem
branch
2 times, most recently
from
19474c3 to
7e9ae05
Compare
github-actions
bot
added
the
repl
Ericson2314
force-pushed
the
local-store-filesystem
branch
from
7e9ae05 to
481d605
Compare
Member
Ericson2314
reviewed
Feb 25, 2026
src/libstore/binary-cache-store.cc
Outdated
|
|
||
| try { | ||
| getFile(info->url, *decompressor); | ||
| getFile(CanonPath{"/" + info->url}, *decompressor); |
Member
There was a problem hiding this comment.
IIRC @xokdvium and I had seen an example in the Harmonia integration test of an absolute URL being used here? That would not break. We might want to double check?
Contributor
Author
There was a problem hiding this comment.
They weren't absolute. Stuff like nar-bridge from snix uses relative references with query parameters:
curl: [HTTP/2] [13] [:method: GET]
curl: [HTTP/2] [13] [:scheme: https]
curl: [HTTP/2] [13] [:authority: nixos.snix.store]
curl: [HTTP/2] [13] [:path: /nar/snix-castore/CiUSIDx38iUK-ZuIbulax1ctEWOhHRUp25ujusl-vopC2DF_GJYC?narsize=511440]
curl: [HTTP/2] [13] [user-agent: curl/8.18.0 Nix/2.34.0pre20260226_db06ac4]
curl: [HTTP/2] [13] [accept: */*]
curl: [HTTP/2] [13] [accept-encoding: deflate, gzip, br, zstd]
xokdvium
commented
Feb 25, 2026
|
|
||
| void HttpBinaryCacheStore::upsertFile( | ||
| const std::string & path, RestartableSource & source, const std::string & mimeType, uint64_t sizeHint) | ||
| const CanonPath & path, RestartableSource & source, const std::string & mimeType, uint64_t sizeHint) |
Contributor
Author
There was a problem hiding this comment.
Pretty sure the API must not change. We can use CanonPath just in the local binary cache store implementation though
Ericson2314
force-pushed
the
local-store-filesystem
branch
4 times, most recently
from
54bd57f to
5adf9b0
Compare
github-actions
bot
added
fetching
Ericson2314
force-pushed
the
local-store-filesystem
branch
from
5adf9b0 to
667ad46
Compare
Contributor
Author
|
@Ericson2314, I'm a bit apprehensive about using CanonPath because the string might have query parameters too and that's certainly the wrong type for it. I have a less intrusive solution for now so that we can unblock the fixes. |
xokdvium
force-pushed
the
local-store-filesystem
branch
from
667ad46 to
a4ebebe
Compare
When the common pattern for store config constructors is to accept an arbitrary string it's unclear whether it needs pct-decoding or not. Prior to c436b7a the string passed to the constructors was a mix of encoded authority and decoded path concatenated with a `/`. After that commit it accidentally started accepting pct-encoded result of renderAuthorityAndPath, but only in some code paths. This lead to file:///tmp/a+b to be created on disk in /tmp/a%2Bb directory. Similar issue affected the less-known variant with local:///tmp/a+b. Regular store references that are local paths were not affected. This patch changes the constructors to accept different types to signify what is actually needed to let the factory method handle this in a consistent way: - std::filesystem::path - local binary cache store and local store - ParsedURL::Authority - for ssh stores - ParsedURL - for http stores (Some MinGW build fixes by Amaan Qureshi <git@amaanq.com>)
xokdvium
force-pushed
the
local-store-filesystem
branch
from
a4ebebe to
224c818
Compare
Member
|
@xokdvium OK fair point with the query params in the path case. |
Ericson2314
approved these changes
Feb 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
When the common pattern for store config constructors is to accept
an arbitrary string it's unclear whether it needs pct-decoding or not.
Prior to c436b7a the string passed to
the constructors was a mix of encoded authority and decoded path concatenated
with a
/. After that commit it accidentally started accepting pct-encodedresult of
renderAuthorityAndPath, but only in some code paths. This lead tofile:///tmp/a+bto be created on disk in/tmp/a%2Bbdirectory. Similar issueaffected the less-known variant with
local:///tmp/a+b. Regular store referencesthat are local paths were not affected.
This patch changes the constructors to accept different types to signify what
is actually needed to let the factory method handle this in a consistent way:
std::filesystem::path- local binary cache store and local storeParsedURL::Authority- for ssh storesParsedURL- for http storesContext
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.