๐ก๏ธ Sentinel: [HIGH] Gherkin ๋ด๋ณด๋ด๊ธฐ ๊ฒฝ๋ก ํ์ ์ทจ์ฝ์ ์์ #43
๐ก๏ธ Sentinel: [HIGH] Gherkin ๋ด๋ณด๋ด๊ธฐ ๊ฒฝ๋ก ํ์ ์ทจ์ฝ์ ์์ #43seonghobae wants to merge 1 commit into
Conversation
exportGherkin ๋ช ๋ น์์ ์ ์์ ์ธ ๊ฒฝ๋ก(์: ../../tmp/pwned.txt)๋ฅผ ํตํด ํ๋ก์ ํธ ๋ฃจํธ ๋๋ ํ ๋ฆฌ ์ธ๋ถ์ ํ์ผ ์์คํ ์ ์ ๊ทผํ ์ ์๋ ๊ฒฝ๋ก ํ์(Path Traversal) ์ทจ์ฝ์ ์ ์์ ํ์ต๋๋ค. path.resolve์ path.relative๋ฅผ ์ฌ์ฉํ์ฌ ์ถ๋ ฅ ๊ฒฝ๋ก๊ฐ ํญ์ ํ๋ก์ ํธ ๋ฃจํธ ๋ด๋ถ์ ์์นํ๋๋ก ์ ํํฉ๋๋ค.
|
๐ Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a ๐ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Linter diff in the way? Review this PR in Change Stack to focus on meaningful changes and expand context only when needed. ๐ WalkthroughSummary by CodeRabbit๋ฆด๋ฆฌ์ค ๋ ธํธ
์์ฝGherkin ๋ด๋ณด๋ด๊ธฐ์ ๋ณ๊ฒฝ ์ฌํญ๊ฒฝ๋ก ํธ๋๋ฒ์ค ์ทจ์ฝ์ ์์
์์ ์ฝ๋ ๋ฆฌ๋ทฐ ๋์ด๋๐ฏ 3 (์ ์ ) | โฑ๏ธ ~20๋ถ ์
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
โ Failed checks (1 error)
โ Passed checks (4 passed)
โจ Finishing Touches๐ Generate docstrings
๐งช Generate unit tests (beta)
โจ Simplify code
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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: 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 `@src/export/gherkin.ts`:
- Around line 43-51: The current path-check uses resolve/relative string checks
which can be bypassed via symlinks; update the export flow to compare real
paths: compute realRoot = realpathSync(config.root) and realTarget =
realpathSync(dirname(outputPath)) (or realpathSync(outputPath) if it exists) and
ensure realTarget.startsWith(realRoot) before calling mkdirSync or
writeFileSync; additionally, use lstatSync on each path component as needed to
detect unexpected symlinks and fail if a symlink would escape the root
(references: outputPath, rel, config.root, mkdirSync, writeFileSync).
In `@tests/gherkin.test.ts`:
- Around line 30-37: Update the test so it asserts the thrown error is a
VspecError and that error.code === "INVALID_ARGUMENT" in addition to the
message: import or reference the VspecError symbol, replace the single
expect(...).toThrowError(...) with a try/catch around exportGherkin({ ...
output: "...." }) that fails if no error is thrown, then in the catch assert
expect(err).toBeInstanceOf(VspecError),
expect(err.code).toBe("INVALID_ARGUMENT"), and still check the message (e.g.,
expect(err.message).toMatch(/Export path escapes/)) to fully validate the error
contract thrown by exportGherkin.
๐ช 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: f1d9d1e1-ae32-447b-8459-7e95df4239ff
๐ Files selected for processing (3)
.jules/sentinel.mdsrc/export/gherkin.tstests/gherkin.test.ts
| const outputPath = resolve(config.root, output); | ||
|
|
||
| const rel = relative(config.root, outputPath); | ||
| if (rel.startsWith("..") || isAbsolute(rel)) { | ||
| throw new VspecError("INVALID_ARGUMENT", "Export path escapes the project root."); | ||
| } | ||
|
|
||
| mkdirSync(dirname(outputPath), { recursive: true }); | ||
| writeFileSync(outputPath, text); |
There was a problem hiding this comment.
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
# Verify current implementation is lexical-only (no canonical/symlink guard).
rg -n 'exportGherkin|resolve\(|relative\(|realpathSync|lstatSync|writeFileSync' src/export/gherkin.tsRepository: Seongho-Bae/vooster-v2-mvp
Length of output: 371
๐ Script executed:
#!/bin/bash
sed -n '1,120p' src/export/gherkin.tsRepository: Seongho-Bae/vooster-v2-mvp
Length of output: 2385
์ฌ๋ณผ๋ฆญ ๋งํฌ๋ก ํ๋ก์ ํธ ๋ฃจํธ ๊ฒฝ๊ณ๋ฅผ ์ฐํํด ์ฐ๊ธฐ๊ฐ ๊ฐ๋ฅํจ (src/export/gherkin.ts:43-51)
ํ์ฌ ๊ฒ์ฌ๋ resolve/relative๋ก โ๋ฌธ์์ด ๊ฒฝ๊ณโ๋ง ํ์ธํ๊ณ , config.root/๋ถ๋ชจ/๋์ ๊ฒฝ๋ก์ realpath ๋ฐ symlink ์ฌ๋ถ๋ฅผ ์ ํ ๊ฒ์ฆํ์ง ์์ต๋๋ค. ๋ฃจํธ ๋ด๋ถ์ symlink์ด ํฌํจ๋๋ฉด mkdirSync(...)/writeFileSync(...)๊ฐ ์ค์ ๋ก ๋ฃจํธ ๋ฐ์ ๊ฐ๋ฆฌํค๋ ๊ฒฝ๋ก์ ํ์ผ์ ๊ธฐ๋กํ ์ ์์ต๋๋ค.
๐ ์ ์ ์์ ์
-import { mkdirSync, readFileSync, writeFileSync } from "node:fs";
+import { existsSync, lstatSync, mkdirSync, readFileSync, realpathSync, writeFileSync } from "node:fs";
import { dirname, join, relative, resolve, isAbsolute } from "node:path";
@@
- const outputPath = resolve(config.root, output);
+ const rootReal = realpathSync(config.root);
+ const outputPath = resolve(rootReal, output);
- const rel = relative(config.root, outputPath);
+ const rel = relative(rootReal, outputPath);
if (rel.startsWith("..") || isAbsolute(rel)) {
throw new VspecError("INVALID_ARGUMENT", "Export path escapes the project root.");
}
mkdirSync(dirname(outputPath), { recursive: true });
+ const parentReal = realpathSync(dirname(outputPath));
+ const parentRel = relative(rootReal, parentReal);
+ if (parentRel.startsWith("..") || isAbsolute(parentRel)) {
+ throw new VspecError("INVALID_ARGUMENT", "Export path escapes the project root.");
+ }
+ if (existsSync(outputPath) && lstatSync(outputPath).isSymbolicLink()) {
+ throw new VspecError("INVALID_ARGUMENT", "Export path escapes the project root.");
+ }
writeFileSync(outputPath, text);๐ค 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 `@src/export/gherkin.ts` around lines 43 - 51, The current path-check uses
resolve/relative string checks which can be bypassed via symlinks; update the
export flow to compare real paths: compute realRoot = realpathSync(config.root)
and realTarget = realpathSync(dirname(outputPath)) (or realpathSync(outputPath)
if it exists) and ensure realTarget.startsWith(realRoot) before calling
mkdirSync or writeFileSync; additionally, use lstatSync on each path component
as needed to detect unexpected symlinks and fail if a symlink would escape the
root (references: outputPath, rel, config.root, mkdirSync, writeFileSync).
| it("rejects path traversal attempts that escape the project root", () => { | ||
| initProject({ root: tmpRoot, key: "TEST" }); | ||
| createUseCase({ cwd: tmpRoot, title: "Test Path Traversal", primaryActor: "system" }); | ||
|
|
||
| expect(() => { | ||
| exportGherkin({ cwd: tmpRoot, key: "TEST-001", output: "../../../../../../../../../../../../../../../../tmp/pwned.txt" }); | ||
| }).toThrowError("Export path escapes the project root."); | ||
| }); |
There was a problem hiding this comment.
๐งฉ Analysis chain
๐ Script executed:
#!/bin/bash
# Verify whether tests assert error code/type contract in addition to message.
rg -n 'Export path escapes the project root|INVALID_ARGUMENT|VspecError|toThrowError' tests/gherkin.test.ts src/export/gherkin.tsRepository: Seongho-Bae/vooster-v2-mvp
Length of output: 338
์๋ฌ ๋ฉ์์ง๋ฟ ์๋๋ผ VspecError ์ฝ๋ ๊ณ์ฝ๋ ๋จ์ธํ๋๋ก ํ ์คํธ๋ฅผ ๋ณด๊ฐํ์ธ์.
src/export/gherkin.ts๋ ๊ฒฝ๋ก ํ์ถ ์new VspecError("INVALID_ARGUMENT", ...)๋ฅผ ๋์ง๋๋ฐ,tests/gherkin.test.ts๋ ํ์ฌ ๋ฉ์์ง๋งtoThrowError๋ก ๊ฒ์ฆํ๊ณ ์์ต๋๋ค.error๊ฐVspecError์ธ์ง์error.code === "INVALID_ARGUMENT"๊น์ง ํจ๊ป ํ์ธํ๋๋ก ๋ณ๊ฒฝํด ํ๊ท๋ฅผ ๋ง์์ฃผ์ธ์.
๐ค 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 `@tests/gherkin.test.ts` around lines 30 - 37, Update the test so it asserts
the thrown error is a VspecError and that error.code === "INVALID_ARGUMENT" in
addition to the message: import or reference the VspecError symbol, replace the
single expect(...).toThrowError(...) with a try/catch around exportGherkin({ ...
output: "...." }) that fails if no error is thrown, then in the catch assert
expect(err).toBeInstanceOf(VspecError),
expect(err.code).toBe("INVALID_ARGUMENT"), and still check the message (e.g.,
expect(err.message).toMatch(/Export path escapes/)) to fully validate the error
contract thrown by exportGherkin.
1 participant
๐จ Severity: HIGH
๐ก Vulnerability:
vspec export gherkin์คํ ์--output์ต์ ์../../๊ฐ์ ๊ฒฝ๋ก ํ์ ๋ฌธ์์ด์ ์ ๋ ฅํ๋ฉด ํ๋ก์ ํธ ๋ฃจํธ ์ธ๋ถ ์์์ ์์น์ ํ์ผ์ ์ธ ์ ์๋ ์ทจ์ฝ์ ์ด ๋ฐ๊ฒฌ๋์์ต๋๋ค.๐ฏ Impact: ์ ์์ ์ธ ํ์ผ ๋ฎ์ด์ฐ๊ธฐ๋ ํ๋ก์ ํธ ์ธ๋ถ ์์ญ์ ํ์ผ ์์ฑ์ผ๋ก ์ธํด ์์คํ ์์์ด ๋ฐ์ํ ์ ์์ต๋๋ค.
๐ง Fix:
path.resolve๋ก ์ ๋ ๊ฒฝ๋ก๋ฅผ ๋ง๋ค๊ณpath.relative๋ฅผ ์ฌ์ฉํด ๋์ ๊ฒฝ๋ก๊ฐconfig.root๋ด๋ถ์ ์์นํ๋์ง ์๊ฒฉํ ๊ฒ์ฌํ๋๋กsrc/export/gherkin.ts๋ฅผ ์์ ํ์ต๋๋ค. ๊ฒ์ฌ์ ์คํจํ๋ฉดVspecError("INVALID_ARGUMENT")๋ฅผ ๋ฐ์์ํต๋๋ค.โ Verification:
pnpm typecheck๋ฐpnpm test๊ฐ ๋ชจ๋ ์ฑ๊ณตํ๋ฉฐ,tests/gherkin.test.ts์ ์ด๋ฅผ ๋ฐฉ์ดํ๋ ์ ์ฉ ํ ์คํธ ์ผ์ด์ค๋ฅผ ์ถ๊ฐํ์ต๋๋ค.PR created automatically by Jules for task 16705097182536217132 started by @seonghobae