📋 Review Summary

This PR implements a configurable CORS allowlist for the Qwen daemon, replacing the unconditional browser-origin rejection with an opt-in allowlist via --allow-origin <pattern>. The implementation is well-designed, thoroughly tested, and maintains backward compatibility. The code demonstrates strong security awareness with proper validation, strict parsing, and thoughtful edge-case handling.

🔍 General Feedback

Positive aspects:

• Excellent security-first design with strict validation and clear threat modeling
• Comprehensive test coverage (25+ tests for auth functions, 10+ for server integration)
• Bit-for-bit backward compatibility when --allow-origin is unset
• Clear documentation in code comments explaining the "why" behind decisions
• Proper separation of concerns: validation at boot, middleware at runtime, capability advertisement
• Thoughtful handling of edge cases (Origin: null, empty arrays, wildcard + no-token scenario)

Architecture decisions:

• Single-middleware ownership of CORS policy is the right pattern
• Boot-time validation prevents runtime surprises
• Conditional capability tag (allow_origin) allows client pre-flighting without leaking the allowlist
• Echoing origin verbatim (not literal *) paired with Vary: Origin follows CORS best practices

🎯 Specific Feedback

🟢 Medium

1. File: packages/cli/src/serve/auth.ts:84 - The parseAllowOriginPatterns function lowercases origins for case-insensitive matching per RFC 6454 §4, which is correct. However, the comment mentions "scheme/host lowercase per RFC 6454 §4" but the code lowercases the entire origin including the port. While this works correctly (ports aren't case-sensitive), the comment could be more precise about what's being lowercased and why.

   Suggestion: Clarify comment to: "Lowercase the entire origin for case-insensitive comparison (scheme and host are case-insensitive per RFC 6454; port is preserved numerically but string-lowercase is safe for digits)."

2. File: packages/cli/src/serve/auth.ts:147-157 - The Origin: null rejection includes a detailed comment explaining the security rationale (sandboxed iframes, file:// docs). This is excellent, but the comment speculates about future opt-in: "Operators who genuinely need null origins... can ask for an opt-in flag if/when that materializes."

   Suggestion: Consider removing the speculative future-flag language to avoid creating an expectation. The current strict behavior is well-justified on its own.

3. File: packages/cli/src/serve/runQwenServe.ts:374-393 - The boot validation for --allow-origin '*' without a token is excellent. However, the error message is quite long (5 lines of string concatenation).

   Suggestion: Consider breaking this into a more readable template literal or multi-line string for maintainability, though the current approach is functional.

🔵 Low

1. File: packages/cli/src/serve/auth.ts:56-62 - The InvalidAllowOriginPatternError constructor builds an error message with detailed guidance. The message is comprehensive but could benefit from including the canonical form when the pattern is a valid URL but has extra path/query components (the common typo case).

   Suggestion: When parsed.origin !== entry, include parsed.origin in the error message to show operators exactly what the correct value should be. Example:

   throw new InvalidAllowOriginPatternError(
     entry,
     `expected the canonical origin ${JSON.stringify(parsed.origin)} ` +
       'without trailing slash, path, userinfo, or query',
   );

   This is already done in the code—just noting it's a good pattern that could be emphasized more in the class-level JSDoc.

2. File: packages/cli/src/serve/capabilities.ts:205 - The allow_origin capability is added to the registry with since: 'v1'. This is correct since it's part of the v1 protocol surface, but it's worth noting that clients checking for this feature will need to handle both its presence and absence gracefully (which the code already does via the conditional advertisement).

   Suggestion: Consider adding a comment in the JSDoc noting that clients should handle the absence of this tag as "CORS not configured" rather than "protocol version too old."

3. File: packages/cli/src/commands/serve.ts:149-162 - The CLI flag description is comprehensive but quite long. The example is helpful but takes up significant space.

   Suggestion: Consider moving the detailed example to the user documentation and keeping the CLI help more concise. This is a minor style point—the current approach is functional and informative.

4. File: packages/cli/src/serve/server.test.ts:5246-5260 - The test for "demo self-origin shim still works when --allow-origin is set" is an excellent regression anchor. The comment explains the rationale well. Consider adding a similar regression test for the case where opts.allowOrigins is explicitly set to an empty array ([]) to ensure it behaves identically to undefined.

   Note: Actually, I see this test exists at line 5320-5337 ("empty allowOrigins array behaves identically to undefined"). This is great—just noting the pattern is applied consistently.

5. File: docs/users/qwen-serve.md:214 - The documentation table entry for --allow-origin is comprehensive. One minor suggestion: the phrase "Subdomain wildcards (https://*.example.com) are intentionally unsupported" could benefit from a brief explanation of why (e.g., "to prevent accidental over-permissioning; list each subdomain explicitly or use * with --require-auth").

   Note: The documentation does mention using * paired with --require-auth as an alternative, which is good. The rationale is implicit but could be more explicit.

✅ Highlights

1. Security-first design: The boot-time validation that refuses --allow-origin '*' without a bearer token is an excellent safety mechanism. This prevents operators from accidentally exposing the daemon to any cross-origin request without authentication.

2. Comprehensive test coverage: The test suite covers:

   • Pattern parsing (happy path, case insensitivity, wildcard, mixed patterns)
   • Every rejection branch (trailing slash, path, userinfo, non-URL, empty hostname, malformed entries)
   • CORS middleware behavior (matched/unmatched origins, OPTIONS preflight, Origin: null, wildcard)
   • Integration tests (capability advertisement, demo shim compatibility, empty array regression)

3. Clear threat modeling: Comments throughout the code explain the security rationale, such as:

   • Why Origin: null is always rejected (sandboxed iframe attack vector)
   • Why the allowlist isn't exposed in /capabilities (enumeration risk)
   • Why echoing the origin verbatim (not *) is correct for browser caching

4. Backward compatibility: The implementation preserves bit-for-bit behavior when --allow-origin is unset, ensuring existing deployments aren't affected.

5. Documentation quality: Both inline comments and user-facing documentation are thorough, explaining not just what the feature does but why design decisions were made.

6. Proper middleware ordering: The CORS middleware is installed before hostAllowlist and bearerAuth, ensuring it runs early in the request chain. The OPTIONS preflight short-circuit is safe because the actual request still runs through the full chain.

7. Cache-aware headers: Setting Vary: Origin on both match and reject paths prevents intermediaries from serving stale responses to different origins—a subtle but important detail for production deployments behind proxies.

Thanks — agreed, the docs/help/warning overstate the boot check. The actual gate is token configured (any source: --token / env / --require-auth), not --require-auth specifically. Will update the 5 affected spots (CLI help text, runQwenServe stderr breadcrumb, qwen-serve.md flag table + security section, qwen-serve-protocol.md --allow-origin section) to align with the implementation rather than tighten the code — the current gate already covers the real API surface, and forcing --require-auth would break legitimate * + token + loopback dev workflows that want to keep /health reachable for operator probes.

✅ Local verification report — PR 4527

Tested as the maintainer reviewer ahead of merge. Verified at head 6d61549cd (3 commits ahead of daemon_mode_b_main), against a fresh worktree on Linux, in a dedicated tmux session.

Build

Unit tests

npm test -w @qwen-code/qwen-code -- src/serve/auth.test.ts src/serve/server.test.ts

→ 294 / 294 passed in 13.77s (PR description said 289 — the extra 5 were added in 6d61549cd "fix(serve): address allow-origin review feedback", so the suite grew on top of the original count).

Live daemon smoke — flag set (--allow-origin http://localhost:3000 --allow-origin http://localhost:5173)

Boot-time * + token gate

Malformed-pattern boot rejection (all exit 1)

serve --help

T15 — --allow-origin advertised with full description including the boot-refuse-on-* invariant and the caps.features.allow_origin pre-flight pointer. ✅

Backward-compat regression (flag unset)

The "no flag → bit-for-bit prior behavior" claim in the PR description holds end-to-end.

Observations for the merge decision

• Single-middleware ownership of the CORS policy is what's actually shipped — denyBrowserOriginCors is replaced, not layered, when --allow-origin is set. Confirmed via packages/cli/src/serve/server.ts:526-539 (if (opts.allowOrigins?.length) … else { app.use(denyBrowserOriginCors); }).
• Defense in depth on the *+no-token invariant: the gate runs both at boot in runQwenServe.ts:374-388 and again inside createApp (server.ts:528-535) for embedded callers that construct the app directly. T13a observed the boot one; embedded callers cannot bypass it.
• No header leakage on reject (T9) is the property that makes the unmatched-403 a true CORS denial, not an over-eager allow + downstream rejection.
• The Origin: null rejection (T6) holds even when * is configured — confirms the carve-out for sandboxed-iframe/file:// attack vectors.
• Lint warnings on the build are all pre-existing (not introduced by this PR's files).

Verdict

Implementation behaves exactly as the PR description and follow-up review thread describe. Backward-compat regression passes. No surprises in the live daemon. Recommendation: ready to merge once the daemon_mode_b_main integration branch is otherwise green.

_{Verified locally on Linux 6.12.63 / Node from repo node_modules, worktree at qwen-code-pr4527, tmux session pr4527. Daemon bound to 127.0.0.1:4170, workspace /tmp.}