Skip to content

chore(issue-22): v0.2.0 코드 리뷰 반영 — tidy-first 5개 커밋#29

Merged
ppzxc merged 5 commits into
mainfrom
chore/issue-22
Mar 21, 2026
Merged

chore(issue-22): v0.2.0 코드 리뷰 반영 — tidy-first 5개 커밋#29
ppzxc merged 5 commits into
mainfrom
chore/issue-22

Conversation

@ppzxc

@ppzxc ppzxc commented Mar 21, 2026

Copy link
Copy Markdown
Owner

Summary

  • Webhook Sender에 공유 HTTP Transport 추가 — TCP 커넥션/TLS 세션 재사용
  • InputType.IsValid() dead code 제거
  • GET /inputs/{inputId}/messages/{messageId} placeholder → 501 Not Implemented
  • TCP 수신기 1 MiB 경계값 테스트 추가 (accepted / exceeded)
  • buildEvalData() 내 ParsedData 키가 내장 키(input, id 등)를 덮어쓰는 보안 버그 수정

Motivation

PR #21 코드 리뷰 제안 사항(issue #22)을 tidy-first 원칙에 따라 처리.
구조 변경(structural) 먼저 커밋 후, 동작 변경(behavioral)을 TDD 사이클로 후속 커밋.

Changes

  • webhook/sender.go: Sendertransport/insecureTransport 필드, resp.Body drain 추가
  • domain/input_type.go: IsValid() 삭제 (호출 0건)
  • http/router.go: placeholder 핸들러를 writeError(501) 로 교체
  • http/handler_test.go: TestGetMessageByID_Returns501 추가
  • tcp/listener_test.go: TestListener_MaxMessageSize_Accepted/Exceeded 추가
  • service/relay_worker.go: builtinEvalKeys 가드로 ParsedData 키 충돌 차단
  • service/relay_worker_test.go: TestRelayWorker_ParsedDataDoesNotOverrideBuiltinKeys 추가

Test plan

  • 빌드 확인 (CGO_ENABLED=1 go build ./cmd/server/)
  • 전체 테스트 통과 (go test -race ./... -timeout 60s)
  • 정적 분석 (go vet ./...)

@ppzxc ppzxc left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 리뷰

전반적으로 tidy-first 원칙 준수, TDD RED→GREEN 명확, 보안 버그 수정(commit 5)이 핵심. 품질 높음.


🟡 개선 권장 (blocking 아님)

1. webhook/sender.go&http.Transport{} 제로값 사용

http.DefaultTransport의 운영 설정(IdleConnTimeout: 90s, TLSHandshakeTimeout: 10s, MaxIdleConns: 100)이 적용되지 않음.

  • TLSHandshakeTimeout: 0 → TLS 핸드셰이크가 무기한 대기 가능
  • IdleConnTimeout: 0 → 유휴 커넥션이 만료 없이 누적

개선안:

func NewSender() *Sender {
    base := http.DefaultTransport.(*http.Transport).Clone()
    insecure := http.DefaultTransport.(*http.Transport).Clone()
    insecure.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec
    return &Sender{transport: base, insecureTransport: insecure}
}

→ 별도 이슈로 추적


2. http/router.go — 501 핸들러에 인증 미적용

토큰 없는 요청도 501 반환. 실제 구현 시 인증 로직 추가를 잊을 수 있음.
현재 placeholder는 수용 가능하나, 구현 시 auth 추가를 반드시 체크리스트에 포함할 것.

→ 별도 이슈로 추적


✅ 잘 된 점

  • relay_worker.go:builtinEvalKeys — 패키지 레벨 var로 반복 할당 방지, 방어 로직 명확
  • io.Copy(io.Discard, resp.Body)defer Close() 전에 drain하여 커넥션 재사용 보장
  • TCP 경계값 테스트가 maxMessageBytes 미노출 상수를 같은 패키지에서 직접 참조 → 리팩터링 내성 확보
  • InputType.IsValid() dead code 제거 — 범용 릴레이 설계 의도와 일치

ppzxc added 5 commits March 21, 2026 12:19
Sender now holds two *http.Transport fields (secure/insecure) initialised
in NewSender(). Send() selects the appropriate transport and wraps it in a
per-call http.Client — connection pool is reused across calls.

Also drains resp.Body before Close() to guarantee keep-alive reuse.
Zero callers in production or test code. Hardcoding three fixed types
(BESZEL/DOZZLE/GENERIC) contradicts the generic-relay design; removing the
method avoids confusion and reduces maintenance surface.
Route previously delegated to h.Healthz (200 OK), misleading callers
into thinking the endpoint was functional. Now returns 501 Not
Implemented with a problem+json body.

TDD: RED (200) → GREEN (501).
Accepted: (maxMessageBytes-1)-byte payload is delivered normally.
Exceeded: (maxMessageBytes+1)-byte payload is silently dropped by
bufio.Scanner (ErrTooLong) and Receive is never called.

Documents existing behaviour; no production code changed.
buildEvalData() was unconditionally merging msg.ParsedData into the eval
map, allowing a ParsedData field named "input" (or "id", "payload", etc.)
to silently overwrite the authoritative builtin value. This could cause
filter/routing expressions to evaluate against attacker-controlled data.

Guard with a package-level builtinEvalKeys set; ParsedData keys that
collide with builtins are silently ignored.

TDD: RED (filter failed when builtin was overwritten) → GREEN.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant