Fix usage of Url::join. #380
Merged
Conversation
This file contains 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
This exercises a current bug in Janus where it can't interoperate with an aggregator that is not serving out of the "base" of its domain (i.e. "https://example.com/" will work, "https://example.com/dap/" will not). The test currently fails; the following commit will fix this bug & make the test pass. This was missed because all existing tests of sufficient complexity to potentially exercise a bug in aggregator endpoint handling were testing against endpoints set to the base of a domain.
This is required to fix a *different* footgun in URL than the one causing our non-base-domain heartache: if a URL doesn't end in a slash, Url::join will, rather than joining the paths, replace the final path element with the additional path elements. I also update deserialization to go through the constructor -- I believe all constructed Task values now go through the constructor. I don't fix that the aggregator_endpoints field is pub -- hypothetically someone that owns a Task value could mutate the field. We should probably do as other types do & make all fields private with getters created as necessary.
Janus joined absolute paths to aggregator endpoint URLs in several locations. Url::join treats an absolute path as replacing the existing path. This is not documented [1], nor do any of the documentation's examples show this behavior, but the behavior is demonstrated by [2]. Due to this, Janus would remove any existing path from an aggregator endpoint before sending requests of the affected types, effectively breaking interoperation. [1] https://docs.rs/url/2.2.2/url/struct.Url.html#method.join [2] https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=6b80dd9c27d87613afda5e2e440441cd
divergentdave
approved these changes
Aug 11, 2022
tgeoghegan
requested changes
Aug 11, 2022
There was a problem hiding this comment.
We should put a similar check in ClientParameters::new, since Url::join is also used to construct the URLs that the client will access for HPKE configs and uploads.
We do this for the same reason we ensure the aggregator endpoints in a Task end in a slash.
tgeoghegan
approved these changes
Aug 11, 2022
Comment on lines
+63
to
+70
| // Ensure provided aggregator endpoints end with a slash, as we will be joining additional | ||
| // path segments into these endpoints & the Url::join implementation is persnickety about | ||
| // the slash at the end of the path. | ||
| for url in &mut aggregator_endpoints { | ||
| if !url.as_str().ends_with('/') { | ||
| url.set_path(&format!("{}/", url.path())); | ||
| } | ||
| } |
There was a problem hiding this comment.
Meganit: we could put an extension trait on Url in janus_core that defines a method Url::normalize that does this. Then again this code probably won't change so we can live with a little copy-pasta.
There was a problem hiding this comment.
I see your point, but I'm OK eating the duplication for something this simple -- IMO the direct readability win is worth the minor duplication. If the common functionality was more than ~three lines of code, was likely to change, etc I would definitely avoid the repetition somehow.
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.
There are fixes to two separate unexpected behaviors of the Url::join method:
I also update the Janus-Janus monolithic integration test to exercise the previously-untested case of an aggregator endpoint which has a nontrivial path.