fix Implement logging for authentication errors#100
Open
odaysec wants to merge 1 commit intoRoblox:mainfrom
Open
fix Implement logging for authentication errors#100odaysec wants to merge 1 commit intoRoblox:mainfrom
odaysec wants to merge 1 commit intoRoblox:mainfrom
Conversation
Added logging for various error conditions during authentication handling. The fix is to avoid sending detailed exception information to the user and instead: (1) log detailed errors server-side; and (2) return generic, user-friendly error messages in HTTP responses. The user should get enough information to know that an error occurred and maybe at a high level what went wrong (e.g., “invalid response from server”), but not internal exception messages, raw JSON payloads, or other diagnostic data. For this specific file, the best approach without changing overall functionality is: 1. Keep the overall control flow and error categories the same (i.e., still differentiate `StateMismatchError`, `MissingAuthCodeError`, JSON errors, `ClientResponseError`, `ClientError`, and generic `Exception`), because that likely feeds into the rest of the add-on via `event.exception`. 2. Stop including `str(exception)` and `exception.doc` in the HTTP response text returned to the browser. 3. Introduce logging of exception details using Python’s standard `logging` module so developers still have access to stack traces and messages. This is a well-known library and can be safely imported. 4. Optionally, include a generic correlation ID in both the logs and the error message if you want matching, but to keep changes minimal we can just log the exception and return a high-level message. Concretely: - Add `import logging` at the top alongside other imports. - In `handle_request`, for each `except` block that currently calls `self.__get_error_response` with messages containing `str(exception)` or `exception.doc`, change those messages to generic text such as: - State mismatch: keep clear but non-technical wording. - Missing auth code: a short, generic statement without appending `str(exception)`. - JSON decode error: “The server returned an invalid response during login.” - `ClientResponseError`: “Login request failed due to a server error.” (optionally include status code in a user-safe way). - `ClientError`: “Login request failed due to a network error.” - Generic `Exception`: “An internal error occurred during login.” - In each of those `except` blocks, before returning, log the exception with `logging.exception(...)` or `logging.error(..., exc_info=True)` so the stack trace and message are preserved server-side. This single change to the error-message construction addresses all five variants since they all flow through `__get_error_response`’s `message` parameter and are currently including `str(exception)` (or other exception-derived content).
This file contains hidden or 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
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.
Added logging for various error conditions during authentication handling.
The fix is to avoid sending detailed exception information to the user and instead: (1) log detailed errors server-side; and (2) return generic, user-friendly error messages in HTTP responses. The user should get enough information to know that an error occurred and maybe at a high level what went wrong (e.g., “invalid response from server”), but not internal exception messages, raw JSON payloads, or other diagnostic data.
For this specific file, the best approach without changing overall functionality is:
StateMismatchError,MissingAuthCodeError, JSON errors,ClientResponseError,ClientError, and genericException), because that likely feeds into the rest of the add-on viaevent.exception.str(exception)andexception.docin the HTTP response text returned to the browser.loggingmodule so developers still have access to stack traces and messages. This is a well-known library and can be safely imported.Concretely:
import loggingat the top alongside other imports.handle_request, for eachexceptblock that currently callsself.__get_error_responsewith messages containingstr(exception)orexception.doc, change those messages to generic text such as:str(exception).ClientResponseError: “Login request failed due to a server error.” (optionally include status code in a user-safe way).ClientError: “Login request failed due to a network error.”Exception: “An internal error occurred during login.”exceptblocks, before returning, log the exception withlogging.exception(...)orlogging.error(..., exc_info=True)so the stack trace and message are preserved server-side.This single change to the error-message construction addresses all five variants since they all flow through
__get_error_response’smessageparameter and are currently includingstr(exception)(or other exception-derived content).