Skip to content
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.

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

Enable querystring to be encoded #5990

Merged
merged 30 commits into from Jun 7, 2021
Merged

Conversation

@BaronGreenback
Copy link
Contributor

@BaronGreenback BaronGreenback commented May 5, 2021

With this PR, url encoded querystrings can be used, and will be processed as normal.

This overcomes an issue inherent in the current DLNA implementation.

DLNA uses SOAP (which is based upon XML). Ampersands (&) present in URL's cause the XML to become invalid.
To overcome this, the JF (10.X -> 10.7.5) HtmlEncodes the url. (Strictly speaking, this isn't DLNA compliant, as URI's should be percent encoded.) It also doesn't work with all DLNA devices.

This PR uses middleware to decode any querystring where only 1 key exists without a value (this is how encoded querystrings are presented to Kestrel). From then on, it is treated as if the querystring was presented in the clear.

The solution others seem to use is to hard encode the parameters into the uri path. This isn't an option for us with the current implementation as we have numerous optional options.

@BaronGreenback BaronGreenback requested a review from crobibero May 5, 2021
@Ullmie02
Copy link
Member

@Ullmie02 Ullmie02 commented May 6, 2021

Is it possible to add tests for this?

Copy link
Member

@crobibero crobibero left a comment

Approved pending tests

@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented May 7, 2021

Interestingly dot net decodes values if they are stored in a dictionary - but not for binding!

@BaronGreenback
Copy link
Contributor Author

@BaronGreenback BaronGreenback commented May 7, 2021

Added tests

@BaronGreenback BaronGreenback requested review from crobibero and Ullmie02 May 7, 2021
@crobibero
Copy link
Member

@crobibero crobibero commented May 7, 2021

/azp run

@azure-pipelines
Copy link

@azure-pipelines azure-pipelines bot commented May 7, 2021

Azure Pipelines successfully started running 1 pipeline(s).
BaronGreenback and others added 6 commits May 7, 2021
Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Cody Robibero <cody@robibe.ro>
Co-authored-by: Cody Robibero <cody@robibe.ro>
Copy link
Member

@cvium cvium left a comment

See comment about TestsController

@@ -37,6 +37,7 @@
<ItemGroup>
<ProjectReference Include="../../Jellyfin.Api/Jellyfin.Api.csproj" />
<ProjectReference Include="../../Jellyfin.Server.Implementations/Jellyfin.Server.Implementations.csproj" />
<ProjectReference Include="..\Jellyfin.Server.Integration.Tests\Jellyfin.Server.Integration.Tests.csproj" />

This comment has been minimized.

@Bond-009

Bond-009 May 15, 2021
Member

Stay consistent and use forwards slashes everywhere

This comment has been minimized.

@BaronGreenback

BaronGreenback May 15, 2021
Author Contributor

That'll be Visual Studio running on windows.

{
private readonly JellyfinApplicationFactory _factory;

public EncodedQueryStringTest(JellyfinApplicationFactory factory)

This comment has been minimized.

@cvium

cvium May 24, 2021
Member

I'm not going to make you change it, but I think you could've used the default TestServer for this with a custom controller and registering your middleware. Using the factory is a little overkill.

@cvium
cvium approved these changes May 24, 2021
@cvium cvium requested a review from Bond-009 May 25, 2021
BaronGreenback and others added 10 commits Jun 7, 2021
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
…troller.cs

Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
…troller.cs

Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
Co-authored-by: Claus Vium <cvium@users.noreply.github.com>
@cvium
cvium approved these changes Jun 7, 2021
@cvium cvium merged commit 93387ba into jellyfin:master Jun 7, 2021
16 checks passed
16 checks passed
@github-actions
Rebase
Details
@github-actions
Labeling
Details
@github-actions
Analyze (csharp) Analyze (csharp)
Details
@github-actions
Check Backport
Details
@github-actions
Project board Project board
Details
@github-code-scanning
CodeQL No new or fixed alerts
Details
@azure-pipelines
Jellyfin Server Build #20210607.28 succeeded
Details
@azure-pipelines
Jellyfin Server (Build Debug) Build Debug succeeded
Details
@azure-pipelines
Jellyfin Server (Build Release) Build Release succeeded
Details
@azure-pipelines
Jellyfin Server (Compatibility Check Common) Compatibility Check Common succeeded
Details
@azure-pipelines
Jellyfin Server (Compatibility Check Controller) Compatibility Check Controller succeeded
Details
@azure-pipelines
Jellyfin Server (Compatibility Check Model) Compatibility Check Model succeeded
Details
@azure-pipelines
Jellyfin Server (Compatibility Check Naming) Compatibility Check Naming succeeded
Details
@azure-pipelines
Jellyfin Server (Test Linux) Test Linux succeeded
Details
@azure-pipelines
Jellyfin Server (Test Windows) Test Windows succeeded
Details
@azure-pipelines
Jellyfin Server (Test macOS) Test macOS succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants