Skip to content

✨ feat: warn about CORS preflight requests on Web - #2556

Merged
AlexV525 merged 2 commits into
mainfrom
feat/web-cors-preflight-warning
Jul 13, 2026
Merged

AlexV525 merged 2 commits into
mainfrom
feat/web-cors-preflight-warning

Conversation

@CaiJingLong

@CaiJingLong CaiJingLong commented Jul 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Detect when a request is not a CORS "simple request" and will trigger a preflight (OPTIONS) request on the Web platform, then emit a warningLog explaining why (non-safelisted method, custom headers, non-simple Content-Type, upload progress listener, or withCredentials).
  • Enrich the XMLHttpRequest.onError error message with CORS guidance when the request was preflighted, so users see an actionable hint instead of a generic network error.
  • Add an enableCORSWarning opt-out flag (defaults to true) so apps that handle CORS correctly can silence the per-request warning. The enriched error message is always emitted regardless of this setting.
  • Extract the detection logic into pure functions in lib/src/cors.dart (corsPreflightReason, collectCorsPreflightReasons, corsEnrichedErrorReason) so they can be unit-tested on the VM without a browser.

Closes #2201.

AI attribution

Implementation, tests, and PR workflow by Devin (GLM-5.2); local review pass by the human operator.

Verification

Added 23 unit tests covering corsPreflightReason (method / content-type / custom-header combinations), collectCorsPreflightReasons (upload-listener, withCredentials, combined cases), and corsEnrichedErrorReason (empty / non-empty / multi-reason). dart analyze --fatal-infos and dart test -p vm pass locally on dio_web_adapter.

Unverified: The browser-side runtime paths (actual warningLog emission, XMLHttpRequest.onError enrichment in a real browser) are not exercised locally — they require browser CI. The existing corsTests in dio_test cover CORS preflight classification behavior but do not assert on the new warning or enriched error message.

Detect when a request is not a CORS "simple request" and will trigger a
preflight (OPTIONS) request, then emit a warning log and enrich the
XMLHttpRequest.onError message with CORS guidance.

Closes #2201

