Skip to content

fix: bound Fernet slot runs, Kiro error-body read, and skill-path line slice - #5310

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/t4-response-bounds
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:fix/t4-response-bounds

Conversation

@luvs01

@luvs01 luvs01 commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Cap embedded Fernet token runs per slot at 64 so one encrypted_content slot cannot amplify into an unbounded number of request parts.
  • Read Kiro text-fallback HTTP error bodies through readDisplaySafeErrorPayloadText (bounded, abort-aware) instead of an unbounded response.text().
  • Bound the skill-bundle first-line path slice in maybeElideSkillText to 4,096 chars; oversized marker paths pass through unchanged.

Verification

  • bun test tests/codex-integration/multi-agent-compat.test.ts tests/claude-integration/claude-inbound.test.ts tests/providers/kiro/kiro-stream.test.ts — 228 pass, 0 fail.
  • bun x tsc --noEmit — clean.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Review readiness checklist

This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:

  • All CI tests are green on my local testing.

  • I pushed my PR to the latest dev commit.

  • I resolved all correct Codex and CodeRabbit findings.

  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Sep 20, 2026
@github-actions

github-actions Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

Automatic ready-for-review conversion failed; please mark the pull request ready manually if it is still a draft.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers notified: @lidge-jun @Ingwannu

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 64 / 80

이 PR은 “한 곳이 커지면 전체가 무한정 커지는” 길을 세 군데에서 막습니다. 첫째, 섞여 있는 encrypted_content 슬롯 안에서 Fernet 토큰을 찾을 때 슬롯당 최대 64개까지만 잘라 내고, 그 이상은 요청 조각으로 더 쪼개지 않습니다. 둘째, Kiro가 글자 폴백(text-fallback)으로 다시 요청했다가 HTTP 에러가 나면, 예전처럼 response.text()로 응답 본문을 끝없이 읽지 않고 readDisplaySafeErrorPayloadText로 읽고, 어댑터가 넘긴 abortSignal도 같이 씁니다. 셋째, Claude 쪽 스킬 문서 줄이기(maybeElideSkillText)에서 “Base directory for this skill: …” 뒤 경로를 첫 줄에서 꺼낼 때 최대 4,096자만 보고, 그보다 길고 줄바꿈도 없으면 그냥 원문을 둡니다. base는 dev이고, Fernet 상한·과대 경로·Kiro 에러 본문 취소에 대한 테스트가 붙어 있습니다. 지금은 draft이고 readiness 체크리스트는 0/4입니다.

라인 - src/server/responses/encrypted-payload.ts fernetTokenRuns / encryptedSlotParts: 64개에서 끊으면, 나머지 토큰은 맨 끝 input_text에 통째로 남습니다. 테스트도 encrypted_content 64개 + 마지막이 input_text인 것을 기대로 잡습니다. 증폭(DoS)은 막히지만, 남은 암호문이 “읽을 수 있는 글”처럼 취급될 수 있습니다. hasUnreadableEncryptedAgentTask도 같은 fernetTokenRuns를 쓰므로, 65번째 이후는 평문처럼 보일 수 있습니다.
라인 - src/claude/inbound.ts maybeElideSkillText: 경로가 4,096자를 넘고 그 안에 줄바꿈이 없으면 줄이기를 하지 않고 원문을 통과시킵니다. 메모리 폭주를 막는 쪽은 맞지만, 일부러 긴 경로를 넣으면 blockedSkills 대상 스킬 문서가 그대로 남을 수 있습니다(줄이기 실패 시 통과).
라인 - 같은 함수: basename을 split/pop 대신 lastIndexOf("/")로 바꿨고 끝 /도 뗍니다. Windows C:\…/ 정규화와 C:claude-api 같은 기존 케이스는 대체로 맞습니다. 다만 “줄이기 실패 = 통과”와 맞춰, 과대 경로 테스트는 “통과한다”만 확인하고 “막힌 스킬이면 줄여야 한다”는 쪽은 없습니다.
라인 - src/adapters/kiro/stream.ts + adapter.ts: 폴백 에러 본문을 bounded reader로 바꾼 것은 분명한 이득이고, 테스트도 큰 본문에서 cancel·적은 pull을 봅니다. abortSignal은 인터페이스상 선택(?)이라, 나중에 폴백을 다른 경로에서 만들면 신호가 빠질 수 있습니다(지금은 어댑터 한 곳이 넣음).
라인 - PR 상태: draft + readiness 0/4. 작성자 검증·체크리스트는 합류 전 일입니다.

메인테이너의 판단이 필요한 지점

Fernet 64개 상한 이후 남은 토큰을 input_text로 둘지, 버리거나 [encrypted content omitted]처럼 지울지. 스킬 경로가 과대일 때 “줄이지 않고 통과”가 맞는지, 아니면 줄이기를 포기하는 대신 본문만 잘라낼지.

너의 추천

세 군데 상한 모두 방향이 좋고 테스트도 핵심을 칩니다. 합류 가치 있는 방어 패치입니다. 다만 Fernet 잔여분을 input_text로 남기는 동작은 보안 PR답게 메인테이너가 한 번 고르고, 선택하면 주석이나 테스트 이름에 “알고 가는 타협”이라고 짧게 남기면 좋겠습니다. 스킬 쪽도 과대 경로 = 통과가 의도면 그대로 두고, draft 체크리스트를 채운 뒤 ready로 올리면 됩니다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun

Copy link
Copy Markdown
Owner

Carried into #5619 at fe13531 with your authorship kept. Follow-ups make a slot with more than 64 Fernet runs fail closed, move the Kiro regression into a registered sibling to respect the file-size cap, and document the new bounds. Closing in favor of #5619. Thank you!

@lidge-jun lidge-jun closed this Sep 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants