add cors to securtiy#40
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds end-to-end CORS support: config types and normalization, runtime handlers (headers/preflight/apply), security runtime wiring, CLI scaffolding to render/ensure config/cors.ts, dynamic project-config import fixes, tests, and documentation updates. ChangesCORS Support Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/cli/src/project/config.ts (1)
56-72: 💤 Low valueUnbounded import cache growth via incrementing nonce.
projectConfigImportNonceincrements on every call and is appended as?t=...to the import URL, which intentionally bypasses Node's ESM module cache. Each invocation therefore creates a brand-new module record that is never collected for the lifetime of the process. This is benign for one-shot CLI commands, but ifloadProjectConfigis invoked repeatedly (dev/watch mode, test harnesses, long-lived workers), the cache will grow without bound.Consider only busting the cache when the file's mtime/content has actually changed, or scope the nonce to "first load vs reload" rather than incrementing per call.
🤖 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 `@packages/cli/src/project/config.ts` around lines 56 - 72, The current global projectConfigImportNonce increments on every call in importProjectConfigFile and forces a fresh ESM import URL each time, leaking module records; change this to only bump the cache-busting token when the source file actually changes by tracking per-file state (e.g., a Map keyed by filePath storing last mtime or a content hash and a nonce) and use that per-file nonce instead of the global projectConfigImportNonce; update importProjectConfigFile to read the file mtime (or compute a short content hash) before composing the import URL and only update the stored nonce for that file when mtime/hash differs, keeping the rest of the logic (configureEnvRuntime, setting process.env entries, and resolveConfigExport) intact.
🤖 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 `@apps/docs/docs/security.md`:
- Around line 61-62: Update the misleading “single config entrypoint” sentence
in the security.md text to reflect that CORS now has its own config file;
specifically, mention both config/cors.ts and the other config entry (e.g., the
main config) as separate configuration entrypoints instead of a single one, by
editing the sentence that references a single config entrypoint so it states
that cross-origin rules are controlled by config/cors.ts while other settings
live in the primary config file.
In `@packages/cli/src/project/config.ts`:
- Around line 56-84: importProjectConfigFile mutates shared module state
(process.env and configureEnvRuntime) and can be racy; fix by serializing
concurrent invocations or removing direct process.env mutation: implement a
simple mutex/promise chain around importProjectConfigFile (use a module-scoped
Promise lock) so only one call at a time modifies process.env and
configureEnvRuntime, or remove the for-loop that writes process.env entirely and
rely on configureEnvRuntime to provide env values to the config loader;
additionally preserve previous configureEnvRuntime state instead of
unconditionally calling configureEnvRuntime(undefined) (save existing runtime
value at start and restore it in the finally block). Ensure references:
importProjectConfigFile, configureEnvRuntime, projectConfigImportNonce and
callers like loadProjectConfig are adjusted to use the serialized path or the
no-mutation approach.
In `@packages/security/src/cors.ts`:
- Around line 9-12: The matchesPathPattern function builds a RegExp from
developer-controlled patterns and can be vulnerable to ReDoS; add pattern
validation at config-normalization time (e.g., in the config normalizer that
prepares cors paths) to reject pathological patterns before they reach
matchesPathPattern: implement a validateCorsPathPattern that checks
wildcardCount (e.g., reject >3 wildcards) and pattern length (e.g., reject >200
chars) and call it for each configured pattern, or alternatively replace RegExp
construction in matchesPathPattern with a safe custom wildcard matcher that does
not use backtracking regexes (reference symbols: matchesPathPattern,
escapeRegex, and the config normalizer where cors paths are loaded).
---
Nitpick comments:
In `@packages/cli/src/project/config.ts`:
- Around line 56-72: The current global projectConfigImportNonce increments on
every call in importProjectConfigFile and forces a fresh ESM import URL each
time, leaking module records; change this to only bump the cache-busting token
when the source file actually changes by tracking per-file state (e.g., a Map
keyed by filePath storing last mtime or a content hash and a nonce) and use that
per-file nonce instead of the global projectConfigImportNonce; update
importProjectConfigFile to read the file mtime (or compute a short content hash)
before composing the import URL and only update the stored nonce for that file
when mtime/hash differs, keeping the rest of the logic (configureEnvRuntime,
setting process.env entries, and resolveConfigExport) intact.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2c81aec-69b6-4ee5-b208-de81c5677862
📒 Files selected for processing (28)
apps/docs/docs/auth/index.mdapps/docs/docs/security.mdpackages/cli/src/cli.tspackages/cli/src/project.tspackages/cli/src/project/config.tspackages/cli/src/project/registry.tspackages/cli/src/project/scaffold.tspackages/cli/src/project/scaffold/config-renderers.tspackages/cli/src/project/scaffold/framework.tspackages/cli/src/project/scaffold/project-renderers.tspackages/cli/src/project/shared.tspackages/cli/tests/cli.test.tspackages/config/src/defaults.tspackages/config/src/index.tspackages/config/src/loader.tspackages/config/src/type-helpers.tspackages/config/src/types.tspackages/config/tests/broadcast-config.type.test.tspackages/config/tests/config.type.test.tspackages/config/tests/security-config.test.tspackages/config/tests/security-config.type.test.tspackages/core/src/portable/holo.tspackages/core/tests/runtime.test.tspackages/security/src/contracts.tspackages/security/src/cors.tspackages/security/src/index.tspackages/security/src/runtime.tspackages/security/tests/package.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/security/src/cors.ts`:
- Around line 95-102: The response currently returns early when
resolveAllowedOrigin(config, origin) is falsy without adding a Vary: Origin
header, allowing caches to mistakenly serve the denied response to other
origins; update the branch where allowedOrigin is falsy to call
appendVary(result, 'Origin') (the same helper used later) before returning
result so that any response contingent on request.headers.get('origin')
advertises Vary: Origin and avoids cache poisoning; reference
resolveAllowedOrigin, appendVary, and the result response object to locate and
modify the code.
- Around line 119-126: The apply function is currently replacing any existing
Vary header when it merges CORS headers; update the merge so existing Vary
values on response.headers are preserved and combined with the CORS Vary values
from headers(request) instead of overwritten. In the apply function (referencing
nextHeaders and headers(request)), read the current Vary from response.headers
and the new Vary from headers(request), combine them into a de-duplicated,
comma-separated list (preserving existing tokens like Accept-Encoding,
Accept-Language), and set that combined string on nextHeaders before returning
the new Response.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d50309c0-8e9c-4d08-947c-3a13051aa60f
📒 Files selected for processing (5)
apps/docs/docs/security.mdpackages/cli/src/project/config.tspackages/cli/tests/project-config.test.tspackages/security/src/cors.tspackages/security/tests/package.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/docs/docs/security.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/src/project/config.ts
- packages/security/tests/package.test.ts
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 by CodeRabbit
New Features
Install/CLI
Documentation
Tests