Fix for review comments

Author: ashok672

msal/__main__.py

@@ -339,6 +339,8 @@ def _main():
             logging.error("Invalid input: %s", e)
         except KeyboardInterrupt:  # Useful for bailing out a stuck interactive flow
             print("Aborted")
+        except Exception as e:
+            logging.error("Error: %s", e)
 
 if __name__ == "__main__":
     _main()

msal/application.py

@@ -965,8 +965,14 @@ def initiate_auth_code_flow(
         :param str response_mode:
             .. deprecated::
                 This parameter is deprecated and will be removed in a future version.
-                The response_mode is always set to ``form_post`` for security reasons,
-                regardless of the value provided. Do not use this parameter.
+                
+                **For PublicClientApplication**: response_mode is automatically set to
+                ``form_post`` for security reasons. This parameter is ignored.
+                
+                **For ConfidentialClientApplication**: You should configure your web
+                framework to accept form_post responses instead of query responses.
+                While this parameter still works, it will be removed in a future version.
+                Using query-based response modes is less secure and should be avoided.
 
         :return:
             The auth code flow. It is a dict in this form::
@@ -990,7 +996,9 @@ def initiate_auth_code_flow(
             import warnings
             warnings.warn(
                 "The 'response_mode' parameter is deprecated and will be removed in a future version. "
-                "Response mode is always 'form_post' for security reasons.",
+                "For public clients, response_mode is automatically set to 'form_post'. "
+                "For confidential clients, configure your web framework to use 'form_post'. "
+                "Query-based response modes are less secure.",
                 DeprecationWarning,
                 stacklevel=2)
         client = _ClientWithCcsRoutingInfo(

msal/oauth2cli/authcode.py

@@ -111,52 +111,33 @@ class _AuthCodeHandler(BaseHTTPRequestHandler):
     def do_GET(self):
         # For flexibility, we choose to not check self.path matching redirect_uri
         #assert self.path.startswith('/THE_PATH_REGISTERED_BY_THE_APP')
-        parsed_url = urlparse(self.path)
-        qs = parse_qs(parsed_url.query)
 
-        if qs.get('code'):  # Auth code via GET is a security risk - reject it
-            logger.error("Received auth code via query string (GET request). "
-                        "This is a security risk. Only form_post (POST) is supported.")
+        # Check if this is a blank redirect (eSTS error flow where user clicked OK)
+        qs = parse_qs(urlparse(self.path).query)
+        if not qs or (not qs.get('code') and not qs.get('error')):
+            # Blank redirect from eSTS error - show generic error and mark done
             self._send_full_response(
-                "Authentication method not supported. "
-                "The application requires response_mode=form_post for security. "
-                "Please ensure your application registration uses form_post response mode.",
-                is_ok=False)
-        elif qs.get("error"):  # Errors can come via GET - process them
-            auth_response = _qs2kv(qs)
-            self._process_auth_response(auth_response)
-        elif not qs and parsed_url.path == '/':
-            # GET request to root with no query params - this is likely from clicking OK on eSTS error
-            # Treat it as an error since success would come via POST
-            logger.warning("Received GET request to root path with no query parameters - treating as authentication error")
-            auth_response = {
-                "error": "authentication_failed", 
-                "error_description": "Please close this window and try again."
-            }
-            # Include the original state so state validation doesn't fail
-            if self.server.auth_state:
-                auth_response["state"] = self.server.auth_state
-            self._process_auth_response(auth_response)
+                "Authentication could not be completed. "
+                "You can close this window and return to the application.")
+            self.server.done = True
         else:
+            # GET request with parameters (shouldn't happen with form_post, but handle gracefully)
             self._send_full_response(self.server.welcome_page)
         # NOTE: Don't do self.server.shutdown() here. It'll halt the server.
 
     def do_POST(self):
         # Handle form_post response mode where auth code is sent via POST body
         content_length = int(self.headers.get('Content-Length', 0))
         post_data = self.rfile.read(content_length).decode('utf-8')
-        try:
-            from urllib.parse import parse_qs as parse_qs_post
-        except ImportError:
-            from urlparse import parse_qs as parse_qs_post
 
-        qs = parse_qs_post(post_data)
+        qs = parse_qs(post_data)
         if qs.get('code') or qs.get('error'):  # So, it is an auth response
             auth_response = _qs2kv(qs)
             logger.debug("Got auth response via POST: %s", auth_response)
             self._process_auth_response(auth_response)
         else:
             self._send_full_response("Invalid POST request", is_ok=False)
+        # NOTE: Don't do self.server.shutdown() here. It'll halt the server.
 
     def _process_auth_response(self, auth_response):
         """Process the auth response from either GET or POST request."""
@@ -177,7 +158,15 @@ def _process_auth_response(self, auth_response):
             # to avoid showing literal placeholder text like "$error_description"
             safe_data.setdefault("error", "")
             safe_data.setdefault("error_description", "")
-            safe_data.setdefault("error_uri", "")
+            # Format error message nicely: include ": description." only if description exists
+            if "code" not in auth_response:  # This is an error response
+                error_desc = auth_response.get("error_description", "").strip()
+                if error_desc:
+                    safe_data["error_message"] = f"{safe_data['error']}: {error_desc}."
+                else:
+                    safe_data["error_message"] = safe_data["error"]
+            else:
+                safe_data["error_message"] = ""
             self._send_full_response(template.safe_substitute(**safe_data))
             self.server.auth_response = auth_response  # Set it now, after the response is likely sent
 
@@ -336,6 +325,15 @@ def _get_auth_response(self, result, auth_uri=None, timeout=None, state=None,
         welcome_uri = "http://localhost:{p}".format(p=self.get_port())
         abort_uri = "{loc}?error=abort".format(loc=welcome_uri)
         logger.debug("Abort by visit %s", abort_uri)
+        
+        # Enforce response_mode=form_post for security
+        if auth_uri:
+            parsed = urlparse(auth_uri)
+            params = parse_qs(parsed.query)
+            params['response_mode'] = ['form_post']  # Enforce form_post
+            new_query = urlencode(params, doseq=True)
+            auth_uri = parsed._replace(query=new_query).geturl()
+        
         self._server.welcome_page = Template(welcome_template or "").safe_substitute(
             auth_uri=auth_uri, abort_uri=abort_uri)
         if auth_uri:  # Now attempt to open a local browser to visit it
@@ -369,7 +367,7 @@ def _get_auth_response(self, result, auth_uri=None, timeout=None, state=None,
             "Authentication complete. You can return to the application. Please close this browser tab.\n\n"
             "For your security: Do not share the contents of this page, the address bar, or take screenshots.")
         self._server.error_template = Template(error_template or
-            "Authentication failed. $error: $error_description.\n\n"
+            "Authentication failed. $error_message\n\n"
             "For your security: Do not share the contents of this page, the address bar, or take screenshots.")
 
         self._server.timeout = timeout  # Otherwise its handle_timeout() won't work

msal/oauth2cli/oauth2.py

@@ -176,14 +176,22 @@ def _build_auth_request_params(self, response_type, **kwargs):
         response_type = self._stringify(response_type)
 
         params = {'client_id': self.client_id, 'response_type': response_type}
-        # Strictly enforce form_post for security - query string is not allowed
-        params['response_mode'] = 'form_post'
-        if 'response_mode' in kwargs and kwargs['response_mode'] != 'form_post':
+        params.update(kwargs)
+        if 'response_mode' in params:
             import warnings
             warnings.warn(
-                "The 'response_mode' parameter will be overridden to 'form_post' for better security.",
-                UserWarning)
-        params.update({k: v for k, v in kwargs.items() if k != 'response_mode'})  # Exclude response_mode from kwargs
+                "The 'response_mode' parameter is deprecated and will be removed in a future version. "
+                "For public clients, response_mode is automatically set to 'form_post'. "
+                "For confidential clients, configure your web framework to use 'form_post'. "
+                "Query-based response modes are less secure.",
+                DeprecationWarning,
+                stacklevel=3)
+            if params['response_mode'] != 'form_post':
+                warnings.warn(
+                    "response_mode='form_post' is recommended for better security. "
+                    "See https://tools.ietf.org/html/rfc6749#section-4.1.2",
+                    UserWarning,
+                    stacklevel=3)
         params = {k: v for k, v in params.items() if v is not None}  # clean up
         if params.get('scope'):
             params['scope'] = self._stringify(params['scope'])
@@ -474,13 +482,6 @@ def initiate_auth_code_flow(
             3. and then relay this dict and subsequent auth response to
                :func:`~obtain_token_by_auth_code_flow()`.
         """
-        if "response_mode" in kwargs:
-            import warnings
-            warnings.warn(
-                "The 'response_mode' parameter is deprecated and will be removed in a future version. "
-                "Response mode is always 'form_post' for security reasons.",
-                DeprecationWarning,
-                stacklevel=2)
         response_type = kwargs.pop("response_type", "code")  # Auth Code flow
             # Must be "code" when you are using Authorization Code Grant.
             # The "token" for Implicit Grant is not applicable thus not allowed.

tests/test_response_mode.py

@@ -54,19 +54,23 @@ def test_query_mode_override_with_successful_post_authentication(self):
             # Verify deprecation warning
             deprecation_warnings = [x for x in w if issubclass(x.category, DeprecationWarning)]
             self.assertEqual(len(deprecation_warnings), 1, "Should have exactly one deprecation warning")
-            self.assertIn("deprecated", str(deprecation_warnings[0].message).lower())
+            warning_msg = str(deprecation_warnings[0].message).lower()
+            self.assertIn("deprecated", warning_msg)
+            self.assertIn("public clients", warning_msg)
+            self.assertIn("confidential clients", warning_msg)
 
-            # Verify override warning
+            # Verify security warning for non-form_post
             user_warnings = [x for x in w if issubclass(x.category, UserWarning)]
-            self.assertEqual(len(user_warnings), 1, "Should have exactly one override warning")
-            self.assertIn("overridden", str(user_warnings[0].message).lower())
+            self.assertEqual(len(user_warnings), 1, "Should have exactly one security warning")
+            self.assertIn("form_post", str(user_warnings[0].message).lower())
 
-        # Part 2: Verify form_post is enforced in auth_uri
+        # Part 2: Verify response_mode parameter in flow
+        # Note: At oauth2.py level, response_mode is passed through as-is.
+        # The enforcement to form_post happens in authcode.py when opening the browser.
         parsed = urlparse(flow["auth_uri"])
         params = parse_qs(parsed.query)
+        # The flow still contains the query mode at this level
         self.assertIn("response_mode", params)
-        self.assertEqual(params["response_mode"][0], "form_post",
-                        "response_mode should be overridden to form_post")
 
         # Part 3: Verify actual POST authentication works (form_post flow)
         with AuthCodeReceiver() as receiver:
@@ -116,10 +120,10 @@ def send_get_request():
                     )
                 )
 
-                # Verify the GET request is rejected
-                self.assertEqual(response.status_code, 400, "GET with auth code should be rejected")
-                self.assertIn("not supported", response.text.lower(),
-                             "Error message should indicate method not supported")
+                # Verify the GET request shows welcome page (doesn't process auth code)
+                # When no welcome_template is provided, an empty 200 response is returned
+                self.assertEqual(response.status_code, 200, "GET request should return 200")
+                # The key is that it doesn't set auth_response - the auth code is ignored
 
             receiver._scheduled_actions = [(1, send_get_request)]