Generated with [Devin](https://devin.ai)

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@CaiJingLong
CaiJingLong requested a review from a team as a code owner July 8, 2026 08:07
@CaiJingLong CaiJingLong mentioned this pull request Jul 8, 2026
4 of 7 tasks

@AlexV525 AlexV525 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for driving this — the extraction of corsPreflightReason into a pure, testable helper is a nice touch, the CHANGELOG entry is clean, and feat/web-cors-preflight-warning already matches the branch-naming convention that just landed in AGENTS.md. Left three inline comments on the substantive points; a few AGENTS.md formalities below.

AGENTS.md formalities (fixup, or a follow-up)

  • Commit / PR title should be gitmoji + Conventional per AGENTS.md §8.2 — ✨ feat: warn about CORS preflight requests on Web.
  • AI attribution disclosure per §8.3: Devin's Co-Authored-By trailer is present ✓, but if GLM-5.2 also produced code / tests / docs (as noted on #2549), it should have its own trailer. Please also add one line to the PR description stating which agent did which stage — design / implementation / tests / review.
  • Test-plan item 4 — “existing corsTests still cover runtime behavior” isn't quite right: those tests predate this PR and can't cover the newly-added warning + enriched-error paths. Either run browser CI and check the box, or replace the note with an honest “browser paths not exercised locally”. (Related: AGENTS.md §8.6 now prefers a prose description of what was actually verified over a boilerplate test-plan checklist — no need to restructure this PR, just heads-up for the next one.)

- Remove the public export of corsPreflightReason to avoid adding
  permanent public API surface for testing only.
- Extract collectCorsPreflightReasons and corsEnrichedErrorReason into
  pure functions in cors.dart so the upload-listener, withCredentials,
  and enriched-error branches are unit-testable without a browser.
- Add enableCORSWarning opt-out flag (defaults to true) so apps that
  handle CORS correctly can silence the per-request warning.
- Add 8 unit tests covering the new pure functions.

Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: GLM-5.2 <[email protected]>
@CaiJingLong CaiJingLong changed the title feat: warn about CORS preflight requests on Web ✨ feat: warn about CORS preflight requests on Web Jul 13, 2026
@CaiJingLong

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review — all three inline comments and the AGENTS.md formalities have been addressed in e69c194:

  1. Removed the export of corsPreflightReason — the test file now imports src/cors.dart directly, and no public API surface was added.

  2. Lifted the untested branches into pure functions — collectCorsPreflightReasons (combines corsPreflightReason with the upload-listener and withCredentials runtime factors) and corsEnrichedErrorReason (builds the enriched onError message) are now in cors.dart and unit-tested with 8 new tests. The adapter calls these pure functions, so every new branch is covered by VM-testable assertions.

  3. Added enableCORSWarning opt-out flag — defaults to true, but apps that handle CORS correctly can set BrowserHttpClientAdapter(enableCORSWarning: false). The enriched error message is always emitted regardless of this setting, so the diagnostic value is preserved even when the warning is silenced.

AGENTS.md formalities:

  • Commit title now uses gitmoji + Conventional format (✨).
  • Added Co-Authored-By: GLM-5.2 trailer and AI attribution line in the PR description.
  • Replaced the boilerplate test-plan checklist with an honest verification paragraph + an Unverified section for the browser-side runtime paths.

@github-actions

Copy link
Copy Markdown
Contributor

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
plugins/web_adapter/lib/src/adapter_impl.dart 🔴 19.27% 🔴 20.35% 🟢 1.08%
plugins/web_adapter/lib/src/cors.dart 🔴 0% 🟢 96.55% 🟢 96.55%
Overall Coverage 🟢 85.55% 🟢 85.64% 🟢 0.09%

Minimum allowed coverage is 0%, this run produced 85.64%

AlexV525 added a commit that referenced this pull request Jul 13, 2026
Introduces `AGENTS.md` (+ Simplified Chinese mirror) as this repo's
rules for AI-agent-assisted contributions, along with pointers from
`CONTRIBUTING` and a PR-template checklist item.

`AGENTS.md` is the de-facto standard filename that agent tools (Claude
Code, Codex, Cursor, Devin, etc.) pick up automatically, so agents on
external forks get these rules without extra setup.

### What the guidelines cover

1. **Motivation first** — every non-trivial change must answer four
questions (what can't be done today, who needs it, why in core rather
than an interceptor/adapter/transformer/plugin, cost). `Closes #NNNN`
alone does not count as prior discussion — the referenced issue must
show maintainer buy-in. One PR = one concern.
2. **Mandatory, effective, non-duplicated tests** — tests must fail
without the change; coverage minimum is a floor, not a target; no
test-theater for coverage numbers.
3. **Compatibility is sacred** — additive-by-default; deprecations with
`Use XXX instead. This will be removed in X.0.0` targeting the next
major (X is a placeholder — pick the actual next major from the affected
package's `pubspec.yaml`); no SDK-lower-bound bumps outside the
Compatibility Policy; watch behavioral breaks. Explicit
**sensitive-area** list (SSL/TLS + cert pinning, redirects, cookies,
headers, timeout/cancellation/connection pooling, interceptor pipeline,
request-body encoding) and **dependency-change** rules (no drive-by
bumps; state motivation; narrowest constraint; disclose new
transitives).
4. **Understand before you change** — no API guessing; cross-platform
pub-cache paths + `PUB_CACHE` env var.
5. **Production quality only** — no
`TODO`/mock/simplified-and-called-done code.
6. **When to stop and ask** — general rule for ambiguity and
out-of-scope work, distinct from §3's breaking-change escalation.
7. **Repository layout** — Melos mono-repo, per-package SDK lower
bounds.
8. **Commits, changelog, and PR hygiene**
   - §8.1 **Branch naming** — `category/ticket-id-or-short-description`
- §8.2 **Commit format** — gitmoji **or** Conventional Commits (either
is acceptable) with an emoji reference table matched to the repo's
actual usage
- §8.3 **AI attribution mandatory** — every contributing agent gets a
`Co-Authored-By:` trailer; the PR body discloses agent + stage (design /
implementation / tests / review)
   - §8.4 CHANGELOG rules
- §8.5 **Self-review your diff and commit message** — remove debug
prints, commented code, unrelated formatting; `git add .` / `-A`
forbidden; re-read the commit message against the staged diff so a
message from a prior attempt doesn't land verbatim
- §8.6 **Opening the PR** — draft PRs for large/exploratory changes;
**describe verification honestly** in prose rather than a boilerplate
"Test plan" checklist; anything genuinely un-verifiable goes under an
`Unverified` paragraph
- §8.7 **Review iteration** — append commits (no squash-force-push
mid-review); no close/reopen to reset state; design feedback is
conversation, not instruction; CI failures require log-reading, not
retry-until-green
9. **Patterns that may lead to closure** — cross-reference table linking
each anti-pattern back to the rule it violates, softened wording (`may`
/ `might`).

### Iteration notes

Follow-ups after the initial commit addressed:

- **@AmosHuKe** (first round) — cross-platform pub-cache paths and
`PUB_CACHE`, `「包」 → package` in the Chinese version, mainland-accessible
Melos URL, softened minor-version breaking-change wording per dio's own
CHANGELOG preamble.
- **@AmosHuKe** (second round) — the concrete `6.0.0` in the deprecation
example could mislead agents that don't check `pubspec.yaml`; replaced
with an `X.0.0` placeholder plus a note to derive the real next major
from the affected package.
- **@CaiJingLong via GLM-5.2** (first round) — expanded the workflow
gaps GLM flagged (commit format, self-review, review iteration,
when-to-ask, sensitive areas, dependency rules, branch naming, draft
PRs); collapsed the "patterns that may lead to closure" section from a
full restatement of §§1–8 into a cross-reference table.
- **@AlexV525** direct edit — softened §8.2 from "gitmoji **and**
Conventional Commits" to "gitmoji **or** Conventional Commits", matching
the actual mix in `git log` (recent commits use one or the other, not
both).
- **@CaiJingLong via GLM-5.2** (re-review) — flagged a duplicated §8.7
block (removed), a garbled §8.2 sentence in the English version
(rewritten to match the ZH), the `🥅` gitmoji being frequent in the repo
but missing from the reference table (added), and a missing §5 row in
the closure cross-reference table (added).
- **Self-application** — the "re-read the commit message against the
staged diff" clause in §8.5 was added after an actual near-miss on this
branch, where a stale message (`Fix deprecation hints`) was almost
applied to a different fix. Documenting the source in the commit and the
doc keeps the rule from looking arbitrary.
- **Stress-tested against #2556** (opened by @CaiJingLong via GLM-5.2 +
Devin right after this PR) — that PR validated the branch-naming
convention and motivated the "describe verification honestly" rule in
§8.6, after its Test-plan section carried a well-intentioned but
misleading "existing tests still cover" note on an untested new code
path.

### Files

- `AGENTS.md` — English rules
- `AGENTS-ZH.md` — Simplified Chinese mirror
- `CONTRIBUTING.md` / `CONTRIBUTING-ZH.md` — link to the new guidelines
- `.github/PULL_REQUEST_TEMPLATE.md` — checklist item pointing to
`AGENTS.md`

### Verification

Documentation change; no code paths to exercise. All internal
`§`-references were grep-verified for consistency after each renumbering
pass. The gitmoji reference table in §8.2 was cross-checked against the
last 300 commits (`git log --pretty=format:'%s' -300 | grep -oE
'^[^A-Za-z0-9 (]+' | sort | uniq -c`) to ensure the emojis it endorses
match the repo's real usage frequency.

---------

Co-authored-by: Claude <[email protected]>
@AlexV525
AlexV525 merged commit bfb871a into main Jul 13, 2026
5 checks passed
@AlexV525
AlexV525 deleted the feat/web-cors-preflight-warning branch July 13, 2026 07:33
@Waxo

Waxo commented Oct 21, 2026 •

Copy link
Copy Markdown

Options to disable warn has been removed. Can you add it back, dio is just spamming the logs.

@AlexV525

Copy link
Copy Markdown
Member

Options to disable warn has been removed. Can you add it back, dio is just spamming the logs.

@Waxo Could you elaborate? The flag still seems to be alive on the latest code:

this.enableCORSWarning = true,

@Waxo

Waxo commented Oct 22, 2026

Copy link
Copy Markdown

My bad got the wrong info my colleague said it was removed. Library wasn't up to date.
Just checked it too

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.

Prompt for not simple requests on Web

3 participants