Lti blackboard debug

[ascholerChemeketa]

First commit adds very verbose debugging on the error path that should be producing the message Albert B is seeing in blackboard. All of this logging should only happen on error, so it shouldn't be an overwhelming increase. We can back out this commit or reduce what is logged once we track down the root issue.

Second has some permanent logging improvements.

[Copilot]

🟡 Changes recommended

Critical logging issues and unresolved container configuration problems block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR passes deployment settings to the admin container and adds LTI error-path diagnostics.

Changes:

• Adds Caddy/Certbot environment configuration for admin.
• Logs failed LTI token and service requests, including JSON parsing failures.

File summaries

File	Review summary
docker-compose.yml	Adds admin environment variables. Moderate issues remain with the :80 default and missing LANDING_PAGE_URL.
components/rsptx/lti1p3/pylti1p3/service_connector.py	Adds LTI diagnostics. Critical issues remain with unredacted response dumps and duplicate exception logging; malformed JSON handling also needs adjustment.

Review details

Suppressed comments (4)

components/rsptx/lti1p3/pylti1p3/service_connector.py:139

• This branch has already read the response body with await r.text(), so r.__dict__ includes the cached _body as well as all headers and internal state. Malformed LMS responses can contain sensitive data or be very large; avoid dumping the full object and use a bounded, redacted diagnostic instead.

                rslogger.error(
                    f"get_access_token json decode error: {r.status} - {r.reason}. Headers: {r.headers}. Full response: {r.__dict__}"
                )

components/rsptx/lti1p3/pylti1p3/service_connector.py:197

• The generic service-request error path also serializes the entire aiohttp response object into logs. A failed LMS request can carry cookies, sensitive headers, or a large response body, so this creates the same data-exposure and log-volume risk outside the token endpoint; use the same bounded, redacted diagnostic helper here.

        if not r.ok:
            rslogger.error(
                f"Service request failed: {r.status} - {r.reason}. Headers: {r.headers}. Full response: {r.__dict__}"
            )

components/rsptx/lti1p3/pylti1p3/service_connector.py:138

• The new JSON-decode log is only reachable for the non-application/json fallback. If Blackboard returns malformed JSON with the standard application/json content type, await r.json() raises before reaching this block, so the failure remains unlogged. Put both parsing paths under the same JSONDecodeError handler.

                rslogger.error(
                    f"get_access_token json decode error: {r.status} - {r.reason}. Headers: {r.headers}. Full response: {r.__dict__}"

docker-compose.yml:543

• The admin container still does not receive LANDING_PAGE_URL; Compose only interpolates variables listed under environment, and the admin root router uses settings.landing_page_url for unauthenticated redirects. Setting this value in .env therefore still has no effect in the admin container, which leaves the configuration gap described in the PR unresolved.

      - CERTBOT_EMAIL=${CERTBOT_EMAIL}

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical configuration and credential-exposure risks, along with unresolved error-handling issues, must be addressed.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

components/rsptx/lti1p3/pylti1p3/service_connector.py:122

• This handler also catches connection, DNS, and timeout exceptions raised before post() returns, while r is still None. Calling r.text() then raises AttributeError and masks the original network failure (and the later LtiServiceException(r) cannot represent it); handle the r is None case by logging/re-raising the original exception.

        except Exception:
            raw_body = await r.text()

components/rsptx/lti1p3/pylti1p3/service_connector.py:118

• Logging the entire ClientResponse.__dict__ is not a safe diagnostic boundary: it can include arbitrary response headers/body and request metadata. Error logs can therefore retain credentials, cookies, or other LMS response data; log only an allowlisted status/reason (and a deliberately sanitized, bounded body if needed).

                    f"get_access_token post failed: {r.status} - {r.reason}. Headers: {r.headers}. Full response: {r.__dict__}"

components/rsptx/lti1p3/pylti1p3/service_connector.py:124

• The exception-path diagnostic repeats the unsafe full-object logging. r.__dict__ can include response headers/body and request metadata, so this can expose secrets or sensitive LMS details in retained logs; restrict this message to explicitly allowlisted fields.

                f"get_access_token exception caught: {r.status} - {r.reason}. Headers: {r.headers}. Full response: {r.__dict__}"

components/rsptx/lti1p3/pylti1p3/service_connector.py:138

• The JSON-decode error path also logs the complete response object, including potentially sensitive headers and body contents. Keep the error diagnostic bounded and allowlisted rather than serializing __dict__ into logs.

                    f"get_access_token json decode error: {r.status} - {r.reason}. Headers: {r.headers}. Full response: {r.__dict__}"

components/rsptx/lti1p3/pylti1p3/service_connector.py:14

• The pylti1p3 directory is explicitly intended to remain generic, but this import makes ServiceConnector depend on the Runestone-only rsptx.logging package. Consumers that use this maintained library without the full Runestone application will now fail at import time; use the existing local logger or standard-library logging instead.

from rsptx.logging import rslogger

• Files reviewed: 3/3 changed files
• Comments generated: 3
• Review effort level: Lite

[ascholerChemeketa]

re: copilot review items

The duplicative logging is intentional. I'm also not too concerned about logging oath info temporarily on this error path. We'll be backing out most of that logging once we get the blackboard issue figured out.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate issues affect sensitive error disclosure, failure handling, and diagnostic accuracy.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

bases/rsptx/admin_server_api/routers/lti1p3.py:967

• LtiServiceException is raised for every non-OK service response, including authentication failures, missing scopes, and LMS 5xx responses—not just a closed course (service_connector.py:194-198). The new user-facing text therefore tells instructors to investigate course closure for unrelated failures, which can send support in the wrong direction. Use a generic LMS access error or select the guidance from the response status.

            detail=f"Error accessing your course's assignments. Your course may be closed in your Learning Management System. Debugging information: {e}",

components/rsptx/lti1p3/pylti1p3/service_connector.py:14

• This module is inside the generic, locally maintained pylti1p3 copy, whose README says to avoid Runestone-specific modifications. Importing rsptx.logging couples the library to the host application and prevents reuse outside this repository; use the package's generic logger or inject a logger instead.

from rsptx.logging import rslogger

components/rsptx/lti1p3/pylti1p3/service_connector.py:125

• The LtiServiceException raised at line 120 is caught by this broad except Exception, so every non-2xx token response emits both the “post failed” and “exception caught” error logs, with the latter being misleading. Add an except LtiServiceException: raise branch before the generic handler so one failure produces one diagnostic.

            rslogger.error(
                f"get_access_token exception caught: {r.status} - {r.reason}. Headers: {r.headers}. Full response: {r.__dict__}"
            )

components/rsptx/lti1p3/pylti1p3/service_connector.py:124

• This repeats the unsafe full-object dump after await r.text(). The response body may now be cached on the ClientResponse, so this error path can persist an LMS-controlled body along with request metadata (including credentials) in application logs. Use the same redacted/allowlisted logging helper as the other error paths rather than r.__dict__.

            rslogger.error(
                f"get_access_token exception caught: {r.status} - {r.reason}. Headers: {r.headers}. Full response: {r.__dict__}"

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

[ascholerChemeketa]

Fixed