Backport #4531 to 0.5.x#4635
Closed
pepone wants to merge 1 commit into
Closed
Conversation
There was a problem hiding this comment.
Pull request overview
Backports the adapter-id escaping fix for Slice1 service address encoding/decoding and locator resolution so adapter IDs with URI-sensitive characters are represented safely in ServiceAddress.Params.
Changes:
- Escapes Slice1-decoded adapter IDs before storing them in service address params.
- Unescapes
adapter-idparams when encoding Slice1 and when resolving locator locations. - Adds Slice proxy round-trip cases for escaped adapter-id values.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/IceRpc.Slice.Tests/ProxyTests.cs |
Adds adapter-id round-trip test cases with escaped URI characters. |
src/IceRpc.Slice/ServiceAddressSliceEncoderExtensions.cs |
Unescapes stored adapter-id params before writing Slice1 wire strings. |
src/IceRpc.Slice/ServiceAddressSliceDecoderExtensions.cs |
Escapes Slice1 wire adapter-id strings before adding them to URI params. |
src/IceRpc.Locator/LocatorInterceptor.cs |
Unescapes adapter-id params before constructing locator requests. |
src/IceRpc.Locator/Internal/LocationResolver.cs |
Unescapes adapter-id params during recursive locator resolution. |
Comments suppressed due to low confidence (1)
src/IceRpc.Locator/Internal/LocationResolver.cs:132
- This cached recursive resolution path also lacks coverage for escaped adapter IDs. The existing LocationResolver tests use 'bar', so they would pass even if this unescape call were removed; add a test that resolves a well-known service address with an escaped adapter-id and verifies the recursive lookup uses the raw adapter ID.
if (serviceAddress is not null && serviceAddress.Params.TryGetValue("adapter-id", out string? escapedAdapterId))
{
try
{
// Resolves adapter ID recursively, by checking first the cache. If we resolved the well-known
// service address, we request a cache refresh for the adapter ID.
(serviceAddress, _) = await PerformResolveAsync(
new Location { IsAdapterId = true, Value = Uri.UnescapeDataString(escapedAdapterId) },
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
+79
| [TestCase("ice:/hello?adapter-id=foo%23bar", null, SliceEncoding.Slice1)] // '#' | ||
| [TestCase("ice:/hello?adapter-id=foo%25bar", null, SliceEncoding.Slice1)] // '%' | ||
| [TestCase("ice:/hello?adapter-id=foo%20bar", null, SliceEncoding.Slice1)] // ' ' | ||
| [TestCase("ice:/hello?adapter-id=foo%23bar%25baz%20qux%26quux", null, SliceEncoding.Slice1)] // '#', '%', ' ', '&' |
Comment on lines
+60
to
+61
| location = request.ServiceAddress.Params.TryGetValue("adapter-id", out string? escapedAdapterId) ? | ||
| new Location { IsAdapterId = true, Value = Uri.UnescapeDataString(escapedAdapterId) } : |
Comment on lines
+55
to
+58
| if (serviceAddress is not null && serviceAddress.Params.TryGetValue("adapter-id", out string? escapedAdapterId)) | ||
| { | ||
| (serviceAddress, _) = await ResolveAsync( | ||
| new Location { IsAdapterId = true, Value = adapterId }, | ||
| new Location { IsAdapterId = true, Value = Uri.UnescapeDataString(escapedAdapterId) }, |
Comment on lines
221
to
+223
| if (decoder.DecodeString() is string adapterId && adapterId.Length > 0) | ||
| { | ||
| serviceAddressParams = serviceAddressParams.Add("adapter-id", adapterId); | ||
| serviceAddressParams = serviceAddressParams.Add("adapter-id", Uri.EscapeDataString(adapterId)); |
Member
There was a problem hiding this comment.
Copilot is right here, we over-escape.
Putting this over-escaping bug aside, I am wondering if this bug-fix is truly semver-compatible.
Say your 0.5.1 application receives an adapter ID with an "invalid" adapter ID (what we're fixing here). It's still a proper string that you can use and compare with a hard-coded string literal safely. It's undesirable for a patch release to change this decoded string.
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
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.
Backport of #4531 — escape adapter-id value in
ServiceAddressparams.Summary
The
adapter-idvalue decoded from a wire-format ice service address was inserted into the URI parameter string unescaped, allowing URI-component injection when the adapter-id contains#,&,%, or space (extra params, broken fragment, locator-resolution surprises). The fix:Uri.EscapeDataString(adapterId)when adding toParams.Uri.UnescapeDataString(escapedAdapterId)when reading back out.LocationResolver,LocatorInterceptor): unescape on read so theLocation.Valueis the raw adapter-id string the locator sees.Test plan
dotnet test --filter "FullyQualifiedName~ProxyTests"(IceRpc.Slice.Tests) — 38/38 pass (4 new adapter-id round-trip cases).dotnet test tests/IceRpc.Locator.Tests— 44/44 pass.Backport notes
Cherry-pick of 2385211. Adaptations:
src/IceRpc.Ice/Operations/IceProxyIceDecoderExtensions.cs/IceProxyIceEncoderExtensions.cson main — both live undersrc/IceRpc.Slice/ServiceAddressSliceDecoderExtensions.cs/ServiceAddressSliceEncoderExtensions.cson 0.5.x. Ported by hand to the two equivalent call sites.LocationResolver.cs,LocatorInterceptor.cs) merged cleanly.tests/IceRpc.Ice.Tests/ProxyTests.cs(a file that doesn't exist on 0.5.x). Added them totests/IceRpc.Slice.Tests/ProxyTests.cs:Decode_proxyinstead, withSliceEncoding.Slice1to exercise the same code path.Sibling Tier B PR sharing this file: #4634 (backport of #4548, wrap URI decode errors) — whichever lands first, the other will need a trivial rebase.