Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 7e2677d The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No documentation updates needed for this PR. This is an internal CORS security improvement that gates localhost origins to development/test environments only—no user-facing API or configuration changes. |
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: Medium
🔴❗ Critical (1) ❗🔴
🔴 1) agents-api/src/__tests__/utils/cors.test.ts Missing ENVIRONMENT mock breaks existing tests and lacks production rejection coverage
files:
agents-api/src/__tests__/utils/cors.test.tsagents-api/src/__tests__/manage/utils/cors.test.ts(duplicate test file)
Issue: The PR adds a new condition env.ENVIRONMENT === 'development' || env.ENVIRONMENT === 'test' to the localhost CORS check, but the existing CORS tests mock the env module without including ENVIRONMENT. Since the tests use vi.doMock('../../env', () => ({ env: {...} })) to completely replace the env export, env.ENVIRONMENT will be undefined during test execution — causing the new localhost condition to evaluate to false. This likely breaks existing tests that expect localhost origins to be allowed. Additionally, there are no tests that verify localhost is rejected in production or pentest environments, which is the core purpose of this security change.
Why: This is a security-related change intended to prevent localhost origins from being accepted in production environments. Without:
- Fixing the existing mocks to include
ENVIRONMENT: 'development'orENVIRONMENT: 'test' - Adding explicit tests for
ENVIRONMENT: 'production'andENVIRONMENT: 'pentest'that verify localhost rejection
...a future developer could refactor the condition (e.g., invert the logic, remove the environment check) and the tests would either fail for the wrong reason or pass without verifying the security behavior.
Fix:
- Update existing test mocks to include
ENVIRONMENT:
// In "development mode (localhost API)" describe block (line 81-86)
vi.doMock('../../env', () => ({
env: {
INKEEP_AGENTS_API_URL: 'http://localhost:3002',
INKEEP_AGENTS_MANAGE_UI_URL: undefined,
ENVIRONMENT: 'development', // Add this
},
}));- Add new test cases for production/pentest rejection:
describe('production mode rejects localhost origins', () => {
beforeEach(() => {
vi.doMock('../../env', () => ({
env: {
ENVIRONMENT: 'production',
INKEEP_AGENTS_API_URL: 'https://api.agents.inkeep.com',
INKEEP_AGENTS_MANAGE_UI_URL: 'https://app.inkeep.com',
},
}));
});
it('should REJECT localhost origins in production', async () => {
const { isOriginAllowed } = await import('../../middleware/cors');
expect(isOriginAllowed('http://localhost:3000')).toBe(false);
expect(isOriginAllowed('http://127.0.0.1:3000')).toBe(false);
});
it('should still allow configured UI URL in production', async () => {
const { isOriginAllowed } = await import('../../middleware/cors');
expect(isOriginAllowed('https://app.inkeep.com')).toBe(true);
});
});
describe('pentest mode rejects localhost origins', () => {
beforeEach(() => {
vi.doMock('../../env', () => ({
env: {
ENVIRONMENT: 'pentest',
INKEEP_AGENTS_API_URL: 'https://api.agents.inkeep.com',
INKEEP_AGENTS_MANAGE_UI_URL: 'https://app.inkeep.com',
},
}));
});
it('should REJECT localhost origins in pentest', async () => {
const { isOriginAllowed } = await import('../../middleware/cors');
expect(isOriginAllowed('http://localhost:3000')).toBe(false);
expect(isOriginAllowed('http://127.0.0.1:3000')).toBe(false);
});
});Refs:
- cors.test.ts:81-86 — mock missing ENVIRONMENT
- cors.ts:42-47 — new environment check
- vitest.config.ts:28 — ENVIRONMENT set to 'test' globally
- workAppsAuth.ts:33-49 — existing pattern for environment-gated localhost
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
.changeset/smart-clocks-cover.md:5Changeset message truncated mid-sentence
🚫 REQUEST CHANGES
Summary: The CORS security hardening is a good change that aligns with existing patterns (e.g., workAppsAuth.ts). However, the existing test suite needs to be updated to: (1) include ENVIRONMENT in the mocked env objects so tests don't break, and (2) add explicit test coverage for localhost rejection in production/pentest environments to validate the security behavior being introduced.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
agents-api/src/middleware/cors.ts |
Security assessment of the change itself | Validated as correct — the change improves security by gating localhost to dev/test only. No security concerns found. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
2 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-devops |
3 | 0 | 0 | 0 | 1 | 0 | 1 |
pr-review-security-iam |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
| Total | 7 | 1 | 0 | 0 | 1 | 0 | 2 |
Note: Findings from pr-review-standards, pr-review-tests, and pr-review-devops about test coverage were semantically deduplicated into a single Critical finding since they all addressed the same underlying issue.
| "@inkeep/agents-api": patch | ||
| --- | ||
|
|
||
| Now localhost origins are only allowed when ENVIRONMENT is development or test. In production/pentest, CORS will require either INKEEP_AGENTS_MANAGE_UI_URL to be set or the origin to share the same base |
There was a problem hiding this comment.
🟡 Minor: Changeset message truncated
Issue: The changeset description ends mid-sentence: "...or the origin to share the same base" — missing the completion.
Why: Incomplete changelog messages can confuse package consumers trying to understand the release behavior.
Fix: (1-click apply)
| Now localhost origins are only allowed when ENVIRONMENT is development or test. In production/pentest, CORS will require either INKEEP_AGENTS_MANAGE_UI_URL to be set or the origin to share the same base | |
| Now localhost origins are only allowed when ENVIRONMENT is development or test. In production/pentest, CORS will require either INKEEP_AGENTS_MANAGE_UI_URL to be set or the origin to share the same base domain as the API. |
Refs:
…t card CORS tests: The cors middleware now gates localhost/127.0.0.1 origins on ENVIRONMENT being 'development' or 'test' (PR #2066). Test mocks were missing this field, causing false rejections. E2E test: Use a[href*="/agents/activities-planner"] instead of text=Activities planner to avoid matching the project heading/breadcrumb. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The CORS middleware now gates localhost/127.0.0.1 origins on ENVIRONMENT === 'development' || 'test' (from PR #2066). The duplicate test file at __tests__/manage/utils/ was missing this mock value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Commit 37e72ed gated localhost CORS on env.ENVIRONMENT but did not update the test mocks, causing 4 CORS tests to fail in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* chore: improve CI performance by upgrading runner and removing test overhead - Upgrade ci job runner from ubuntu-latest to ubuntu-16gb for more resources - Remove OpenTelemetry NodeSDK initialization from test setup (was creating full auto-instrumentation per worker thread with no benefit in unit tests) - Reduce agents-api vitest maxThreads from 10 to 8 and minThreads from 4 to 2 to better match runner core count and reduce per-worker initialization cost Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: remove unused OTel test devDependencies Remove @opentelemetry/exporter-trace-otlp-proto and @opentelemetry/sdk-metrics from agents-api devDependencies since they were only used in the test setup OTel initialization that was removed in the previous commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * perf: fix turbo cache cascade invalidation Three targeted fixes to prevent catastrophic cache invalidation in CI: 1. Exclude test files from build task inputs - prevents test file changes in core packages from cascading build hash invalidation to all 11+ downstream packages. Uses officially documented $TURBO_DEFAULT$ with negation globs (turbo 1.12+). 2. Remove transit dependency from lint task - transit is a no-op coordination task (no package defines a transit script) but its hash changes on any file change, cascading to all downstream lint tasks. Lint only reads local source files and doesn't need dependency ordering. 3. Move TURBO_TOKEN/TURBO_TEAM to job-level env - ensures all turbo invocations (check, knip) use remote cache, not just pnpm check. Also adds timeout-minutes: 30 as a safety guardrail. Evidence: PR #2068 changed 2 test files but caused 36/45 cache misses. With these fixes, the same change would cause ~6 misses (only the directly affected packages). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: auto-format with biome * fix: add ENVIRONMENT to CORS test mocks broken by #2066 Commit 37e72ed gated localhost CORS on env.ENVIRONMENT but did not update the test mocks, causing 4 CORS tests to fail in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: add explicit permissions block to CI workflow Matches the pattern used by release.yml, ci-maintenance.yml, and stale.yml. Documents intent and prevents unintended privilege escalation if the workflow is later modified. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
No description provided.