Skip to content

[codex] implement sign-in page from figma#2999

Draft
qingqing-ux wants to merge 11 commits intogoplus:uifrom
qingqing-ux:codex/sign-in-page
Draft

[codex] implement sign-in page from figma#2999
qingqing-ux wants to merge 11 commits intogoplus:uifrom
qingqing-ux:codex/sign-in-page

Conversation

@qingqing-ux
Copy link
Copy Markdown
Collaborator

Summary

  • add a new /sign-in route with a Figma-aligned login page and safe returnTo handling
  • route existing unauthenticated entry points through /sign-in and harden callback redirects against unsafe targets
  • replace the hero illustration with an exact Figma export and document the provider env keys

Test Plan

  • cd spx-gui && npm run type-check
  • cd spx-gui && npm test -- --run
  • local browser check of /sign-in?returnTo=%2Fproject%2Falice%2Fdemo on desktop and mobile

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request implements a new branded /sign-in page in spx-gui, replacing direct redirects to Casdoor. It introduces a sign-in entry utility layer for safe returnTo normalization and provider-aware redirects for WeChat, QQ, and username/password. The UI is modularized into hero and panel components, and existing entry points across the application have been updated to route through the new page. Review feedback highlights a discrepancy between the implementation plan and the code regarding the hero illustration's file format, and suggests sanitizing the returnTo parameter in the initiateSignIn function for improved security.

**Files:**
- Create: `spx-gui/src/components/sign-in/SignInHero.vue`
- Create: `spx-gui/src/components/sign-in/SignInPanel.vue`
- Create: `spx-gui/src/pages/sign-in/assets/sign-in-hero.svg`
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The implementation plan specifies creating a .svg file for the hero illustration, but the implementation in spx-gui/src/pages/sign-in/index.vue uses a .png file (sign-in-hero.png).

While this might be a deliberate choice due to the complexity of the illustration, it's good practice to keep documentation aligned with the implementation. SVGs are generally preferred for illustrations over raster formats like PNG due to their scalability without quality loss and potentially smaller file size for vector-based graphics.

If using a PNG is intentional, please consider updating this plan to reflect the change.

Comment on lines +66 to +73
export function initiateSignIn(returnTo: string = getDefaultReturnTo(), additionalParams?: Record<string, string>) {
// Workaround for casdoor-js-sdk not supporting override of `redirectPath` in `signin_redirect`.
const casdoorSdk = new Sdk({
...casdoorConfig,
redirectPath: `${casdoorAuthRedirectPath}?returnTo=${encodeURIComponent(returnTo)}`
})
casdoorSdk.signin_redirect()
casdoorSdk.signin_redirect(additionalParams)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For defense-in-depth, it would be beneficial to sanitize the returnTo parameter within the initiateSignIn function. Although the returnTo value is correctly validated in callback.vue upon return from the authentication provider, sanitizing it here prevents unsanitized, user-controllable data from being passed to an external service (Casdoor). This hardens the function against potential misuse if it's ever called from a new context that forgets to sanitize the returnTo value beforehand.

Note: You'll also need to import normalizeSafeReturnTo from ./sign-in-entry.

Suggested change
export function initiateSignIn(returnTo: string = getDefaultReturnTo(), additionalParams?: Record<string, string>) {
// Workaround for casdoor-js-sdk not supporting override of `redirectPath` in `signin_redirect`.
const casdoorSdk = new Sdk({
...casdoorConfig,
redirectPath: `${casdoorAuthRedirectPath}?returnTo=${encodeURIComponent(returnTo)}`
})
casdoorSdk.signin_redirect()
casdoorSdk.signin_redirect(additionalParams)
}
export function initiateSignIn(returnTo: string = getDefaultReturnTo(), additionalParams?: Record<string, string>) {
const safeReturnTo = normalizeSafeReturnTo(returnTo)
// Workaround for casdoor-js-sdk not supporting override of `redirectPath` in `signin_redirect`.
const casdoorSdk = new Sdk({
...casdoorConfig,
redirectPath: `${casdoorAuthRedirectPath}?returnTo=${encodeURIComponent(safeReturnTo)}`
})
casdoorSdk.signin_redirect(additionalParams)
}

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