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

Already on GitHub? Sign in to your account

Specified the contentType for different requests to prevent errors when clients supply their own contentTypes #1692

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

abnanda1 commented Mar 14, 2013

  • Specified the contentType as "application/x-www-form-urlencoded" for the /negotiate, /send, /abort and /ping requests
    #947
Owner

davidfowl commented Mar 14, 2013

Why are all requests form URL encoded? That looks wrong even tho it works the content type should reflect the type of request.

Contributor

NTaylorMullen commented Mar 14, 2013

Can we verify that JQuery doesn't auto-detect content type if it's still default? I'm worried about it auto-detecting and therefore causing our current implementations to behave properly, essentially what David's concerned about

Contributor

NTaylorMullen commented Mar 14, 2013

Also add functional tests for this to verify it does not fail when a user uses ajaxSetup in their own code.

Owner

davidfowl commented Mar 14, 2013

@NTaylorMullen I was thinking that we could change it in our unit tests and see if anything fails but it's just too global.

Contributor

NTaylorMullen commented Mar 14, 2013

@davidfowl we'd reset it all back to its initial defaults at the end of the test

@davidfowl davidfowl commented on the diff Mar 15, 2013

...t.SignalR.Tests.Common/Connections/MyBadConnection.cs
@@ -5,6 +5,11 @@ namespace Microsoft.AspNet.SignalR.FunctionalTests
{
public class MyBadConnection : PersistentConnection
{
+ public override Task ProcessRequest(Hosting.HostContext context)

@davidfowl davidfowl commented on an outdated diff Mar 15, 2013

....Tests.Common/Connections/ExamineRequestConnection.cs
@@ -0,0 +1,17 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+
+namespace Microsoft.AspNet.SignalR.Tests.Common.Connections
+{
+ public class ExamineRequestConnection : PersistentConnection
+ {
+ public override Task ProcessRequest(Hosting.HostContext context)

@davidfowl davidfowl commented on the diff Mar 15, 2013

...AspNet.SignalR.Hosting.AspNet.Samples/Scripts/hubs.js
@@ -424,6 +424,12 @@
ping: function () {
/// <summary>Calls the Ping method on the server-side StatusHub hub.&#10;Returns a jQuery.Deferred() promise.</summary>
return proxies.StatusHub.invoke.apply(proxies.StatusHub, $.merge(["Ping"], $.makeArray(arguments)));
+ },
+
+ sendMessage: function (msg) {

@davidfowl davidfowl commented on the diff Mar 15, 2013

...et.SignalR.Hosting.AspNet.Samples/Raw/TestPagePC.html
@@ -0,0 +1,30 @@
+<!DOCTYPE html>
@davidfowl

davidfowl Mar 15, 2013

Owner

why is this committed?

@davidfowl davidfowl commented on the diff Mar 15, 2013

...t.SignalR.Hosting.AspNet.Samples/Raw/TestPageHub.html
@@ -0,0 +1,39 @@
@davidfowl

davidfowl Mar 15, 2013

Owner

Why is this committed?

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

...sting.AspNet.Samples/Hubs/ConnectDisconnect/Status.cs
@@ -27,5 +27,19 @@ public void Ping()
{
Clients.Caller.pong();
}
+
+ public Task SendMessage(string msg)
+ {
+ // Clients.All.addMessage(msg).wait();
+ Person p = new Person();
+ p.Name = "John"; p.Age=12;
+ return Clients.All.addPerson(p);
+ }
+
+ class Person
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

private class

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

...icrosoft.AspNet.SignalR.Hosting.AspNet.Samples.csproj
@@ -163,6 +163,8 @@
<Content Include="Hubs\ShapeShare\ShapeShare.js" />
<Content Include="Hubs\ShapeShare\index.htm" />
<Content Include="Raw\Default.aspx" />
+ <Content Include="Raw\TestPageHub.html" />
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

What are these two files?

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

....SignalR.Hosting.AspNet.Samples/Raw/TestConnection.cs
using System.Threading.Tasks;
namespace Microsoft.AspNet.SignalR.Samples
{
public class TestConnection : PersistentConnection
{
- protected override Task OnReceived(IRequest request, string connectionId, string data)
+ public static string NegotiateRequestType {
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

{ get; private set; }

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

....SignalR.Hosting.AspNet.Samples/Raw/TestConnection.cs
using System.Threading.Tasks;
namespace Microsoft.AspNet.SignalR.Samples
{
public class TestConnection : PersistentConnection
{
- protected override Task OnReceived(IRequest request, string connectionId, string data)
+ public static string NegotiateRequestType {
+ get;
+ private set; }
+ public static string PingRequestType { get; private set; }
+
+ public override Task ProcessRequest(Hosting.HostContext context)
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

Add "using Microsoft.AspNet.SignalR.Hosting;" to the top and remove the "Hosting."

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

....SignalR.Hosting.AspNet.Samples/Raw/TestConnection.cs
+ {
+ NegotiateRequestType = context.Request.Headers.GetValues("Content-Type")[0];
+ }
+
+ else if (IsPingRequest(context.Request))
+ {
+ PingRequestType = context.Request.Headers.GetValues("Content-Type")[0];
+ }
+
+ return base.ProcessRequest(context);
+ }
+
+ protected override Task OnConnected(IRequest request, string connectionId)
+ {
+ return Connection.Send(connectionId, NegotiateRequestType);
+ // return base.OnConnected(request, connectionId);
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

Remove this line

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

....SignalR.Hosting.AspNet.Samples/Raw/TestConnection.cs
+
+ return base.ProcessRequest(context);
+ }
+
+ protected override Task OnConnected(IRequest request, string connectionId)
+ {
+ return Connection.Send(connectionId, NegotiateRequestType);
+ // return base.OnConnected(request, connectionId);
+ }
+
+ private static bool IsNegotiationRequest(IRequest request)
+ {
+ return request.Url.LocalPath.EndsWith("/negotiate", StringComparison.OrdinalIgnoreCase);
+ }
+
+ private static bool IsPingRequest(IRequest request)
{
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

When are you telling the client about the ping request header type?

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

...sts/Tests/FunctionalTests/Transports/All/SendFacts.js
@@ -36,4 +36,21 @@ testUtilities.runWithAllTransports(function (transport) {
connection.stop();
};
});
+
+ QUnit.asyncTimeoutTest(transport + " contentType set correctly ", testUtilities.defaultTestTimeout, function (end, assert, testName) {
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

remove trailing space in test name, add period

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

...sts/Tests/FunctionalTests/Transports/All/SendFacts.js
@@ -36,4 +36,21 @@ testUtilities.runWithAllTransports(function (transport) {
connection.stop();
};
});
+
+ QUnit.asyncTimeoutTest(transport + " contentType set correctly ", testUtilities.defaultTestTimeout, function (end, assert, testName) {
+ var connection = testUtilities.createConnection("examine-request", end, assert, testName);
+
+ connection.received(function (data) {
+ // assert.equal(typeof (data), "string", "contentType set correctly in the reuqest");
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

Remove this line

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

....Tests/Tests/UnitTests/Transports/LongPollingFacts.js
+});
+
+QUnit.test("Content Type set correctly", function () {

@NTaylorMullen NTaylorMullen commented on the diff Mar 15, 2013

....Tests.Common/Connections/ExamineRequestConnection.cs
@@ -0,0 +1,45 @@
+using System;
@NTaylorMullen

NTaylorMullen Mar 15, 2013

Contributor

Same comments here as that are on TestConnection. But why are there two connections, what's the second one for?

@abnanda1 abnanda1 closed this Mar 15, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment