Skip to content

fix(matrix): add debug logging for OIDC registration and deduplicate redirect normalization#957

Merged
penso merged 2 commits intomainfrom
festive-plume
May 6, 2026
Merged

fix(matrix): add debug logging for OIDC registration and deduplicate redirect normalization#957
penso merged 2 commits intomainfrom
festive-plume

Conversation

@penso
Copy link
Copy Markdown
Collaborator

@penso penso commented May 2, 2026

Summary

  • Add comprehensive debug logging to the Matrix OIDC registration flow so operators can diagnose invalid_redirect_uri failures (discussion matrix OIDC on a reverse proxy does not work #872)
  • Deduplicate loopback redirect normalization — build_client_metadata() no longer has its own copy of the https→http rewrite
  • Pass the normalized redirect URI consistently to both build_client_metadata() and client.oauth().login()

With RUST_LOG=moltis_matrix=debug,moltis_gateway=debug, the following is now logged:

  1. The redirect_uri received from the frontend (channels.oauth_start called)
  2. MAS server metadata: issuer URL and registration endpoint
  3. Client registration parameters: original vs normalized redirect_uri, loopback detection, ApplicationType
  4. Full serialized ClientMetadata JSON sent to the MAS registration endpoint
  5. On failure: enhanced WARN with both Display and Debug error formats (the Debug format often includes the HTTP response body from MAS)

Validation

Completed

  • cargo test -p moltis-matrix -- oidc::tests — all 12 tests pass
  • cargo fmt --all -- --check — clean
  • cargo clippy -p moltis-matrix -p moltis-gateway -- -D warnings — clean

Remaining

  • ./scripts/local-validate.sh

Manual QA

  1. Run Moltis with RUST_LOG=moltis_matrix=debug,moltis_gateway=debug
  2. Navigate to Channels → Add Matrix → OIDC auth mode
  3. Attempt OIDC login — verify debug logs show redirect_uri, application type, and full client metadata
  4. If registration fails, verify the enhanced WARN includes the MAS error details

…redirect normalization

Users behind a reverse proxy report that Matrix OIDC still fails with
`invalid_redirect_uri` after the ApplicationType::Web fix (#893). The
only log line visible is the final WARN from the dispatch layer, making
it impossible to diagnose why MAS rejects the redirect URI.

Add debug-level logging throughout the OIDC registration flow:
- Server metadata (issuer, registration endpoint)
- Client registration parameters (redirect_uri, application type)
- Full serialized client metadata sent to MAS
- Enhanced error WARN with both Display and Debug error formats
- Gateway-level log of the redirect_uri received from the frontend

Also fix duplicated loopback normalization: build_client_metadata() had
its own copy of the https→http rewrite that duplicated
normalize_loopback_redirect(). Now it receives the pre-normalized URI,
eliminating the maintenance hazard. Pass the normalized URI consistently
to both build_client_metadata() and client.oauth().login().

Discussion: #872
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 2, 2026

Greptile Summary

This PR adds structured debug logging to the Matrix OIDC registration flow and deduplicates the loopback redirect URI normalization, moving it entirely into start_oidc_login so build_client_metadata now expects a pre-normalized URI.

  • Logging: Adds debug! spans at each step of start_oidc_login (server metadata discovery, registration parameters, serialized ClientMetadata JSON) plus an enhanced warn! on failure that surfaces the MAS error alongside all registration context.
  • Deduplication: The https→http rewrite for loopback URIs is removed from build_client_metadata; callers must call normalize_loopback_redirect first — enforced by the updated doc-comment and a renamed test.
  • Diagnostics types: New ClientRegistrationDiagnostics and ClientRegistrationFailure structs carry context through the error path, and are covered by two new unit tests.

Confidence Score: 5/5

Safe to merge — changes are additive logging only with a low-risk normalization refactor backed by passing tests.

All functional logic is unchanged: the loopback normalization deduplication produces identical runtime behavior since build_client_metadata now receives an already-normalized URI. The new diagnostic types are only exercised on the error path and carry no side effects. Two new unit tests cover the new structs end-to-end, and the full existing test suite passes.

No files require special attention.

Important Files Changed

Filename Overview
crates/matrix/src/oidc.rs Added ClientRegistrationDiagnostics and ClientRegistrationFailure helper types; deduped loopback normalization so callers pre-normalize before passing to build_client_metadata; added structured debug/warn logging throughout start_oidc_login; tests updated and two new tests added.
crates/gateway/src/channel.rs Added a single debug! log at the entry of oauth_start recording account_id and the raw redirect_uri string received from the frontend.

Sequence Diagram

sequenceDiagram
    participant FE as Frontend
    participant GW as channel.rs (oauth_start)
    participant OI as oidc.rs (start_oidc_login)
    participant MAS as MAS Server

    FE->>GW: oauth_start(account_id, redirect_uri)
    GW->>GW: debug log: channels.oauth_start called
    GW->>OI: start_oidc_login(client, account_id, redirect_uri)
    OI->>MAS: server_metadata()
    MAS-->>OI: issuer, registration_endpoint
    OI->>OI: debug log: server metadata discovered
    OI->>OI: normalize_loopback_redirect(redirect_uri) → registration_redirect
    OI->>OI: build_client_metadata(registration_redirect)
    OI->>OI: debug log: client registration parameters
    OI->>OI: Raw::new(&metadata) → raw_metadata
    OI->>OI: ClientRegistrationDiagnostics::new(...)
    OI->>OI: debug log: full client metadata JSON
    OI->>MAS: oauth().login(registration_redirect, ..., registration_data).build()
    alt Success
        MAS-->>OI: OAuthAuthorizationData { url, state }
        OI->>OI: info log: OIDC login started
        OI-->>GW: OidcLoginPending
        GW-->>FE: result
    else Failure
        MAS-->>OI: Error
        OI->>OI: ClientRegistrationFailure::new(diagnostics, error)
        OI->>OI: warn log: registration failed + full diagnostics
        OI-->>GW: ChannelError
        GW-->>FE: error
    end
Loading

Reviews (3): Last reviewed commit: "fix(matrix): surface OIDC registration d..." | Re-trigger Greptile

Comment thread crates/matrix/src/oidc.rs Outdated
@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq Bot commented May 2, 2026

Merging this PR will degrade performance by 21.65%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

❌ 1 regressed benchmark
✅ 38 untouched benchmarks
⏩ 5 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
env_substitution 10.9 µs 14 µs -21.65%

Comparing festive-plume (455dac1) with main (347376c)

Open in CodSpeed

Footnotes

  1. 5 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 2, 2026

Codecov Report

❌ Patch coverage is 90.47619% with 12 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/matrix/src/oidc.rs 90.47% 12 Missing ⚠️

📢 Thoughts on this report? Let us know!

@penso penso merged commit 209f080 into main May 6, 2026
23 of 32 checks passed
@penso penso deleted the festive-plume branch May 6, 2026 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant