Add optional MTA-STS policy fetch and popup diagnostics#12
Open
TJBK wants to merge 15 commits into
Open
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
📝 WalkthroughWalkthroughThis PR adds support for user-configurable DKIM selector probing and enriches the popup results display with score explanations, plain-text reports, root-vs-tab scope comparison, fix guidance for each pillar, and a manual domain lookup form. Mail provider definitions are expanded with new providers. ChangesCustom DKIM Selectors & Popup Enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
entrypoints/popup/main.ts (2)
1062-1077:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear the cached root/tab comparison when Settings re-run DNS checks.
This branch refreshes
lastResultwith the new DNS-affecting settings but leavescompareResultintact, so backing out of Settings can show comparison data generated with stale selectors/provider/MTA-STS options.Suggested fix
try { const result = await runDnsCheck(tabHostname, lastMode, { treatDnsResolutionErrorsAsFailure: settings.treatDnsResolutionErrorsAsFailure, dnsProvider: settings.dnsProvider, customDkimSelectors: settings.customDkimSelectors, fetchMtaStsPolicy: settings.fetchMtaStsPolicy, }); lastResult = result; + compareResult = null; await syncToolbarIconFromResult(result); } catch {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entrypoints/popup/main.ts` around lines 1062 - 1077, When re-running DNS checks from the Settings view (the block where currentView === 'settings' and dnsRefresh triggers runDnsCheck), you update lastResult but leave compareResult stale; update the code in that branch to clear the cached comparison (set compareResult to null/undefined) immediately after a successful runDnsCheck (and before calling syncToolbarIconFromResult) so comparisons use the new DNS-related settings (refer to runDnsCheck, lastResult, compareResult, syncToolbarIconFromResult).
592-603:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways keep manual lookup available on the error screen.
When
getActiveTabHostname()fails,tabHostnameis empty, so this conditional removes the new manual-check workflow on unsupported tabs.Suggested fix
- ${tabHostname ? renderManualLookupForm(tabHostname) : ''} + ${renderManualLookupForm(tabHostname)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entrypoints/popup/main.ts` around lines 592 - 603, The error page currently hides the manual lookup form when tabHostname is falsy; in renderError replace the conditional so renderManualLookupForm is always included (keep the header brand fallback when tabHostname is falsy), i.e. remove the `${tabHostname ? renderManualLookupForm(tabHostname) : ''}` guard and call `renderManualLookupForm(tabHostname)` unconditionally, leaving `bindManualLookupForm()` as-is so the manual-check workflow remains available even when `getActiveTabHostname()` fails; ensure `renderManualLookupForm` can handle an empty/undefined `tabHostname`.
🧹 Nitpick comments (1)
lib/checks/mailInfra.ts (1)
412-423: ⚡ Quick winRun optional MTA-STS policy fetch in parallel with the other checks.
When enabled,
checkMtaStsPolicyis awaited after the initialPromise.all, adding avoidable latency to every run.💡 Proposed refactor
- const [ns, mtaSts, tlsRpt, dnssec, m365Tenant] = await Promise.all([ + const mtaStsPolicyPromise = options?.fetchMtaStsPolicy + ? checkMtaStsPolicy(d) + : Promise.resolve<MailInfraCheck | null>(null); + + const [ns, mtaSts, tlsRpt, dnssec, m365Tenant, mtaStsPolicy] = await Promise.all([ checkNs(d, dns), checkMtaSts(d, dns), checkTlsRpt(d, dns), checkDnssec(d, dns), checkM365Tenant(d), + mtaStsPolicyPromise, ]); - if (!options?.fetchMtaStsPolicy) { + if (!mtaStsPolicy) { return [mx, ns, mtaSts, tlsRpt, dnssec, m365Tenant]; } - const mtaStsPolicy = await checkMtaStsPolicy(d); return [mx, ns, mtaSts, mtaStsPolicy, tlsRpt, dnssec, m365Tenant];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/checks/mailInfra.ts` around lines 412 - 423, The current code runs checkMtaStsPolicy only after the initial Promise.all, causing extra latency; instead start checkMtaStsPolicy together with the other checks when options?.fetchMtaStsPolicy is true by including checkMtaStsPolicy(d) in the Promise.all (or kick it off into its own promise before awaiting) and then await the combined results so the returned array uses the resolved mtaStsPolicy value; update the Promise.all call that currently contains checkNs, checkMtaSts, checkTlsRpt, checkDnssec, checkM365Tenant to conditionally include checkMtaStsPolicy(d) and adjust the returned tuple formation accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@entrypoints/popup/main.ts`:
- Around line 997-1015: The submit handler in bindManualLookupForm wrongly
mutates the global tabHostname so runCheck('apex') will update the actual
browser tab UI; instead preserve the active tab state by not changing
tabHostname or activeTabId for manual lookups—use a local/manual variable (e.g.,
manualHost) and pass that into the checking flow (or call a variant of runCheck
that accepts a host parameter) so bindManualLookupForm only updates
lastResult/compareResult/currentView for the popup UI without altering the real
tab's toolbar icon; update references to runCheck, tabHostname and any code that
reads tabHostname to support an explicit host parameter or a separate
manual-check path.
- Around line 346-363: The loadScopeComparison async handler can apply stale
compareResult and call renderResult(result) after a late await; to fix, guard
the response by capturing a short-lived identifier before the await (for example
capture a local token derived from result.tabHostname + otherMode or
incrementing a module-level compareRequestId) and after the await verify the
identifier/token still matches the current expected value before assigning to
compareResult and calling renderResult(result); if it no longer matches, discard
the response and return early. Ensure the token is checked both in the success
path (before setting compareResult and renderResult) and in the catch path to
avoid applying stale error state.
In `@entrypoints/popup/style.css`:
- Around line 829-833: The CSS rule for the selector .scope-compare__row span
uses the deprecated property value "word-break: break-word"; update this
declaration to use the standard property "overflow-wrap: break-word" instead
(replace the "word-break: break-word" line with "overflow-wrap: break-word") so
Stylelint's declaration-property-value-keyword-no-deprecated rule is satisfied
and equivalent wrapping behavior is preserved for the .scope-compare__row span
selector.
In `@lib/checks/mailInfra.ts`:
- Around line 246-250: The fetch in checkMtaStsPolicy currently has no timeout
and can hang; add an AbortController and a short timeout (e.g. 3–10s) to cancel
the fetch to mta-sts.{domain}/.well-known/mta-sts.txt, pass controller.signal
into fetch, and clear the timeout when the response completes; update the error
handling around the await fetch(...) and the res.ok branch to treat an
aborted/timed-out request as a failed check (or return a specific timeout
result) so the overall check flow cannot stall.
---
Outside diff comments:
In `@entrypoints/popup/main.ts`:
- Around line 1062-1077: When re-running DNS checks from the Settings view (the
block where currentView === 'settings' and dnsRefresh triggers runDnsCheck), you
update lastResult but leave compareResult stale; update the code in that branch
to clear the cached comparison (set compareResult to null/undefined) immediately
after a successful runDnsCheck (and before calling syncToolbarIconFromResult) so
comparisons use the new DNS-related settings (refer to runDnsCheck, lastResult,
compareResult, syncToolbarIconFromResult).
- Around line 592-603: The error page currently hides the manual lookup form
when tabHostname is falsy; in renderError replace the conditional so
renderManualLookupForm is always included (keep the header brand fallback when
tabHostname is falsy), i.e. remove the `${tabHostname ?
renderManualLookupForm(tabHostname) : ''}` guard and call
`renderManualLookupForm(tabHostname)` unconditionally, leaving
`bindManualLookupForm()` as-is so the manual-check workflow remains available
even when `getActiveTabHostname()` fails; ensure `renderManualLookupForm` can
handle an empty/undefined `tabHostname`.
---
Nitpick comments:
In `@lib/checks/mailInfra.ts`:
- Around line 412-423: The current code runs checkMtaStsPolicy only after the
initial Promise.all, causing extra latency; instead start checkMtaStsPolicy
together with the other checks when options?.fetchMtaStsPolicy is true by
including checkMtaStsPolicy(d) in the Promise.all (or kick it off into its own
promise before awaiting) and then await the combined results so the returned
array uses the resolved mtaStsPolicy value; update the Promise.all call that
currently contains checkNs, checkMtaSts, checkTlsRpt, checkDnssec,
checkM365Tenant to conditionally include checkMtaStsPolicy(d) and adjust the
returned tuple formation accordingly.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6de4602-dc2a-4508-acba-dddd9a1fb20c
📒 Files selected for processing (13)
README.mdentrypoints/background.tsentrypoints/popup/main.tsentrypoints/popup/style.csslib/checkDomain.tslib/checks/mailInfra.test.tslib/checks/mailInfra.tslib/checks/mailProviderSpfHint.tslib/mailProviders/definitions.tslib/parse/dkim.test.tslib/parse/dkim.tslib/settings.tswxt.config.ts
Comment on lines
+246
to
+250
| async function checkMtaStsPolicy(domain: string): Promise<MailInfraCheck> { | ||
| const url = `https://mta-sts.${domain}/.well-known/mta-sts.txt`; | ||
| try { | ||
| const res = await fetch(url, { cache: 'no-store' }); | ||
| if (!res.ok) { |
Contributor
There was a problem hiding this comment.
Add a timeout guard for the policy fetch.
Line 249 performs an external fetch without timeout/cancellation, so this path can hang and stall the whole check flow when the remote endpoint is slow.
💡 Proposed fix
async function checkMtaStsPolicy(domain: string): Promise<MailInfraCheck> {
const url = `https://mta-sts.${domain}/.well-known/mta-sts.txt`;
try {
- const res = await fetch(url, { cache: 'no-store' });
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 5000);
+ let res: Response;
+ try {
+ res = await fetch(url, {
+ cache: 'no-store',
+ signal: controller.signal,
+ });
+ } finally {
+ clearTimeout(timeoutId);
+ }
if (!res.ok) {
return {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function checkMtaStsPolicy(domain: string): Promise<MailInfraCheck> { | |
| const url = `https://mta-sts.${domain}/.well-known/mta-sts.txt`; | |
| try { | |
| const res = await fetch(url, { cache: 'no-store' }); | |
| if (!res.ok) { | |
| async function checkMtaStsPolicy(domain: string): Promise<MailInfraCheck> { | |
| const url = `https://mta-sts.${domain}/.well-known/mta-sts.txt`; | |
| try { | |
| const controller = new AbortController(); | |
| const timeoutId = setTimeout(() => controller.abort(), 5000); | |
| let res: Response; | |
| try { | |
| res = await fetch(url, { | |
| cache: 'no-store', | |
| signal: controller.signal, | |
| }); | |
| } finally { | |
| clearTimeout(timeoutId); | |
| } | |
| if (!res.ok) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/checks/mailInfra.ts` around lines 246 - 250, The fetch in
checkMtaStsPolicy currently has no timeout and can hang; add an AbortController
and a short timeout (e.g. 3–10s) to cancel the fetch to
mta-sts.{domain}/.well-known/mta-sts.txt, pass controller.signal into fetch, and
clear the timeout when the response completes; update the error handling around
the await fetch(...) and the res.ok branch to treat an aborted/timed-out request
as a failed check (or return a specific timeout result) so the overall check
flow cannot stall.
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
entrypoints/popup/main.ts (2)
348-376:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate compare requests when leaving the results view.
compareRequestIdis only bumped during DNS reruns. If the user clicks Settings while a compare is in flight, the late response still passes Line 364 andrenderResult(result)replaces the settings screen with stale results.Suggested fix
function bindSettingsFab(): void { document.getElementById('btn-open-settings')?.addEventListener('click', () => { + compareRequestId++; currentView = 'settings'; renderSettings(); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entrypoints/popup/main.ts` around lines 348 - 376, The compare flow in loadScopeComparison uses compareRequestId to detect stale responses but that ID is only incremented on DNS reruns, so navigating away (e.g., opening Settings) can allow a late compare to call renderResult(result) and overwrite the UI; fix by invalidating in-flight compares when leaving the results view — increment compareRequestId (or set an equivalent "viewActive" flag) from the code path that hides results/shows settings (the same place that renders the settings screen), and ensure loadScopeComparison still checks compareRequestId before calling renderResult; reference compareRequestId, loadScopeComparison, and renderResult when making this change.
1102-1123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
lastResultUpdatesToolbaron every toolbar mutation path.
updateToolbaronly gates the happy path at Lines 1121-1123. Manual lookups can still change the real tab icon via Line 595 (renderError()clears it), Lines 1065-1067 (toolbar-setting changes replaylastResult), and Line 875 (leaving Settings replayslastResult). That reintroduces the manual-lookup/active-tab icon bug on failure and replay paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@entrypoints/popup/main.ts` around lines 1102 - 1123, The toolbar-update flag lastResultUpdatesToolbar must be respected on every code path that mutates the toolbar: set lastResultUpdatesToolbar = updateToolbar at the top of runCheck (already present) and then update all places that currently change the tab icon or call syncToolbarIconFromResult or renderError to first check lastResultUpdatesToolbar; specifically, make renderError, the logic that replays lastResult on settings change (the block that calls syncToolbarIconFromResult around the former "toolbar-setting changes replay lastResult"), and the settings-exit replay path verify lastResultUpdatesToolbar before clearing or updating the toolbar icon so manual lookups do not get overridden. Ensure any early returns or error branches in runCheck also use lastResultUpdatesToolbar to decide whether to call syncToolbarIconFromResult or clear the icon.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@entrypoints/popup/main.ts`:
- Around line 348-376: The compare flow in loadScopeComparison uses
compareRequestId to detect stale responses but that ID is only incremented on
DNS reruns, so navigating away (e.g., opening Settings) can allow a late compare
to call renderResult(result) and overwrite the UI; fix by invalidating in-flight
compares when leaving the results view — increment compareRequestId (or set an
equivalent "viewActive" flag) from the code path that hides results/shows
settings (the same place that renders the settings screen), and ensure
loadScopeComparison still checks compareRequestId before calling renderResult;
reference compareRequestId, loadScopeComparison, and renderResult when making
this change.
- Around line 1102-1123: The toolbar-update flag lastResultUpdatesToolbar must
be respected on every code path that mutates the toolbar: set
lastResultUpdatesToolbar = updateToolbar at the top of runCheck (already
present) and then update all places that currently change the tab icon or call
syncToolbarIconFromResult or renderError to first check
lastResultUpdatesToolbar; specifically, make renderError, the logic that replays
lastResult on settings change (the block that calls syncToolbarIconFromResult
around the former "toolbar-setting changes replay lastResult"), and the
settings-exit replay path verify lastResultUpdatesToolbar before clearing or
updating the toolbar icon so manual lookups do not get overridden. Ensure any
early returns or error branches in runCheck also use lastResultUpdatesToolbar to
decide whether to call syncToolbarIconFromResult or clear the icon.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db5522eb-f954-4f40-97fa-c3ddc2f3a1e4
📒 Files selected for processing (5)
entrypoints/background.tsentrypoints/popup/main.tsentrypoints/popup/style.csslib/checkDomain.tslib/settings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- entrypoints/popup/style.css
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds several usability and diagnostics improvements to JayQuery while keeping the existing DNS-first workflow intact.
Added
Changed
MTA-STS Policy Fetch
The existing
_mta-stsDNS TXT check still runs as part of the normal mail infrastructure checks.This PR adds a separate optional HTTPS policy fetch for:
https://mta-sts./.well-known/mta-sts.txt
PR Size
This PR includes several related popup and mail-infrastructure improvements. If preferred, I’m happy to break it down into smaller PRs, for example:
Validation
npm testpasses: 20 test files, 152 testsnpm run buildpasses.output/chrome-mv3Summary by CodeRabbit