fix(deps): resolve undici and marked runtime crashes - #41
Open
sureshchouksey8 wants to merge 6 commits into
Open
fix(deps): resolve undici and marked runtime crashes#41sureshchouksey8 wants to merge 6 commits into
sureshchouksey8 wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds Git-based file uploads with a new service and controller endpoint, persists OAuth usernames for GitHub and CNB sign-ins, and updates the Docker image plus two dependency versions. ChangesGit file upload feature
OAuth userName persistence
Build and dependency updates
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 4
🧹 Nitpick comments (1)
test/OAuth.test.ts (1)
21-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a CNB counterpart for this new
userNamecontract.This locks down the GitHub path, but the PR also changed the CNB branch to persist
usernameintoOAuthCredential.userName. A small sibling test would keep the second provider-specific parser from regressing unnoticed.🤖 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 `@test/OAuth.test.ts` around lines 21 - 61, Add a CNB-specific test alongside the existing OauthController GitHub test to cover the new OAuthCredential.userName contract. Mirror the setup used in signInWithGithub, but exercise the CNB sign-in path in OauthController and assert that credentialStore.save receives userName populated from the CNB profile username field, so the provider-specific mapping stays covered.
🤖 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 `@source/model/OAuth.ts`:
- Around line 31-33: The OAuth entity currently allows userName to be persisted
as an empty string via the `@Column` default, which creates credential records
that fail later in GitFileService. Update the OAuth model’s userName contract to
require a real value instead of defaulting to empty, and add a
migration/backfill or re-authentication path for any existing rows that already
have blank values. Keep the fix centered on the OAuth class and its userName
field so authenticated upload flows only see valid credentials.
In `@source/service/GitFile.ts`:
- Around line 150-158: Reject duplicate destination paths before copying in
copyIncomingFile and the upload flow that counts successful parts. Normalize
each resolved target path from resolveRepositoryPath and track seen targets so
repeated destinations like docs/a.md and docs/../docs/a.md fail fast instead of
allowing later copies to overwrite earlier ones. Update the logic around
copyIncomingFile and the file-counting loop to surface an error when a duplicate
repository target is detected.
- Around line 59-71: The GitFile.runCommand helper currently calls execFileAsync
without a timeout and lets raw subprocess errors escape. Update runCommand to
enforce a sensible timeout for git operations and wrap execFileAsync failures
into the app’s git-specific error type so callers on the request path get
translated failures instead of generic exceptions. Use the existing runCommand
symbol as the main fix point and preserve the current stdout handling while
adding error mapping for expected git subprocess failures.
In `@test/FileController.test.ts`:
- Around line 25-28: The test setup in FileController.test.ts uses real absolute
Windows-style paths for the files array, which can cause uploadGitFiles()
cleanup to delete unintended files in finally. Update the files fixtures in this
test to use temp paths created by the test itself, following the mkdtemp()
pattern already used in the failure test, so FileController.uploadGitFiles only
removes files owned by the test.
---
Nitpick comments:
In `@test/OAuth.test.ts`:
- Around line 21-61: Add a CNB-specific test alongside the existing
OauthController GitHub test to cover the new OAuthCredential.userName contract.
Mirror the setup used in signInWithGithub, but exercise the CNB sign-in path in
OauthController and assert that credentialStore.save receives userName populated
from the CNB profile username field, so the provider-specific mapping stays
covered.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66ff8575-2a31-4158-8430-2adb153db796
⛔ Files ignored due to path filters (2)
jest.logis excluded by!**/*.logpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
Dockerfilepackage.jsonsource/controller/File.tssource/controller/OAuth.tssource/model/File.tssource/model/OAuth.tssource/service/GitFile.tssource/service/index.tstest/FileController.test.tstest/GitFile.test.tstest/OAuth.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@source/service/GitFile.ts`:
- Around line 174-185: The current target validation in
assertUniqueRepositoryTargets only checks for duplicate lexical paths via
resolveRepositoryPath, but it does not prevent writes through symlinked parents
inside the checked-out repo. Update the GitFile flow to verify each
destination’s existing parent chain with lstat() or canonicalize the resolved
path before mkdir()/copyFile() so any symlinked repository target is rejected
before copying. Keep the change anchored around assertUniqueRepositoryTargets
and resolveRepositoryPath, and ensure the same protection is applied wherever
the final destination path is used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e2aa7c0-ee01-4271-8e64-4ecc9ed73c28
📒 Files selected for processing (5)
source/model/OAuth.tssource/service/GitFile.tstest/FileController.test.tstest/GitFile.test.tstest/OAuth.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- source/model/OAuth.ts
- test/FileController.test.ts
- test/GitFile.test.ts
Comment on lines
+174
to
+185
| protected assertUniqueRepositoryTargets(repositoryFolder: string, files: IncomingGitFile[]) { | ||
| const seenTargets = new Set<string>(); | ||
|
|
||
| for (const { fieldname } of files) { | ||
| const targetPath = this.resolveRepositoryPath(repositoryFolder, fieldname); | ||
|
|
||
| if (seenTargets.has(targetPath)) | ||
| throw new BadRequestError(`Duplicate repository path: ${fieldname}`); | ||
|
|
||
| seenTargets.add(targetPath); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether the current implementation has symlink-aware target checks.
ast-grep outline source/service/GitFile.ts --view expanded
rg -n "assertUniqueRepositoryTargets|copyIncomingFile|copyFile\\(|mkdir\\(|lstat\\(|realpath\\(" source/service/GitFile.tsRepository: FreeCodeCamp-Chengdu/HOP-service
Length of output: 1974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '140,230p' source/service/GitFile.ts | cat -n
printf '\n---\n'
rg -n "resolveRepositoryPath|download|clone|checkout|repositoryFolder" source/service/GitFile.tsRepository: FreeCodeCamp-Chengdu/HOP-service
Length of output: 5070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether there are any existing tests or helpers around symlink/path traversal for this service.
rg -n "GitFileService|assertUniqueRepositoryTargets|resolveRepositoryPath|copyIncomingFile|symlink|lstat|realpath|path traversal" .Repository: FreeCodeCamp-Chengdu/HOP-service
Length of output: 2192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '140,173p' source/service/GitFile.ts | cat -nRepository: FreeCodeCamp-Chengdu/HOP-service
Length of output: 1653
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '146,173p' source/service/GitFile.ts | cat -nRepository: FreeCodeCamp-Chengdu/HOP-service
Length of output: 1487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the repository download step is a plain git checkout/clone or something that strips symlinks.
rg -n "download.*repository|case .*download|gitUtilityCLI|symlink|checkout|clone" source test .Repository: FreeCodeCamp-Chengdu/HOP-service
Length of output: 1506
Reject symlinked repository targets before copying. resolveRepositoryPath() only enforces lexical containment; if the checked-out repo already contains a symlinked parent, mkdir()/copyFile() can write outside repositoryFolder. Check each existing parent with lstat() or canonicalize the destination first.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile, ExecFileOptions } from 'child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@source/service/GitFile.ts` around lines 174 - 185, The current target
validation in assertUniqueRepositoryTargets only checks for duplicate lexical
paths via resolveRepositoryPath, but it does not prevent writes through
symlinked parents inside the checked-out repo. Update the GitFile flow to verify
each destination’s existing parent chain with lstat() or canonicalize the
resolved path before mkdir()/copyFile() so any symlinked repository target is
rejected before copying. Keep the change anchored around
assertUniqueRepositoryTargets and resolveRepositoryPath, and ensure the same
protection is applied wherever the final destination path is used.
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
2 participants
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.
Downgrades undici to ^8.0.2 to fix Node 22 WebIDL crash, and downgrades marked to ^14.1.4 to fix ERR_REQUIRE_ESM since the project compiles to CommonJS.
Summary by CodeRabbit