Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: announcement support html #566

Merged
merged 2 commits into from
Jan 20, 2025
Merged

feat: announcement support html #566

merged 2 commits into from
Jan 20, 2025

Conversation

dreamhunter2333
Copy link
Owner

@dreamhunter2333 dreamhunter2333 commented Jan 20, 2025

User description

#565


PR Type

Enhancement


Description

  • Added HTML support for announcements.

  • Integrated useNotification for displaying announcements.

  • Updated getOpenSettings to use notifications.

  • Modified UI components to display announcements.


Changes walkthrough 📝

Relevant files
Enhancement
App.vue
Add notification provider for HTML announcements                 

frontend/src/App.vue

  • Added n-notification-provider for HTML announcements.
  • Nested n-message-provider within n-notification-provider.
  • +20/-18 
    Header.vue
    Integrate notification in Header component                             

    frontend/src/views/Header.vue

  • Imported and initialized useNotification.
  • Updated getOpenSettings call to include notification.
  • +2/-1     
    About.vue
    Display HTML announcement in About component                         

    frontend/src/views/common/About.vue

  • Display announcement using v-html.
  • Imported useGlobalState to access announcement.
  • +3/-0     
    Login.vue
    Integrate notification in Login component                               

    frontend/src/views/common/Login.vue

  • Imported and initialized useNotification.
  • Updated getOpenSettings call to include notification.
  • +2/-1     
    index.js
    Use notification for HTML announcements in API                     

    frontend/src/api/index.js

  • Updated getOpenSettings to use notification for announcements.
  • Used h function to render HTML content in notifications.
  • +8/-5     
    Configuration changes
    vite.config.js
    Auto-import useNotification in Vite config                             

    frontend/vite.config.js

    • Auto-imported useNotification from naive-ui.
    +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    565 - Partially compliant

    Fully compliant requirements:

    • Add support for HTML in the announcement variable.
    • Ensure the announcement can contain hyperlinks.

    Not compliant requirements:

    • Improve the visual appearance with line breaks.
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🔒 Security concerns

    XSS vulnerability:
    The use of innerHTML in notification.info can introduce XSS vulnerabilities if the announcement content is not properly sanitized. Ensure that the content is sanitized before rendering it as HTML.

    ⚡ Recommended focus areas for review

    Possible Issue

    The notification.info function uses innerHTML which can potentially introduce XSS vulnerabilities if the announcement content is not properly sanitized.

    notification.info({
        content: () => {
            return h("div", {
                innerHTML: announcement.value
            });
        }
    });

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Sanitize HTML to prevent XSS

    Ensure that the announcement variable is properly sanitized before rendering it with
    v-html to prevent potential XSS attacks.

    frontend/src/views/common/About.vue [10]

    -<div v-html="announcement"></div>
    +<div v-html="sanitizeHtml(announcement)"></div>
    Suggestion importance[1-10]: 10

    Why: This suggestion addresses a critical security vulnerability by ensuring that the announcement variable is sanitized before being rendered with v-html, preventing potential XSS attacks.

    10
    General
    Add error handling for notifications

    Add error handling for the notification.info call to ensure it does not break the
    application if it fails.

    frontend/src/api/index.js [93-99]

    -notification.info({
    -    content: () => {
    -        return h("div", {
    -            innerHTML: announcement.value
    -        });
    -    }
    -});
    +try {
    +    notification.info({
    +        content: () => {
    +            return h("div", {
    +                innerHTML: announcement.value
    +            });
    +        }
    +    });
    +} catch (notifyError) {
    +    console.error("Notification error:", notifyError);
    +}
    Suggestion importance[1-10]: 7

    Why: Adding error handling for the notification.info call improves the robustness of the application by ensuring that a failure in the notification system does not break the application.

    7

    @dreamhunter2333 dreamhunter2333 merged commit 3f81fbe into main Jan 20, 2025
    @dreamhunter2333 dreamhunter2333 deleted the feature/dev branch January 20, 2025 05:53
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant