Skip to content

fix: Allow to access path inside manifest that contains spaces #1008

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
speeddragon wants to merge 2 commits into
base: edge
Choose a base branch
from
+42 −1
Open

fix: Allow to access path inside manifest that contains spaces #1008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
09c7248
fix: Allow to access path inside manifest that contains spaces
speeddragon Jul 8, 2026
9702f94
impr: Add extra test to test + inside a key value
speeddragon Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/core/resolver/hb_singleton.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ parse_part(Part, Opts) ->
case maybe_subpath(Part, Opts) of
{resolve, Subpath} -> {resolve, Subpath};
Part ->
case part([$&, $~, $+, $ , $=], Part) of
case part([$&, $~, $+, $=], Part) of

@samcamwilliams samcamwilliams Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This was here because upstream +s were coming out as somehow. Please can you check rigorously that the + still works from browsers etc?

@speeddragon speeddragon Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Is the use case + only for type definition? Eg to+integer=3. I will try to understand how + can be used outside of this example.

@speeddragon speeddragon Jul 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've added a test to check + inside a key value (key=8+8 to be "8 8")
I think after this PR, the failing cases are:

  • Literal + in path segments: still potentially unsafe because + is syntax.
  • + in query keys: intentionally supported for typed params, but can collide with literal-space query keys.

@samcamwilliams samcamwilliams Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Literal + in path segments: still potentially unsafe because + is syntax.

This is 100% right. The + is syntax in path parts. Hard to see how we could build a non-weird, low WTFs/min scheme that supported both +type and +other stuff.

But isn't the problem upstream? Why were some paths coming with space becoming +? Or the answer is that 'whatever it was, it doesn't any more'?

@speeddragon speeddragon Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Browsers convert a plus sign (+) to a space when decoding data submitted through an HTML form with the standard application/x-www-form-urlencoded content type.

So if the page is serving a form, the value of that form will generate + instead of a space when sending the data via a GET request.

But looks like everything works as before (convert to a type, like test typed_key_test), and this just affects the split of the value inside a key.

{no_match, PartKey, <<>>} when ?IS_ID(PartKey) ->
PartKey;
{no_match, PartKey, <<>>} ->
Expand Down Expand Up @@ -718,6 +718,12 @@ typed_key_test() ->
?assertEqual(123, hb_maps:get(<<"test-key">>, Msg2, not_found)),
?assertEqual(not_found, hb_maps:get(<<"test-key">>, Res, not_found)).

query_string_plus_value_decodes_to_space_test() ->
Msgs = from(<<"/a?key=8+8">>, #{}),
?assertEqual(2, length(Msgs)),
[_, Msg] = Msgs,
?assertEqual(<<"8 8">>, hb_maps:get(<<"key">>, Msg)).

subpath_in_key_test() ->
Req = #{
<<"path">> => <<"/a/b/c">>,
Expand Down
35 changes: 35 additions & 0 deletions src/preloaded/arweave/dev_manifest.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,41 @@ manifest_404_error_test_parallel() ->
),
ok.

manifest_path_with_spaces_test_parallel() ->
Opts = #{ <<"store">> => hb_opts:get(store, no_viable_store, #{}) },
Page = #{
<<"content-type">> => <<"image/png">>,
<<"body">> => <<"Page With Spaces">>
},
{ok, PageID} = hb_cache:write(Page, Opts),
IndexPage = #{
<<"content-type">> => <<"text/html">>,
<<"body">> => <<"Index Page">>
},
{ok, IndexID} = hb_cache:write(IndexPage, Opts),
Manifest = #{
<<"paths">> => #{
<<"ID With Spaces.png">> => #{ <<"id">> => PageID },
<<"index.html">> => #{ <<"id">> => IndexID }
},
<<"index">> => #{ <<"path">> => <<"index.html">> }
},
ManifestMsg = #{
<<"device">> => <<"manifest@1.0">>,
<<"body">> => hb_json:encode(Manifest)
},
{ok, ManifestID} = hb_cache:write(ManifestMsg, Opts),
Node = hb_http_server:start_node(Opts),
?assertMatch(
{ok, #{ <<"body">> := <<"Page With Spaces">> }},
hb_http:get(
Node,
<< ManifestID/binary, "/ID%20With%20Spaces.png" >>,
Opts
)
),
ok.

create_generic_manifest(Opts) ->
IndexPage = #{
<<"content-type">> => <<"text/html">>,
Expand Down