[dev] [Itsnotaka] daniel/monorepo-refactor #1781
Draft
Conversation
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
🔒 Comp AI - Security Review🔴 Risk Level: HIGHNo OSV/CVE findings in dependency scan. Multiple files contain hardcoded credentials and API keys (examples in workflows, README.md, SELF_HOSTING.md, and disabled workflows). 📦 Dependency Vulnerabilities✅ No known vulnerabilities detected in dependencies. 🛡️ Code Security AnalysisView 15 file(s) with issues🟡 .github/actions/bun-install/action.yml (MEDIUM Risk)
Recommendations:
🔴 .github/actions/dangerous-git-checkout/action.yml (HIGH Risk)
Recommendations:
🟡 .github/workflows/auto-pr-to-release.yml (MEDIUM Risk)
Recommendations:
🟡 .github/workflows/database-migrations-main.yml (MEDIUM Risk)
Recommendations:
🔴 .github/workflows/database-migrations-release.yml (HIGH Risk)
Recommendations:
🔴 .github/workflows/trigger-tasks-deploy-main.yml (HIGH Risk)
Recommendations:
🟡 .github/workflows/trigger-tasks-deploy-release.yml (MEDIUM Risk)
Recommendations:
🔴 .github/workflows_disabled/e2e-tests.yml (HIGH Risk)
Recommendations:
🟡 .github/workflows_disabled/quick-tests.yml (MEDIUM Risk)
Recommendations:
🔴 .github/workflows_disabled/test-quick.yml (HIGH Risk)
Recommendations:
🟡 .github/workflows_disabled/unit-tests.yml (MEDIUM Risk)
Recommendations:
🔴 CHANGELOG.md (HIGH Risk)
Recommendations:
🟡 Dockerfile (MEDIUM Risk)
Recommendations:
🔴 README.md (HIGH Risk)
Recommendations:
🟡 SELF_HOSTING.md (MEDIUM Risk)
Recommendations:
💡 RecommendationsView 3 recommendation(s)
Powered by Comp AI - AI that handles compliance for you. Reviewed Nov 20, 2025 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
This branch represents a significant modernization of our monorepo infrastructure, shifting us towards a faster, leaner, and more strictly defined development environment. Here is the updated explanation for your team, highlighting the caching improvements:
We have replaced tsup with tsdown across our packages. The Change: tsdown (powered by rolldown) replaces complex configs and "scattered" artifacts with a standardized output structure.
We have configured Bun to use isolated installs (default in Bun 1.3+) via bunfig.toml. Why: Previously, dependencies were "hoisted" to the root, allowing packages to accidentally access libraries they didn't explicitly declare.
We are now using the workspace:* protocol for our internal packages (e.g., in packages/email depending on packages/ui). Why: This tells the package manager to "use whatever version is currently in the folder."
We upgraded to Prisma 6 and switched the generator provider to prisma-client (replacing prisma-client-js). Why: This enables Prisma's new TypeScript-based engine (Query Compiler), moving away from the heavy Rust binary sidecar. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -97,32 +109,36 @@ | |||
| // Not a URL - treat as S3 key | |||
| // Security: Ensure it's not a malformed URL attempting to bypass validation | |||
| const lowerInput = url.toLowerCase(); | |||
| if (lowerInput.includes('://') || lowerInput.includes('amazonaws.com')) { | |||
| throw new Error('Invalid input: Malformed URL detected'); | |||
| if (lowerInput.includes("://") || lowerInput.includes("amazonaws.com")) { | |||
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 22 hours ago
To address the issue, replace the substring check lowerInput.includes("amazonaws.com") with a proper host/URL parsing check. For non-URL inputs, ensure the value is treated strictly as an S3 key, without attempting to catch URLs by substring.
Specifically:
- Remove the substring check for
"amazonaws.com"from the code. - Rely solely on the existence of the URL object (
parsedUrl). If parsing fails, treat the input as an S3 key as intended. - Alternatively, consider using a stricter regex or try another parsing attempt to reject malformed URLs, but in most typical S3 key usage, just ensure path traversal and empty keys are rejected.
Edit only the block in extractS3KeyFromUrl.
No additional dependencies are needed for this fix.
Suggested changeset
1
apps/app/src/app/s3.ts
| @@ -109,7 +109,7 @@ | ||
| // Not a URL - treat as S3 key | ||
| // Security: Ensure it's not a malformed URL attempting to bypass validation | ||
| const lowerInput = url.toLowerCase(); | ||
| if (lowerInput.includes("://") || lowerInput.includes("amazonaws.com")) { | ||
| if (lowerInput.includes("://")) { | ||
| throw new Error("Invalid input: Malformed URL detected"); | ||
| } | ||
|
|
Copilot is powered by AI and may make mistakes. Always verify output.
| } | ||
| // For backward compatibility, hash without salt | ||
| return createHash('sha256').update(apiKey).digest('hex'); | ||
| return createHash("sha256").update(apiKey).digest("hex"); |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 22 hours ago
The insecure use of sha256 for API key hashing should be replaced with a secure, computationally expensive hash (such as bcrypt). The best fix is to use bcrypt's hashSync (for sync use) or hash (for async use), along with the generated salt, to properly hash API keys. This will significantly slow down brute-force attacks in case of data compromise. The change involves:
- Importing
bcryptjs(orbcrypt).bcryptjsis pure JavaScript and often easier to use in serverless/Next.js environments. - Modifying the
hashApiKeyfunction to usebcryptwhen a salt is provided. For backward compatibility, the legacysha256hash can be retained as before if no salt is provided. - Generating salts with
bcrypt(genSaltSyncorgenSalt) instead of making your own salt. - Ensure that generated API keys (random and long) continue as usual.
- Required changes: Update legacy logic for generating and verifying hashes, import
bcryptjs, update salt generation for new API keys, and usebcryptin the hashing function.
All edits occur in apps/app/src/lib/api-key.ts (and only touched areas per instructions).
Suggested changeset
2
apps/app/src/lib/api-key.ts
| @@ -1,4 +1,5 @@ | ||
| import { createHash, randomBytes } from "node:crypto"; | ||
| import bcrypt from "bcryptjs"; | ||
| import type { NextRequest } from "next/server"; | ||
| import { NextResponse } from "next/server"; | ||
|
|
||
| @@ -20,7 +21,8 @@ | ||
| * @returns A random salt string | ||
| */ | ||
| export function generateSalt(): string { | ||
| return randomBytes(16).toString("hex"); | ||
| // Generate a bcrypt salt with 12 rounds (recommended for modern systems) | ||
| return bcrypt.genSaltSync(12); | ||
| } | ||
|
|
||
| /** | ||
| @@ -31,12 +33,10 @@ | ||
| */ | ||
| export function hashApiKey(apiKey: string, salt?: string): string { | ||
| if (salt) { | ||
| // If salt is provided, use it for hashing | ||
| return createHash("sha256") | ||
| .update(apiKey + salt) | ||
| .digest("hex"); | ||
| // Use bcrypt to securely hash the API key with the provided salt | ||
| return bcrypt.hashSync(apiKey, salt); | ||
| } | ||
| // For backward compatibility, hash without salt | ||
| // For backward compatibility, hash without salt using the old method | ||
| return createHash("sha256").update(apiKey).digest("hex"); | ||
| } | ||
|
|
apps/app/package.json
Outside changed files
| @@ -141,7 +141,8 @@ | ||
| "xml2js": "^0.6.2", | ||
| "zaraz-ts": "^1.2.0", | ||
| "zod": "^4.1.12", | ||
| "zustand": "^5.0.8" | ||
| "zustand": "^5.0.8", | ||
| "bcryptjs": "^3.0.3" | ||
| }, | ||
| "devDependencies": { | ||
| "@playwright/experimental-ct-react": "^1.56.1", |
This fix introduces these dependencies
| Package | Version | Security advisories |
| bcryptjs (npm) | 3.0.3 | None |
Copilot is powered by AI and may make mistakes. Always verify output.
| @@ -1,11 +1,11 @@ | |||
| export const logger = { | |||
| info: (message: string, params?: unknown) => { | |||
| console.log(`[INFO] ${message}`, params || ''); | |||
| console.log(`[INFO] ${message}`, params || ""); | |||
Check failure
Code scanning / CodeQL
Use of externally-controlled format string High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 22 hours ago
To fix this vulnerability in apps/app/src/utils/logger.ts, specifically in the logger's info, warn, and error methods, avoid passing user-controlled data directly within the first console.log/console.warn/console.error argument, which results in it being interpreted as a format string. Instead, always use a format placeholder (%s) for the interpolated/user-controlled portion and pass the interpolated/user-controlled data as a separate argument.
Specifically, change lines such as:
console.log(`[INFO] ${message}`, params || "");to:
console.log("[INFO] %s", message, params || "");Apply the same pattern for warn and error. This ensures any % sequences in message will not be interpreted as format placeholders by console.log and avoids format string confusion. No new imports are needed for this fix.
Suggested changeset
1
apps/app/src/utils/logger.ts
| @@ -1,11 +1,11 @@ | ||
| export const logger = { | ||
| info: (message: string, params?: unknown) => { | ||
| console.log(`[INFO] ${message}`, params || ""); | ||
| console.log("[INFO] %s", message, params || ""); | ||
| }, | ||
| warn: (message: string, params?: unknown) => { | ||
| console.warn(`[WARN] ${message}`, params || ""); | ||
| console.warn("[WARN] %s", message, params || ""); | ||
| }, | ||
| error: (message: string, params?: unknown) => { | ||
| console.error(`[ERROR] ${message}`, params || ""); | ||
| console.error("[ERROR] %s", message, params || ""); | ||
| }, | ||
| }; |
Copilot is powered by AI and may make mistakes. Always verify output.
| @@ -94,21 +107,21 @@ | |||
| // Not a URL - treat as S3 key | |||
| // Security: Ensure it's not a malformed URL attempting to bypass validation | |||
| const lowerInput = url.toLowerCase(); | |||
| if (lowerInput.includes('://') || lowerInput.includes('amazonaws.com')) { | |||
| throw new Error('Invalid input: Malformed URL detected'); | |||
| if (lowerInput.includes("://") || lowerInput.includes("amazonaws.com")) { | |||
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 22 hours ago
To fix the problem, avoid using a substring check for "amazonaws.com" and instead, robustly determine if an input string represents a URL by attempting to parse it. If it can be parsed as a URL (even without a valid S3 host), it should be rejected from the S3 key path. Lines 109-112 should be updated to: attempt to parse the string as a URL, and if the result is a valid URL object (regardless of host), reject the input as "malformed". This way, any string that is a valid URL (even if the host is not "amazonaws.com") cannot masquerade as an S3 key. Use the built-in URL constructor to do this check, which is also used above in the function.
No external dependencies are needed, as the built-in URL API suffices. Only the code block on lines 109-112 in apps/portal/src/utils/s3.ts should be changed.
Suggested changeset
1
apps/portal/src/utils/s3.ts
| @@ -106,9 +106,12 @@ | ||
|
|
||
| // Not a URL - treat as S3 key | ||
| // Security: Ensure it's not a malformed URL attempting to bypass validation | ||
| const lowerInput = url.toLowerCase(); | ||
| if (lowerInput.includes("://") || lowerInput.includes("amazonaws.com")) { | ||
| // If input parses as a URL, treat as malformed (should be S3 key only) | ||
| try { | ||
| new URL(url); | ||
| throw new Error("Invalid input: Malformed URL detected"); | ||
| } catch { | ||
| // Not a URL, continue as S3 key | ||
| } | ||
|
|
||
| // Security: Check for path traversal |
Copilot is powered by AI and may make mistakes. Always verify output.
| @@ -110,7 +117,7 @@ | |||
| <BreadcrumbPage>{item.label}</BreadcrumbPage> | |||
| ) : ( | |||
| <BreadcrumbLink asChild> | |||
| <Link href={item.href || '#'}>{item.label}</Link> | |||
| <Link href={item.href || "#"}>{item.label}</Link> | |||
Check warning
Code scanning / CodeQL
Client-side URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 22 hours ago
The best way to fix this problem is to ensure that the href values in breadcrumbs are safe: only allowing navigation to internal routes (relative URLs) and never to external sites. This can be achieved by validating the href value before passing it to the <Link> component. If the href does not start with a safe value ("/") or matches an accepted pattern, it should be replaced with a safe default ("#" or another internal route). For all breadcrumb links, before rendering each <Link>, check that item.href is either undefined, empty, or a safe relative path. You can create a helper function (e.g., isSafeInternalLink(href)) to encapsulate the logic and use it consistently for breadcrumb items, dropdown items, and hidden items.
This change should be made in apps/app/src/components/pages/PageWithBreadcrumb.tsx, in all places where a breadcrumb href is rendered (item.href and similar), by wrapping the values with the safety check helper.
Suggested changeset
1
apps/app/src/components/pages/PageWithBreadcrumb.tsx
| @@ -20,6 +20,15 @@ | ||
|
|
||
| import PageCore from "./PageCore.tsx"; | ||
|
|
||
|
|
||
| function isSafeInternalLink(href?: string): string { | ||
| if (!href || typeof href !== "string") return "#"; | ||
| // Accept only relative paths starting with "/" and not containing "//" | ||
| if (/^\/(?!\/)/.test(href)) return href; | ||
| // Optionally, further restrict to e.g. `/[orgId]/...` if you wish | ||
| return "#"; | ||
| } | ||
|
|
||
| interface BreadcrumbDropdownItem { | ||
| label: string; | ||
| href: string; | ||
| @@ -104,7 +113,7 @@ | ||
| > | ||
| {item.dropdown.map((dropdownItem) => ( | ||
| <DropdownMenuItem key={dropdownItem.href} asChild> | ||
| <Link href={dropdownItem.href}> | ||
| <Link href={isSafeInternalLink(dropdownItem.href)}> | ||
| {dropdownItem.label.length > maxLabelLength | ||
| ? `${dropdownItem.label.slice(0, maxLabelLength)}...` | ||
| : dropdownItem.label} | ||
| @@ -117,7 +126,7 @@ | ||
| <BreadcrumbPage>{item.label}</BreadcrumbPage> | ||
| ) : ( | ||
| <BreadcrumbLink asChild> | ||
| <Link href={item.href || "#"}>{item.label}</Link> | ||
| <Link href={isSafeInternalLink(item.href)}>{item.label}</Link> | ||
| </BreadcrumbLink> | ||
| )} | ||
| </BreadcrumbItem> | ||
| @@ -132,7 +141,7 @@ | ||
| <DropdownMenuContent align="start"> | ||
| {hiddenItems.map((hiddenItem) => ( | ||
| <DropdownMenuItem key={hiddenItem.label} asChild> | ||
| <Link href={hiddenItem.href || "#"}> | ||
| <Link href={isSafeInternalLink(hiddenItem.href)}> | ||
| {hiddenItem.label} | ||
| </Link> | ||
| </DropdownMenuItem> |
Copilot is powered by AI and may make mistakes. Always verify output.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is an automated pull request to merge daniel/monorepo-refactor into dev.
It was created by the [Auto Pull Request] action.