fix(webapp): prevent duplicate preview env image tags #2475
Conversation
|
WalkthroughgetDeploymentImageRef’s signature changed to accept environmentType (EnvironmentType) and deploymentShortCode (string). The image tag construction now uses environmentType.toLowerCase() and deploymentShortCode (envType.deploymentShortCode) instead of the previous environmentSlug or a random suffix. initializeDeployment.server.ts now generates a single deploymentShortCode (nanoid) and passes it into getDeploymentImageRef and deployment creation. Tests were updated to use environmentType, include deploymentShortCode values, rework ECR gating to RUN_ECR_TESTS, adjust non-ECR hosts, expand teardown guarding, and add assertions around repo creation/reuse and image-ref uniqueness. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (2)
14-18: Centralize nanoid generation and stick to subpath imports.
- To avoid duplicating the same customAlphabet config across files and improve consistency, extract a shared generator (e.g., nanoid8) and use it here.
- Also, per webapp guideline, avoid importing from @trigger.dev/core root; confirm the correct subpath that exports tryCatch and switch to it.
Apply in this file:
-import type { EnvironmentType } from "@trigger.dev/core/v3"; -import { customAlphabet } from "nanoid"; - -const nanoid = customAlphabet("1234567890abcdefghijklmnopqrstuvwxyz", 8); +import type { EnvironmentType } from "@trigger.dev/core/v3"; +import { nanoid8 } from "../utils/id";- const randomSuffix = nanoid(); + const randomSuffix = nanoid8();New utility (outside this diff):
// apps/webapp/app/v3/utils/id.ts import { customAlphabet } from "nanoid"; export const nanoid8 = customAlphabet("1234567890abcdefghijklmnopqrstuvwxyz", 8);Additionally, verify and, if valid, change:
// Before import { tryCatch } from "@trigger.dev/core"; // After (if available) import { tryCatch } from "@trigger.dev/core/v3";Also applies to: 120-123
120-123: Confirm intended tag format vs PR description; consider doc update.The new tag is version.envType.randomSuffix (e.g., 20250903.1.preview.abcdef12). The PR description example shows version.preview-some-branch.randomSuffix. If dropping the branch slug is intentional, update the PR text and any docs; if not, we can optionally include a sanitized short branch fragment for PREVIEW.
Note: Adding a suffix increases tag cardinality; ensure ECR lifecycle policies keep repo size under control.
apps/webapp/test/getDeploymentImageRef.test.ts (2)
11-12: Escape all dynamic regex parts, not only host.Minor: testNamespace and testProjectRef are also interpolated; future values could include regex metacharacters. Use a general escaper or a small dev dep like escape-string-regexp.
-const escapeHostForRegex = (host: string) => host.replace(/\./g, "\\."); +const escapeForRegex = (s: string) => + s.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");Then wrap host, namespace, and projectRef with escapeForRegex in your patterns.
141-166: Also assert repoCreated is false on reuse for completeness.This makes the reuse behavior explicit.
expect(imageRef.isEcr).toBe(true); + expect(imageRef.repoCreated).toBe(false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/webapp/app/v3/getDeploymentImageRef.server.ts(2 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(1 hunks)apps/webapp/test/getDeploymentImageRef.test.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/getDeploymentImageRef.server.tsapps/webapp/test/getDeploymentImageRef.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/getDeploymentImageRef.server.tsapps/webapp/test/getDeploymentImageRef.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/getDeploymentImageRef.server.tsapps/webapp/test/getDeploymentImageRef.test.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/v3/services/initializeDeployment.server.tsapps/webapp/app/v3/getDeploymentImageRef.server.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Our tests are all vitest
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}: Do not import app/env.server.ts into tests, either directly or indirectly
Tests should only import classes/functions from files under apps/webapp/app/**/*.ts
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests must use Vitest
Tests should avoid mocks or stubs and use helpers from @internal/testcontainers when Redis or Postgres are needed
Test files live beside the files under test and should use descriptive describe and it blocks
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
🧬 Code graph analysis (3)
apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (1)
environment(2178-2201)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
apps/webapp/app/v3/registryConfig.server.ts (1)
RegistryConfig(3-11)
apps/webapp/test/getDeploymentImageRef.test.ts (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
getDeploymentImageRef(104-158)
🪛 ast-grep (0.38.6)
apps/webapp/test/getDeploymentImageRef.test.ts
[warning] 76-80: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( "registry.example.com" )}/${testNamespace}/${testProjectRef}:20250630\\.1\\.development\\.[a-z0-9]{8}$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 104-108: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( testHost )}/${testNamespace}/${testProjectRef2}:20250630\\.1\\.development\\.[a-z0-9]{8}$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 129-133: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( testHost )}/${testNamespace}/${testProjectRef2}:20250630\\.2\\.development\\.[a-z0-9]{8}$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 158-162: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( testHost )}/${testNamespace}/${testProjectRef}:20250630\\.2\\.production\\.[a-z0-9]{8}$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 204-208: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( "registry.example.com" )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.[a-z0-9]{8}$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 211-215: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( "registry.example.com" )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.[a-z0-9]{8}$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
108-114: All call sites successfully migrated to useenvironmentType
Verified no remaining references toenvironmentSlugor calls missing theenvironmentTypeparameter.apps/webapp/app/v3/services/initializeDeployment.server.ts (1)
81-82: LGTM: call site updated to environment.type.Matches the new getDeploymentImageRef signature and lowercasing occurs inside the callee.
apps/webapp/test/getDeploymentImageRef.test.ts (5)
30-31: Good test cleanup gating.Skipping cleanup when RUN_ECR_TESTS !== "1" avoids unnecessary AWS calls in local runs.
59-84: Regex-based assertion reads clearly and is robust.Switch to environmentType works; the tag shape check is precise.
86-139: ECR happy-path test looks solid.Repo creation vs reuse assertions are correct and the tag regex matches the new format.
168-219: Nice: uniqueness test covers the original collision bug.Validates suffix randomness and format.
222-259: Auth test gating is appropriate.Tying ECR auth tests to RUN_ECR_TESTS keeps CI noise down.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
105-113: API change: ensure all call sites pass deploymentShortCodeSignature now requires environmentType and deploymentShortCode. Verify no callers still pass environmentSlug or omit deploymentShortCode.
This also addresses the earlier feedback about using a stable short code instead of a random suffix; please confirm deploymentShortCode maps to WorkerDeployment.shortCode at creation time.
#!/bin/bash # Find old env prop usage rg -nP --type=ts '\benvironmentSlug\b' # List all getDeploymentImageRef call sites lacking deploymentShortCode in the arg object rg -nP --type=ts -n 'getDeploymentImageRef\s*\(\s*{([^}]*)}\s*\)' -U \ | awk -F: '{print $1":"$2}' | sort -u | while read loc; do file="${loc%%:*}"; line="${loc##*:}" # show the callsite block sed -n "$((line-5)),$((line+15))p" "$file" | awk 'BEGIN{f=0}/getDeploymentImageRef/{f=1} f{print} /}/{if(f){print ""; exit}}' \ | grep -q 'deploymentShortCode' || echo "MISSING deploymentShortCode -> $file:$line" done
🧹 Nitpick comments (6)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (2)
119-121: New tag format prevents collisions; add light validation for Docker tag safetyGuard against accidental invalid characters in deploymentShortCode to avoid push failures.
- const envType = environmentType.toLowerCase(); - const imageRef = `${registry.host}/${repositoryName}:${nextVersion}.${envType}.${deploymentShortCode}`; + const envType = environmentType.toLowerCase(); + // Docker/ECR tag charset: letters, digits, underscore, period, dash + if (!/^[A-Za-z0-9_.-]+$/.test(deploymentShortCode)) { + throw new Error("Invalid deploymentShortCode for image tag"); + } + const imageRef = `${registry.host}/${repositoryName}:${nextVersion}.${envType}.${deploymentShortCode}`;
11-11: Follow webapp guideline: avoid root import from @trigger.dev/corePrefer documented subpath exports for webapp imports.
-import { tryCatch } from "@trigger.dev/core"; +// If available from subpath, prefer that (adjust to the correct subpath export) +import { tryCatch } from "@trigger.dev/core/v3";If tryCatch isn’t exported from /v3, ignore this change or re-export it there for consistency.
apps/webapp/test/getDeploymentImageRef.test.ts (4)
11-12: Helper escapes only dots; prefer direct string equality instead of regexRegex isn’t needed here and triggers static-analysis noise. Build the expected string and assert with toBe.
-const escapeHostForRegex = (host: string) => host.replace(/\./g, "\\."); +const buildExpectedRef = (host: string, ns: string, proj: string, ver: string, env: string, code: string) => + `${host}/${ns}/${proj}:${ver}.${env.toLowerCase()}.${code}`;
76-83: Replace dynamic regex assertions with exact string checksRemoves ReDoS lint warnings and simplifies assertions.
Example for the first assertion:
- expect(imageRef.imageRef).toMatch( - new RegExp( - `^${escapeHostForRegex("registry.example.com")}/${testNamespace}/${testProjectRef}:20250630\\.1\\.development\\.test1234$` - ) - ); + expect(imageRef.imageRef).toBe( + buildExpectedRef("registry.example.com", testNamespace, testProjectRef, "20250630.1", "development", "test1234") + );Apply similarly to the other imageRef checks.
Also applies to: 106-113, 132-138, 162-168, 210-216, 217-223
144-170: Avoid inter-test dependency on prior creationThis test relies on the previous test having created the repo. Consider moving repo creation into a beforeAll within the RUN_ECR_TESTS block for isolation.
172-225: Comment tweak: not “random suffix” anymoreUpdate the comment to reflect deploymentShortCode.
- // Even with the same environment type and version, the image refs should be different due to random suffix + // Even with the same environment type and version, the image refs differ due to the deploymentShortCode
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/webapp/app/v3/getDeploymentImageRef.server.ts(2 hunks)apps/webapp/app/v3/services/initializeDeployment.server.ts(2 hunks)apps/webapp/test/getDeploymentImageRef.test.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/v3/services/initializeDeployment.server.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.tsapps/webapp/test/getDeploymentImageRef.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.tsapps/webapp/test/getDeploymentImageRef.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.tsapps/webapp/test/getDeploymentImageRef.test.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/v3/getDeploymentImageRef.server.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Our tests are all vitest
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}: Do not import app/env.server.ts into tests, either directly or indirectly
Tests should only import classes/functions from files under apps/webapp/app/**/*.ts
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests must use Vitest
Tests should avoid mocks or stubs and use helpers from @internal/testcontainers when Redis or Postgres are needed
Test files live beside the files under test and should use descriptive describe and it blocks
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
🧬 Code graph analysis (2)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
apps/webapp/app/v3/registryConfig.server.ts (1)
RegistryConfig(3-11)
apps/webapp/test/getDeploymentImageRef.test.ts (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
getDeploymentImageRef(101-156)
🪛 ast-grep (0.38.6)
apps/webapp/test/getDeploymentImageRef.test.ts
[warning] 77-81: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( "registry.example.com" )}/${testNamespace}/${testProjectRef}:20250630\\.1\\.development\\.test1234$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 106-110: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( testHost )}/${testNamespace}/${testProjectRef2}:20250630\\.1\\.development\\.test1234$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 132-136: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( testHost )}/${testNamespace}/${testProjectRef2}:20250630\\.2\\.development\\.test1234$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 162-166: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( testHost )}/${testNamespace}/${testProjectRef}:20250630\\.2\\.production\\.test1234$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 210-214: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( "registry.example.com" )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.test1234$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 217-221: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^${escapeHostForRegex( "registry.example.com" )}/${testNamespace}/${testProjectRef}:${sameVersion}\\.preview\\.test4321$
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
14-14: Typed env import — LGTMUsing EnvironmentType from the v3 subpath aligns with the API change.
apps/webapp/test/getDeploymentImageRef.test.ts (5)
30-33: Cleanup guard — LGTMSkips deletion when KEEP_TEST_REPO=1 or RUN_ECR_TESTS!=1. Sensible.
62-74: Non‑ECR case updated to environmentType/deploymentShortCode — LGTM
87-115: ECR repo creation path — LGTMGood gating with it.skipIf and expectations for isEcr/repoCreated.
116-141: Second ECR call validates reuse — LGTM
228-228: Auth tests gated on RUN_ECR_TESTS — LGTM
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webapp/test/getDeploymentImageRef.test.ts (1)
263-304: Add coverage for backslashes and equals in tag values (ties to GHAS finding).Extend tests to ensure
parseRegistryTagspreserves backslashes and supports '=' inside values (split-on-first '=').describe("parseRegistryTags", () => { @@ it("should skip invalid tags", () => { expect(parseRegistryTags("=value,key1,=,key2=value2")).toEqual([ { Key: "key1", Value: "" }, { Key: "key2", Value: "value2" }, ]); }); + + it("should allow backslashes and '=' in values", () => { + // String.raw keeps backslashes as typed + const input = String.raw`path=C:\tmp\dir,name=va=l\ue`; + expect(parseRegistryTags(input)).toEqual([ + { Key: "path", Value: "C:\\tmp\\dir" }, + { Key: "name", Value: "va=l\\ue" }, + ]); + }); });
🧹 Nitpick comments (3)
apps/webapp/test/getDeploymentImageRef.test.ts (3)
130-152: Test name vs. behavior mismatch; assert reuse on the same repo.This test uses
testProjectRefwhile the repo created earlier istestProjectRef2, and it doesn’t assertrepoCreated === false. Align it with reuse semantics.Apply:
-it.skipIf(process.env.RUN_ECR_TESTS !== "1")("should reuse existing ECR repository", async () => { - // This should use the repository created in the previous test +it.skipIf(process.env.RUN_ECR_TESTS !== "1")( + "should reuse existing ECR repository for same projectRef", + async () => { const imageRef = await getDeploymentImageRef({ registry: { host: testHost, namespace: testNamespace, username: "test-user", password: "test-pass", ecrTags: registryTags, ecrAssumeRoleArn: roleArn, ecrAssumeRoleExternalId: externalId, }, - projectRef: testProjectRef, - nextVersion: "20250630.2", + projectRef: testProjectRef2, + nextVersion: "20250630.3", environmentType: "PRODUCTION", deploymentShortCode: "test1234", }); - expect(imageRef.imageRef).toBe( - `${testHost}/${testNamespace}/${testProjectRef}:20250630.2.production.test1234` - ); + expect(imageRef.imageRef).toBe( + `${testHost}/${testNamespace}/${testProjectRef2}:20250630.3.production.test1234` + ); expect(imageRef.isEcr).toBe(true); + expect(imageRef.repoCreated).toBe(false); -}); + } +);
154-199: Uniqueness test captures the regression; consider a minimal tag-shape check.Optional: also assert tag charset to catch illegal characters in
deploymentShortCode.Example:
expect(firstImageRef.imageRef).not.toBe(secondImageRef.imageRef); + // Docker tag-safe chars ([A-Za-z0-9_.-]) across the tag part + const tagPart1 = firstImageRef.imageRef.split(":")[1]!; + const tagPart2 = secondImageRef.imageRef.split(":")[1]!; + expect(tagPart1).toMatch(/^[A-Za-z0-9_.-]+$/); + expect(tagPart2).toMatch(/^[A-Za-z0-9_.-]+$/);
202-228: Strengthen ECR auth token validation by decoding.Validate it decodes to “AWS:” for stronger guarantees than base64 charset.
- // Verify the token format (should be a base64-encoded string) - expect(auth.password).toMatch(/^[A-Za-z0-9+/=]+$/); + // Verify token decodes to "AWS:<password>" + expect(auth.password).toMatch(/^[A-Za-z0-9+/=]+$/); + const decoded = Buffer.from(auth.password, "base64").toString("utf8"); + expect(decoded).toMatch(/^AWS:.+$/);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/webapp/test/getDeploymentImageRef.test.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Our tests are all vitest
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}: Do not import app/env.server.ts into tests, either directly or indirectly
Tests should only import classes/functions from files under apps/webapp/app/**/*.ts
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests must use Vitest
Tests should avoid mocks or stubs and use helpers from @internal/testcontainers when Redis or Postgres are needed
Test files live beside the files under test and should use descriptive describe and it blocks
Files:
apps/webapp/test/getDeploymentImageRef.test.ts
🧬 Code graph analysis (1)
apps/webapp/test/getDeploymentImageRef.test.ts (1)
apps/webapp/app/v3/getDeploymentImageRef.server.ts (1)
getDeploymentImageRef(101-156)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (5)
apps/webapp/test/getDeploymentImageRef.test.ts (5)
11-11: No action needed on suite restructuring.Top-level suite name change is fine.
28-31: Good gating of teardown to avoid ECR calls when disabled.Early return on KEEP_TEST_REPO and RUN_ECR_TESTS prevents accidental deletes.
60-77: Assertions updated to the new tag format look correct.Host change, envType lowercasing, and deploymentShortCode suffix match server logic.
81-128: E2E ECR flow covered well; idempotency verified.Nice: repoCreated true on first call, false on second, and imageRef includes envType + short code.
1-200: Noenv.server.tsimports found in tests or the module under test. Rule satisfied.
2 participants
fixes #2473