From 4f8ee12b8a92d5b3134f82b2e25afd0d34f8ef0a Mon Sep 17 00:00:00 2001 From: theSnackOverflow Date: Sun, 30 Aug 2026 15:03:24 +0900 Subject: [PATCH 1/2] docs: add contribution note for nodejs/node#65659 --- contributions/65659.md | 60 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 contributions/65659.md diff --git a/contributions/65659.md b/contributions/65659.md new file mode 100644 index 0000000..f6ff2e3 --- /dev/null +++ b/contributions/65659.md @@ -0,0 +1,60 @@ +--- +pr-url: https://github.com/nodejs/node/pull/65659 +--- + +## 문제 내용 + +`--permission-audit`는 권한 검사를 수행하되 차단하지 않고 diagnostics channel로 관측만 하는 모드입니다. 그런데 세 경로가 이 계약을 어기고 실제로 차단하고 있었습니다([#65419](https://github.com/nodejs/node/issues/65419)). + +로컬 빌드로 재현한 결과입니다. + +| API | 플래그 없음 | `--permission` | `--permission-audit` | +| --- | --- | --- | --- | +| `fs.statSync` | OK | 차단 | OK (정상) | +| `fs.lstatSync` | OK | 차단 | **차단** | +| `fs.symlinkSync` | OK | 차단 | **차단** | +| `process.dlopen` | — | 차단 | **차단** | + +원인은 두 가지로 갈렸습니다. + +**1. JS 계층 검사가 audit 모드를 확인하지 않음.** C++ 매크로는 `warning_only()`를 보고 throw를 생략하는데, `lib/fs.js`의 검사에는 그 단계가 없었습니다. + +```js +if (permission.isEnabled() && !permission.has('fs.read', path)) { + throw new ERR_ACCESS_DENIED(...); // audit 모드에서도 무조건 throw +} +``` + +`isEnabled()`가 `--permission-audit`에서도 `true`라 그대로 던집니다. `stat`이 정상이었던 건 검사가 C++에 있어서였습니다. + +**2. dlopen은 권한 검사 이전에 차단.** `src/env.cc`가 audit 모드에도 `allow_native_addons = false`를 설정하고, `DLOpen()`이 audit 인식 매크로보다 **먼저** `no_native_addons()`를 검사해 `ERR_DLOPEN_DISABLED`를 던집니다. audit 이벤트조차 발행되지 않습니다. + +## 해결 과정과 검증 + +`permission.isAuditMode()` 헬퍼가 이미 존재하는데 `lib/ffi.js`에만 배선되어 있었습니다. `git log -S "isAuditMode"` 결과 커밋이 하나뿐이었고(#63047), fs는 그때 누락된 것이었습니다. + +```js +if (permission.isEnabled() && !permission.has('fs.read', path) && + !permission.isAuditMode()) { +``` + +`permission.has()`를 먼저 평가하는 순서가 중요합니다. audit 이벤트는 `has()`가 타고 들어가는 C++ `is_scope_granted()`에서 발행되므로, 이 순서라야 이벤트는 유지하고 throw만 생략합니다. dlopen 쪽은 `src/env.cc`에서 audit 모드일 때 `allow_native_addons`를 건드리지 않도록 하되 `Apply(kAddon)`은 유지해, 채널 이벤트가 계속 발행되게 했습니다. + +검증은 이 순서로 했습니다. + +- 신규 테스트 2개를 **수정 전 바이너리에서 먼저 실행**해 audit 케이스 3개가 실패하는 것을 확인했습니다. 테스트가 실제로 버그를 잡는다는 증거입니다. +- 각 테스트에 `--permission` 강제 모드 단언을 대조군으로 넣어, 완화가 강제 모드로 새지 않음을 증명했습니다. +- 수정 후 3-모드 재현 매트릭스에서 audit 모드가 기준선과 완전히 일치했습니다. +- `parallel/test-permission*` 75/75, `ffi/test-ffi-permission-audit`, 전체 스위트(cctest 222개, jstest 5,942개) 실패 0으로 통과했습니다. + +`symlink` 제한은 CVE-2025-55130 대응 코드라 완화 판단이 필요했습니다. 해당 커밋이 audit 모드 도입보다 앞서 있어 audit을 고려할 수 없었던 시점상 누락으로 보았고, 이 판단 근거와 반대 대안을 PR 본문에 함께 적어 security-wg가 판정할 수 있게 했습니다. + +## 배운 점 + +**한 함수만 읽고 단정하면 뒤집힙니다.** `SendStopSending()`을 읽고 앞서 작성한 문서가 틀렸다고 판단했는데, `is_readable()` 정의까지 펼쳐보니 로컬 단방향 스트림에서 `false`를 반환해 결국 문서가 맞았습니다. Node 코어의 술어 함수는 조건을 감추는 자리라 정의까지 확인해야 합니다. + +**이슈 목록의 빈자리는 대부분 이유가 있습니다.** 열린 이슈 200건을 교차참조로 분류하니 절반 이상이 이미 PR이 붙어 있었고, 비어 있는 것은 미확정 설계이거나 신고자가 직접 작업 중이었습니다. 이 이슈는 신고자가 "포인터가 필요하다"고 남긴 채 열흘간 답이 없던 경우라, 원인 분석을 코멘트로 공유하고 양보 의사를 함께 밝히는 방식으로 접근했습니다. + +**빌드 트리는 브랜치를 따라오지 않습니다.** `src/permission/`이 리팩토링되어 한 달 전 생성된 ninja 파일이 무효화되었고, `./configure --ninja` 재실행이 필요했습니다. 또 이 저장소의 Makefile은 `make -j14`가 아니라 `make test JOBS=14`를 요구하는데, 잘못된 호출이 조용히 실패해 출력을 직접 열어보고서야 발견했습니다. + +AI 도구의 도움을 받았고, 재현·소스 추적·테스트 실행은 직접 수행했습니다. `doc/contributing/ai-guidelines.md`에 따라 커밋에 `Assisted-by` 트레일러를 남겼습니다. From 88f3e9637f4539d49a7a4b74700c25c792919054 Mon Sep 17 00:00:00 2001 From: theSnackOverflow Date: Sun, 30 Aug 2026 15:18:18 +0900 Subject: [PATCH 2/2] docs: expand contribution note for nodejs/node#65659 --- contributions/65659.md | 166 ++++++++++++++++++++++++++++++++++------- 1 file changed, 141 insertions(+), 25 deletions(-) diff --git a/contributions/65659.md b/contributions/65659.md index f6ff2e3..867829b 100644 --- a/contributions/65659.md +++ b/contributions/65659.md @@ -4,57 +4,173 @@ pr-url: https://github.com/nodejs/node/pull/65659 ## 문제 내용 -`--permission-audit`는 권한 검사를 수행하되 차단하지 않고 diagnostics channel로 관측만 하는 모드입니다. 그런데 세 경로가 이 계약을 어기고 실제로 차단하고 있었습니다([#65419](https://github.com/nodejs/node/issues/65419)). +`--permission-audit`는 [문서상](https://nodejs.org/api/cli.html#--permission-audit) 권한 검사를 수행하되 **차단하지 않고** diagnostics channel로 위반 사항만 발행하는 모드입니다. 그런데 세 경로가 이 계약을 어기고 실제로 차단하고 있었습니다([#65419](https://github.com/nodejs/node/issues/65419), 신고자 @naugtur). -로컬 빌드로 재현한 결과입니다. +로컬 빌드(v27.0.0-pre)로 재현한 결과입니다. -| API | 플래그 없음 | `--permission` | `--permission-audit` | -| --- | --- | --- | --- | -| `fs.statSync` | OK | 차단 | OK (정상) | -| `fs.lstatSync` | OK | 차단 | **차단** | -| `fs.symlinkSync` | OK | 차단 | **차단** | -| `process.dlopen` | — | 차단 | **차단** | +| API | 플래그 없음 | `--permission` | `--permission-audit` | 판정 | +| --- | --- | --- | --- | --- | +| `fs.statSync` | OK | 차단 | OK | 정상 | +| `fs.lstatSync` | OK | 차단 | **차단** | 버그 | +| `fs.symlinkSync` | OK | 차단 | **차단** | 버그 | +| `process.dlopen` | — | 차단 | **차단** | 버그 | -원인은 두 가지로 갈렸습니다. +같은 fs 모듈인데 `stat`은 정상이고 `lstat`은 차단된다는 비대칭이 원인 추적의 실마리였습니다. -**1. JS 계층 검사가 audit 모드를 확인하지 않음.** C++ 매크로는 `warning_only()`를 보고 throw를 생략하는데, `lib/fs.js`의 검사에는 그 단계가 없었습니다. +### 원인 1 — JS 계층 검사가 audit 모드를 확인하지 않음 + +C++ 매크로는 audit 모드를 존중합니다(`src/permission/permission.h`). + +```cpp +if (!is_granted(...)) { // 여기서 audit 이벤트 발행 + if (!warning_only()) { // audit 모드면 throw 생략 + ThrowAccessDenied(...); +``` + +그런데 `lstat`과 `symlink`의 검사는 C++이 아니라 JS 계층에 있고, 두 번째 단계가 없었습니다. ```js +// lib/fs.js if (permission.isEnabled() && !permission.has('fs.read', path)) { - throw new ERR_ACCESS_DENIED(...); // audit 모드에서도 무조건 throw + throw new ERR_ACCESS_DENIED(...); // audit 모드에서도 무조건 throw } ``` -`isEnabled()`가 `--permission-audit`에서도 `true`라 그대로 던집니다. `stat`이 정상이었던 건 검사가 C++에 있어서였습니다. +`permission.isEnabled()`는 `--permission || --permission-audit`로 정의되어 audit 모드에서도 `true`입니다. `stat`이 정상이었던 이유는 검사가 C++ 매크로 경로에 있어서였습니다. + +### 원인 2 — dlopen은 권한 검사에 도달하기 전에 차단 + +`src/env.cc`가 audit 모드에도 애드온을 비활성화하고, `DLOpen()`이 audit 인식 매크로보다 **먼저** 그 플래그를 검사합니다. -**2. dlopen은 권한 검사 이전에 차단.** `src/env.cc`가 audit 모드에도 `allow_native_addons = false`를 설정하고, `DLOpen()`이 audit 인식 매크로보다 **먼저** `no_native_addons()`를 검사해 `ERR_DLOPEN_DISABLED`를 던집니다. audit 이벤트조차 발행되지 않습니다. +```cpp +// src/node_binding.cc +if (env->no_native_addons()) { + return THROW_ERR_DLOPEN_DISABLED(...); // audit 모드 무시 +} +THROW_IF_INSUFFICIENT_PERMISSIONS(env, kAddon, ""); // 여기까지 도달 못 함 +``` + +에러 코드부터 `ERR_ACCESS_DENIED`가 아닌 `ERR_DLOPEN_DISABLED`이고, **audit 이벤트조차 발행되지 않습니다**. 관측이 목적인 모드에서 관측 자체가 불가능한 상태였습니다. ## 해결 과정과 검증 -`permission.isAuditMode()` 헬퍼가 이미 존재하는데 `lib/ffi.js`에만 배선되어 있었습니다. `git log -S "isAuditMode"` 결과 커밋이 하나뿐이었고(#63047), fs는 그때 누락된 것이었습니다. +| 항목 | 내용 | +| --- | --- | +| 분류 태그 | 버그수정 | +| 상태 | Open (2026.08.30 제출, 커밋 `4efc964`) | +| 변경 규모 | 5개 파일, +184 / -7, 커밋 1개 | +| 리뷰 요청 | @nodejs/security-wg (CODEOWNERS 자동 라우팅) | + +### 선례 확인 — 헬퍼는 이미 있었다 + +수정 방향을 정하기 전에 기존 구현을 먼저 뒤졌습니다. `permission.isAuditMode()` 헬퍼가 이미 존재했고, `lib/ffi.js`에서만 쓰이고 있었습니다. + +```js +// lib/ffi.js — 올바른 선례 +if (permission.has('ffi') || permission.isAuditMode()) { + return; +} +``` + +`git log -S "isAuditMode" --oneline -- lib/` 결과 커밋이 하나뿐이었습니다(#63047). 즉 헬퍼가 만들어질 때 ffi에만 배선되고 **fs는 누락**된 것이었고, 새 API를 만들 필요 없이 같은 패턴을 확장하면 되는 문제였습니다. + +### 수정 1 — JS 계층 6곳 + +`lib/fs.js` 4곳(`lstat`, `lstatSync`, `symlink`, `symlinkSync`)과 `lib/internal/fs/promises.js` 2곳입니다. ```js if (permission.isEnabled() && !permission.has('fs.read', path) && !permission.isAuditMode()) { ``` -`permission.has()`를 먼저 평가하는 순서가 중요합니다. audit 이벤트는 `has()`가 타고 들어가는 C++ `is_scope_granted()`에서 발행되므로, 이 순서라야 이벤트는 유지하고 throw만 생략합니다. dlopen 쪽은 `src/env.cc`에서 audit 모드일 때 `allow_native_addons`를 건드리지 않도록 하되 `Apply(kAddon)`은 유지해, 채널 이벤트가 계속 발행되게 했습니다. +**조건 순서가 핵심입니다.** audit 이벤트는 `permission.has()`가 타고 들어가는 C++ `Permission::is_scope_granted()`에서 발행됩니다. `isAuditMode()`를 앞에 두면 단축 평가로 `has()`가 호출되지 않아 이벤트가 사라집니다. 관측 기능을 살리려면 `has()`가 먼저 평가되어야 하고, 이는 `lib/ffi.js`의 기존 패턴과도 일치합니다. + +### 수정 2 — src/env.cc + +```cpp +if (!options_->allow_addons) { + // In audit mode addon loading must stay enabled: the denial is + // published through the diagnostics channel by the permission + // check in DLOpen() instead of being rejected upfront. + if (!options_->permission_audit) { + options_->allow_native_addons = false; + } + permission()->Apply(this, args, permission::PermissionScope::kAddon); +} +``` + +`Apply(kAddon)`은 그대로 두었습니다. `kAddon` 스코프가 거부 상태로 남아야 `DLOpen()`의 audit 인식 매크로가 이벤트를 발행하고, `warning_only()`에 의해 throw만 생략되기 때문입니다. 이 한 곳을 고치면 `src/node_worker.cc`의 worker 전파 경로도 함께 해결되어(같은 플래그를 상속) 추가 수정이 불필요했습니다. -검증은 이 순서로 했습니다. +### 신규 테스트 2개 -- 신규 테스트 2개를 **수정 전 바이너리에서 먼저 실행**해 audit 케이스 3개가 실패하는 것을 확인했습니다. 테스트가 실제로 버그를 잡는다는 증거입니다. -- 각 테스트에 `--permission` 강제 모드 단언을 대조군으로 넣어, 완화가 강제 모드로 새지 않음을 증명했습니다. -- 수정 후 3-모드 재현 매트릭스에서 audit 모드가 기준선과 완전히 일치했습니다. -- `parallel/test-permission*` 75/75, `ffi/test-ffi-permission-audit`, 전체 스위트(cctest 222개, jstest 5,942개) 실패 0으로 통과했습니다. +기존 `test-permission-audit-fs-does-not-deny.js`의 구조(`spawnSync` + 자식 프로세스에서 `dc.subscribe` + stdout 파싱)를 그대로 따랐습니다. -`symlink` 제한은 CVE-2025-55130 대응 코드라 완화 판단이 필요했습니다. 해당 커밋이 audit 모드 도입보다 앞서 있어 audit을 고려할 수 없었던 시점상 누락으로 보았고, 이 판단 근거와 반대 대안을 PR 본문에 함께 적어 security-wg가 판정할 수 있게 했습니다. +- `test-permission-audit-fs-lstat-symlink-does-not-deny.js` — audit 모드에서 통과 + 이벤트의 `permission`/`resource` 단언 +- `test-permission-audit-addons-does-not-deny.js` — audit 모드에서 `ERR_DLOPEN_DISABLED`가 아닌 `ERR_DLOPEN_FAILED`인지 단언(실제 `.node` 파일 없이 검증 가능) + +각 테스트에 **`--permission` 강제 모드 단언을 대조군으로** 넣었습니다. 완화가 강제 모드로 새지 않았다는 증명이 없으면 보안 관련 변경은 리뷰를 통과하기 어렵다고 판단했습니다. + +### 검증 순서 + +문서(`doc/contributing/writing-tests.md`)의 규칙을 따르되, 순서를 의도적으로 잡았습니다. + +1. **수정 전 바이너리에서 신규 테스트를 먼저 실행** — audit 케이스 3개만 실패하고 강제 모드 대조군 3개는 통과. 테스트가 실제로 이 버그를 잡는다는 증거를 먼저 확보했습니다. +2. 수정 후 3-모드 재현 매트릭스 재실행 — audit 모드가 플래그 없는 기준선과 완전히 일치, 강제 모드는 무변화 +3. `python3 tools/test.py --mode=release -J "parallel/test-permission*"` — 75/75 통과 +4. `ffi/test-ffi-permission-audit`, `parallel/test-fs-symlink*` — 통과 +5. `make test JOBS=14` 전체 스위트 — cctest 222개, jstest 5,942개, 실패 0 +6. `eslint`(JS 4파일), `python3 tools/cpplint.py src/env.cc` — 통과 + +### 판단이 필요했던 지점 — CVE 관련 코드 완화 + +`symlink` 제한은 [CVE-2025-55130](https://github.com/nodejs/node/commit/3296b2e734c) 대응으로 들어간 코드였습니다(2025-11, 비공개 보안 PR). 보안 조치를 완화하는 변경이라 근거가 필요했습니다. + +`git log`로 시점을 확인한 결과, 해당 CVE 커밋은 audit 모드 도입(#61869, 2026-05)보다 **앞서 있어 audit을 고려할 수 없었던** 상태였습니다. 의도적 예외가 아니라 시점상 누락으로 판단했고, 이 근거와 함께 **반대 대안**(symlink만 예외로 유지하고 문서에 명시)도 PR 본문에 병기해 security-wg가 판정할 수 있게 했습니다. + +### 신고자 응대 + +이슈 본문에 신고자가 "직접 고치고 싶다, 포인터가 필요하다"고 적어둔 상태였고 열흘간 답이 없었습니다. 선점 문제를 피하기 위해 이슈에 **원인 분석 코멘트를 먼저 올리고**(요청받은 포인터), PR 본문과 코멘트 양쪽에 "직접 작업 중이셨다면 기꺼이 양보하겠다"는 의사를 명시했습니다. ## 배운 점 -**한 함수만 읽고 단정하면 뒤집힙니다.** `SendStopSending()`을 읽고 앞서 작성한 문서가 틀렸다고 판단했는데, `is_readable()` 정의까지 펼쳐보니 로컬 단방향 스트림에서 `false`를 반환해 결국 문서가 맞았습니다. Node 코어의 술어 함수는 조건을 감추는 자리라 정의까지 확인해야 합니다. +### 1. 한 함수만 읽고 내린 판단은 뒤집힐 수 있다 + +이번 조사 중, 앞서 머지된 제 문서 PR(#64888)이 틀렸다고 판단한 순간이 있었습니다. `Stream::SendStopSending()`을 보면 `EndReadable()`이 방향 검사보다 먼저 무조건 실행되기 때문입니다. + +```cpp +void Stream::SendStopSending(error_code code) { + EndReadable(); // 무조건 실행되는 것처럼 보임 + if (!is_pending()) { + if (is_local_unidirectional()) return; +``` + +한 겹 더 내려가 보니 `is_readable()`이 로컬 단방향 스트림에 `false`를 반환하고, `EndReadable()`의 첫 줄이 `if (!is_readable()) return;`이었습니다. **결국 문서가 맞았습니다.** + +Node 코어에서 `is_readable()`, `is_writable()`, `is_pending()` 같은 술어 함수는 조건을 감추는 자리입니다. 호출 그래프를 한 겹만 보고 내린 결론은 정의까지 펼치면 반대로 뒤집힐 수 있다는 걸 실제로 겪었습니다. + +### 2. 이슈 목록의 빈자리는 대부분 이유가 있다 + +기여할 이슈를 찾으면서 열린 이슈 200건을 GraphQL로 교차참조와 함께 받아 분류했습니다. + +| 분류 | 건수 | +| --- | --- | +| 이미 열린 PR 있음 | 105건 (53%) | +| PR 전혀 없음 | 61건 | +| 머지된 PR만 있음(이미 수정됨) | 21건 | +| 닫힌 PR만 있음(시도 실패) | 13건 | + +절반 이상이 선점 상태였고, 비어 있는 61건도 본문을 읽어보면 대부분 이유가 있었습니다. 메인테이너가 "버그가 아니다"라고 판단했거나, 신고자가 철회했거나, 설계가 미확정이거나, 신고자 본인이 착수 예정이었습니다. 실제로 `#65509`는 신고자가 "구현 전 방향 합의를 구한다"고 적어둔 상태였고, `#63305`은 jasnell이 "backpressure model is still an open question"이라고 답한 상태여서 둘 다 제외했습니다. + +**교차참조만으로는 "PR 없음"까지만 알 수 있고, 임자·반대·철회는 스레드를 읽어야 드러납니다.** 자동 필터로 후보를 좁히되 최종 판단은 본문 정독이라는 절차를 세우게 되었습니다. + +### 3. 빌드 트리는 브랜치를 따라오지 않는다 + +한 달 전 빌드해둔 트리에서 새 브랜치를 만들고 `ninja`를 돌렸더니 `child_process_permission.cc` 없음 오류가 났습니다. 그 사이 `src/permission/`이 리팩토링되어(파일 통합, `BooleanPermissions` 도입) 기존 `build.ninja`가 무효화된 것이었고, `./configure --ninja` 재실행이 필요했습니다. + +또 하나, `make -j14 test`가 조용히 실패했습니다. 이 저장소의 Makefile은 `-j` 대신 `JOBS=14` 변수를 요구하는데, 잘못된 호출이 ninja에 빈 `-j`를 넘겨 즉시 죽었고 `tail`이 종료 코드를 삼켜 성공처럼 보였습니다. 출력 파일을 직접 열어보고서야 발견했습니다. **긴 명령의 결과는 종료 코드가 아니라 출력 내용으로 확인해야 한다**는 교훈입니다. -**이슈 목록의 빈자리는 대부분 이유가 있습니다.** 열린 이슈 200건을 교차참조로 분류하니 절반 이상이 이미 PR이 붙어 있었고, 비어 있는 것은 미확정 설계이거나 신고자가 직접 작업 중이었습니다. 이 이슈는 신고자가 "포인터가 필요하다"고 남긴 채 열흘간 답이 없던 경우라, 원인 분석을 코멘트로 공유하고 양보 의사를 함께 밝히는 방식으로 접근했습니다. +### 4. AI 사용 정책은 문서화되어 있다 -**빌드 트리는 브랜치를 따라오지 않습니다.** `src/permission/`이 리팩토링되어 한 달 전 생성된 ninja 파일이 무효화되었고, `./configure --ninja` 재실행이 필요했습니다. 또 이 저장소의 Makefile은 `make -j14`가 아니라 `make test JOBS=14`를 요구하는데, 잘못된 호출이 조용히 실패해 출력을 직접 열어보고서야 발견했습니다. +`doc/contributing/ai-guidelines.md`가 공식 정책으로 존재합니다. AI 사용을 금지하지 않지만 **도구 사용 사실과 개인 검증 내용의 공개**를 요구하고, 특히 리뷰 피드백 응답의 AI 자동화를 명시적으로 금지합니다. -AI 도구의 도움을 받았고, 재현·소스 추적·테스트 실행은 직접 수행했습니다. `doc/contributing/ai-guidelines.md`에 따라 커밋에 `Assisted-by` 트레일러를 남겼습니다. +관례를 확인하기 위해 `git log --grep="Assisted-by"`를 돌려보니 랜딩 커밋이 198개 있었고, TSC 멤버들도 사용하고 있었습니다. 이 형식을 따라 커밋에 `Assisted-by` 트레일러를 남기고, PR 본문에 직접 수행한 검증(재현, 소스 추적, 테스트 실행)을 밝혔습니다. 리뷰 응답은 정책에 따라 직접 작성할 예정입니다.