Fixed IAuthenticationRequest.AddCallbackArgument so that it won't throw if it can't sign arguments.

Signature checks are done in retrieval of arguments if the RP uses GetCallbackArgument.

Fixes Trac #127

7 files changed, 38 insertions, 26 deletions

diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/AuthenticationRequest.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/AuthenticationRequest.cs
index 9912d0b..cea7d21 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/AuthenticationRequest.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/AuthenticationRequest.cs
@@ -189,17 +189,17 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		/// </summary>
 		/// <param name="arguments">The arguments to add to the request's return_to URI.</param>
 		/// <remarks>
-		/// 	<para>Note that these values are NOT protected against tampering in transit.  No
-		/// security-sensitive data should be stored using this method.</para>
+		/// 	<para>Note that these values are NOT protected against eavesdropping in transit.  No
+		/// privacy-sensitive data should be stored using this method.</para>
 		/// 	<para>The values stored here can be retrieved using
-		/// <see cref="IAuthenticationResponse.GetCallbackArguments"/>.</para>
+		/// <see cref="IAuthenticationResponse.GetCallbackArguments"/>, which will only return the value
+		/// if it hasn't been tampered with in transit.</para>
 		/// 	<para>Since the data set here is sent in the querystring of the request and some
 		/// servers place limits on the size of a request URL, this data should be kept relatively
 		/// small to ensure successful authentication.  About 1.5KB is about all that should be stored.</para>
 		/// </remarks>
 		public void AddCallbackArguments(IDictionary<string, string> arguments) {
 			ErrorUtilities.VerifyArgumentNotNull(arguments, "arguments");
-			ErrorUtilities.VerifyOperation(this.RelyingParty.CanSignCallbackArguments, OpenIdStrings.CallbackArgumentsRequireSecretStore, typeof(IAssociationStore<Uri>).Name, typeof(OpenIdRelyingParty).Name);
 
 			foreach (var pair in arguments) {
 				ErrorUtilities.VerifyArgument(!string.IsNullOrEmpty(pair.Key), MessagingStrings.UnexpectedNullOrEmptyKey);
@@ -215,10 +215,11 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		/// <param name="key">The parameter name.</param>
 		/// <param name="value">The value of the argument.</param>
 		/// <remarks>
-		/// 	<para>Note that these values are NOT protected against tampering in transit.  No
-		/// security-sensitive data should be stored using this method.</para>
+		/// 	<para>Note that these values are NOT protected against eavesdropping in transit.  No
+		/// privacy-sensitive data should be stored using this method.</para>
 		/// 	<para>The value stored here can be retrieved using
-		/// <see cref="IAuthenticationResponse.GetCallbackArgument"/>.</para>
+		/// <see cref="IAuthenticationResponse.GetCallbackArgument"/>, which will only return the value
+		/// if it hasn't been tampered with in transit.</para>
 		/// 	<para>Since the data set here is sent in the querystring of the request and some
 		/// servers place limits on the size of a request URL, this data should be kept relatively
 		/// small to ensure successful authentication.  About 1.5KB is about all that should be stored.</para>
@@ -226,7 +227,6 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		public void AddCallbackArguments(string key, string value) {
 			ErrorUtilities.VerifyNonZeroLength(key, "key");
 			ErrorUtilities.VerifyArgumentNotNull(value, "value");
-			ErrorUtilities.VerifyOperation(this.RelyingParty.CanSignCallbackArguments, OpenIdStrings.CallbackArgumentsRequireSecretStore, typeof(IAssociationStore<Uri>).Name, typeof(OpenIdRelyingParty).Name);
 
 			this.returnToArgs.Add(key, value);
 		}
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationRequest.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationRequest.cs
index 8414031..c97654a 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationRequest.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationRequest.cs
@@ -98,10 +98,11 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		/// </summary>
 		/// <param name="arguments">The arguments to add to the request's return_to URI.  Values must not be null.</param>
 		/// <remarks>
-		/// 	<para>Note that these values are NOT protected against tampering in transit.  No
-		/// security-sensitive data should be stored using this method.</para>
+		/// 	<para>Note that these values are NOT protected against eavesdropping in transit.  No
+		/// privacy-sensitive data should be stored using this method.</para>
 		/// 	<para>The values stored here can be retrieved using
-		/// <see cref="IAuthenticationResponse.GetCallbackArguments"/>.</para>
+		/// <see cref="IAuthenticationResponse.GetCallbackArgument"/>, which will only return the value
+		/// if it can be verified as untampered with in transit.</para>
 		/// 	<para>Since the data set here is sent in the querystring of the request and some
 		/// servers place limits on the size of a request URL, this data should be kept relatively
 		/// small to ensure successful authentication.  About 1.5KB is about all that should be stored.</para>
@@ -114,10 +115,11 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		/// <param name="key">The parameter name.</param>
 		/// <param name="value">The value of the argument.  Must not be null.</param>
 		/// <remarks>
-		/// 	<para>Note that these values are NOT protected against tampering in transit.  No
-		/// security-sensitive data should be stored using this method.</para>
+		/// 	<para>Note that these values are NOT protected against eavesdropping in transit.  No
+		/// privacy-sensitive data should be stored using this method.</para>
 		/// 	<para>The value stored here can be retrieved using
-		/// <see cref="IAuthenticationResponse.GetCallbackArgument"/>.</para>
+		/// <see cref="IAuthenticationResponse.GetCallbackArgument"/>, which will only return the value
+		/// if it can be verified as untampered with in transit.</para>
 		/// 	<para>Since the data set here is sent in the querystring of the request and some
 		/// servers place limits on the size of a request URL, this data should be kept relatively
 		/// small to ensure successful authentication.  About 1.5KB is about all that should be stored.</para>
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationResponse.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationResponse.cs
index cc94de0..3ab48db 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationResponse.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/IAuthenticationResponse.cs
@@ -104,10 +104,11 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		/// The value of the argument, or null if the named parameter could not be found.
 		/// </returns>
 		/// <remarks>
-		/// 	<para>This may return any argument on the querystring that came with the authentication response,
-		/// which may include parameters not explicitly added using
-		/// <see cref="IAuthenticationRequest.AddCallbackArguments(string, string)"/>.</para>
-		/// 	<para>Note that these values are NOT protected against tampering in transit.</para>
+		/// Callback parameters are only available if they are complete and untampered with
+		/// since the original request message (as proven by a signature).
+		/// If the relying party is operating in stateless mode <c>null</c> is always
+		/// returned since the callback arguments could not be signed to protect against
+		/// tampering.
 		/// </remarks>
 		string GetCallbackArgument(string key);
 
@@ -118,13 +119,14 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 		/// </summary>
 		/// <returns>A name-value dictionary.  Never null.</returns>
 		/// <remarks>
-		/// <para>This MAY return any argument on the querystring that came with the authentication response,
-		/// which may include parameters not explicitly added using 
-		/// <see cref="IAuthenticationRequest.AddCallbackArguments(string, string)"/>.</para>
-		/// <para>Note that these values are NOT protected against tampering in transit.</para>
+		/// Callback parameters are only available if they are complete and untampered with
+		/// since the original request message (as proven by a signature).
+		/// If the relying party is operating in stateless mode an empty dictionary is always
+		/// returned since the callback arguments could not be signed to protect against
+		/// tampering.
 		/// </remarks>
 		[SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification = "Historically an expensive operation.")]
-		IDictionary<string, string> GetCallbackArguments(); // TODO: change this to a property, and return a cached ReadOnlyDictionary
+		IDictionary<string, string> GetCallbackArguments();
 
 		/// <summary>
 		/// Tries to get an OpenID extension that may be present in the response.
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdMobileTextBox.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdMobileTextBox.cs
index a917e24..2fa2109 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdMobileTextBox.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdMobileTextBox.cs
@@ -635,7 +635,9 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 
 			var response = this.RelyingParty.GetResponse();
 			if (response != null) {
-				string persistentString = response.GetCallbackArgument(UsePersistentCookieCallbackKey);
+				// We don't use response.GetCallbackArgument because it will return null in stateless
+				// mode since the arg may have been tampered with, but this isn't a security decision.
+				string persistentString = this.Page.Request.QueryString[UsePersistentCookieCallbackKey];
 				bool persistentBool;
 				if (persistentString != null && bool.TryParse(persistentString, out persistentBool)) {
 					this.UsePersistentCookie = persistentBool;
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyControlBase.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyControlBase.cs
index 8451dbd..703dda0 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyControlBase.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdRelyingPartyControlBase.cs
@@ -459,7 +459,9 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 			if (receiver == null || receiver == this.ClientID) {
 				var response = this.RelyingParty.GetResponse();
 				if (response != null) {
-					string persistentString = response.GetCallbackArgument(UsePersistentCookieCallbackKey);
+					// We don't use response.GetCallbackArgument because it will return null in stateless
+					// mode since the arg may have been tampered with, but this isn't a security decision.
+					string persistentString = this.Page.Request.QueryString[UsePersistentCookieCallbackKey];
 					bool persistentBool;
 					if (persistentString != null && bool.TryParse(persistentString, out persistentBool)) {
 						this.UsePersistentCookie = persistentBool;
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdTextBox.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdTextBox.cs
index b7c879e..184707c 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdTextBox.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/OpenIdTextBox.cs
@@ -1035,7 +1035,9 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 
 			var response = this.RelyingParty.GetResponse();
 			if (response != null) {
-				string persistentString = response.GetCallbackArgument(UsePersistentCookieCallbackKey);
+				// We don't use response.GetCallbackArgument because it will return null in stateless
+				// mode since the arg may have been tampered with, but this isn't a security decision.
+				string persistentString = this.Page.Request.QueryString[UsePersistentCookieCallbackKey];
 				bool persistentBool;
 				if (persistentString != null && bool.TryParse(persistentString, out persistentBool)) {
 					this.UsePersistentCookie = persistentBool;
diff --git a/src/DotNetOpenAuth/OpenId/RelyingParty/PositiveAnonymousResponse.cs b/src/DotNetOpenAuth/OpenId/RelyingParty/PositiveAnonymousResponse.cs
index 13eb1a2..4692b5b 100644
--- a/src/DotNetOpenAuth/OpenId/RelyingParty/PositiveAnonymousResponse.cs
+++ b/src/DotNetOpenAuth/OpenId/RelyingParty/PositiveAnonymousResponse.cs
@@ -156,6 +156,7 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 			if (this.response.ReturnToParametersSignatureValidated) {
 				return this.response.GetReturnToArgument(key);
 			} else {
+				Logger.OpenId.WarnFormat(OpenIdStrings.CallbackArgumentsRequireSecretStore, typeof(IAssociationStore<Uri>).Name, typeof(OpenIdRelyingParty).Name);
 				return null;
 			}
 		}
@@ -186,6 +187,7 @@ namespace DotNetOpenAuth.OpenId.RelyingParty {
 
 				return args;
 			} else {
+				Logger.OpenId.WarnFormat(OpenIdStrings.CallbackArgumentsRequireSecretStore, typeof(IAssociationStore<Uri>).Name, typeof(OpenIdRelyingParty).Name);
 				return EmptyDictionary<string, string>.Instance;
 			}
 		}