Skip to content

fix(frontend): isolate provider account OAuth callbacks - #270

Open
DeanHH wants to merge 1 commit into
astaxie:mainfrom
DeanHH:main
Open

fix(frontend): isolate provider account OAuth callbacks#270
DeanHH wants to merge 1 commit into
astaxie:mainfrom
DeanHH:main

Conversation

@DeanHH

@DeanHH DeanHH commented Aug 20, 2026

Copy link
Copy Markdown

Summary

Fix an issue where signing in by scanning the DingTalk QR code unexpectedly opens the Create Provider dialog after the administrator reaches the console.

The DingTalk OAuth callback uses generic parameters such as code, state, and error. These parameters could be incorrectly interpreted as a Provider account OAuth callback, causing the Provider creation wizard to open automatically.

Related Issue

N/A.

Changes

  • Require an explicit Provider account OAuth marker before accepting generic OAuth error parameters.
  • Require the same marker before accepting generic authorization codes.
  • Preserve generic callback parsing for marked callbacks and manual Provider account callback input.

Type of Change

  • Bug fix
  • New feature
  • Refactor or maintenance
  • Documentation
  • Deployment or configuration

Verification

  • npm run lint
  • npm run typecheck
  • npm test
  • git diff --check

Compatibility, Security, and Operations

No API, persistence, configuration, or deployment changes. Marked Provider account callbacks and manual callback parsing remain supported.

Checklist

  • The PR title and body are written in English.
  • Tests were added or updated for behavior changes, or the reason they are unnecessary is documented.
  • No credentials, local .env files, databases, backups, or runtime logs are included.
  • Environment variable changes are synchronized across examples, Compose, start.sh, and deployment documentation where applicable.
  • Shared user-facing behavior is documented consistently in English, Simplified Chinese, and Japanese where applicable.
  • data/model-catalog.yaml remains tracked and catalog changes were reviewed where applicable.
  • git diff --check passes.

@astaxie astaxie left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the complete current diff at bbf2e7e. The callback isolation logic is directionally correct: unmarked DingTalk/OIDC callbacks no longer treat generic code or error as Provider-account credentials, while marked and manual callbacks retain the generic names.

[P1] Add regression tests for this authentication-sensitive behavior (frontend/features/admin/core/session.tsx:78-87). The PR changes parsing of OAuth authorization codes and errors but adds no test coverage; the Verification section only lists existing commands. Please add tests proving unmarked query/fragment callbacks with code/error are ignored, marked callbacks still parse them, and manual callback parsing remains compatible.

[P1] Required CI is missing. GitHub currently reports UNSTABLE with an empty status check rollup, so this head cannot be approved or considered merge-ready. Publish the required Backend/Frontend/Deployment/Repository gates and aggregate CI, then request re-review on the exact head.

@astaxie astaxie left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up re-review of exact head bbf2e7e23a27d91deedce3541cc88282fca42b80: all seven GitHub checks are now SUCCESS and the merge state is MERGEABLE/CLEAN. The implementation still correctly isolates unmarked generic OAuth callbacks.

The prior test-coverage blocker remains: this authentication-sensitive parser change still has no regression tests for unmarked code/error, marked callbacks, and manual callback compatibility. Local frontend lint, typecheck, 64 domain tests, 5 component tests, and production build pass, but the changed behavior is not exercised by a new test. This remains comment-only; please add the focused parser tests and request another review.

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.

2 participants