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: |UI| add JUNK_MAIL_FORCE_PASS_LIST #539

Merged
merged 1 commit into from
Dec 30, 2024
Merged

Conversation

dreamhunter2333
Copy link
Owner

@dreamhunter2333 dreamhunter2333 commented Dec 30, 2024

User description

#531


PR Type

enhancement, documentation


Description

  • Added JUNK_MAIL_FORCE_PASS_LIST to the admin API configuration to allow specifying mandatory checks for junk mail.
  • Implemented logic in check_junk.ts to validate emails against the JUNK_MAIL_FORCE_PASS_LIST.
  • Updated Bindings type definition to include JUNK_MAIL_FORCE_PASS_LIST.
  • Documented the new configuration option in both English and Chinese CLI guides.
  • Added an example configuration for JUNK_MAIL_FORCE_PASS_LIST in the wrangler template.

Changes walkthrough 📝

Relevant files
Enhancement
worker_config.ts
Add `JUNK_MAIL_FORCE_PASS_LIST` to admin API configuration

worker/src/admin_api/worker_config.ts

  • Added JUNK_MAIL_FORCE_PASS_LIST to the configuration object.
+1/-0     
check_junk.ts
Implement `JUNK_MAIL_FORCE_PASS_LIST` logic in junk mail check

worker/src/email/check_junk.ts

  • Imported getStringArray utility function.
  • Added logic to check JUNK_MAIL_FORCE_PASS_LIST for junk mail
    validation.
  • Updated SPF, DKIM, and DMARC checks to populate passedList.
  • +27/-17 
    types.d.ts
    Update `Bindings` type with `JUNK_MAIL_FORCE_PASS_LIST`   

    worker/src/types.d.ts

    • Added JUNK_MAIL_FORCE_PASS_LIST to Bindings type definition.
    +1/-0     
    Documentation
    CHANGELOG.md
    Document `JUNK_MAIL_FORCE_PASS_LIST` configuration             

    CHANGELOG.md

  • Documented the addition of JUNK_MAIL_FORCE_PASS_LIST configuration.
  • +1/-0     
    cli.md
    Add `JUNK_MAIL_FORCE_PASS_LIST` documentation in English CLI guide

    vitepress-docs/docs/en/cli.md

    • Added documentation for JUNK_MAIL_FORCE_PASS_LIST configuration.
    +2/-0     
    worker.md
    Add `JUNK_MAIL_FORCE_PASS_LIST` documentation in Chinese CLI guide

    vitepress-docs/docs/zh/guide/cli/worker.md

  • Added documentation for JUNK_MAIL_FORCE_PASS_LIST configuration in
    Chinese.
  • +3/-1     
    wrangler.toml.template
    Add `JUNK_MAIL_FORCE_PASS_LIST` example in wrangler template

    worker/wrangler.toml.template

    • Added JUNK_MAIL_FORCE_PASS_LIST configuration example.
    +2/-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 ✅

    531 - Fully compliant

    Fully compliant requirements:

    • Add an option to specify the required number of validation checks (one, two, or all).
    • Allow users to specify which validations are mandatory.

    Not compliant requirements:
    []

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Issue

    The logic for checking the forcePassList might incorrectly mark emails as junk if the forcePassList is empty. This should be validated to ensure it works as intended.

    const forcePassList = getStringArray(env.JUNK_MAIL_FORCE_PASS_LIST);
    const passedList: string[] = [];
    
    const headers = parsedEmail.headers;
    for (const header of headers) {
        if (!header["key"]) continue;
        if (!header["value"]) continue;
    
        // check spf
        if (header["key"].toLowerCase() == "received-spf") {
            if (!header["value"].toLowerCase().includes("pass")) {
                return true;
            }
            passedList.push("spf");
        }
    
        // check dkim and dmarc
        if (header["key"].toLowerCase() == "authentication-results") {
            if (header["value"].toLowerCase().includes("dkim=")) {
                if (!header["value"].toLowerCase().includes("dkim=pass")) {
                    return true;
                }
                passedList.push("dkim");
            }
            if (header["value"].toLowerCase().includes("dmarc=")) {
                if (!header["value"].toLowerCase().includes("dmarc=pass")) {
                    return true;
                }
                passedList.push("dmarc");
            }
        }
    }
    
    if (forcePassList?.length == 0) return false;
    
    // check force pass list
    return forcePassList.some(
        (checkName) => !passedList.includes(checkName.toLowerCase())
    );

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a null or undefined check for forcePassList before accessing its length property

    Ensure that the forcePassList is properly checked for null or undefined values
    before accessing its length property to avoid potential runtime errors.

    worker/src/email/check_junk.ts [48]

    -if (forcePassList?.length == 0) return false;
    +if (!forcePassList || forcePassList.length == 0) return false;
    Suggestion importance[1-10]: 8

    Why: This suggestion ensures that forcePassList is properly checked for null or undefined values before accessing its length property, which prevents potential runtime errors.

    8

    @dreamhunter2333 dreamhunter2333 merged commit c964d77 into main Dec 30, 2024
    1 check passed
    @dreamhunter2333 dreamhunter2333 deleted the feature/dev branch December 30, 2024 10:38
    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