fix: two carry-over hotfixes (referencegrant name filter, anthropic tracing negative idx) - #10
Merged
Merged
Conversation
…ation
Per Gateway API spec, `ReferenceGrantTo.Name` is optional. When set, the grant
must apply only to that named target; when unset, it acts as a wildcard for
the group/kind. The controller was ignoring the field entirely, so a
name-scoped grant like
to:
- group: aigateway.envoyproxy.io
kind: AIServiceBackend
name: safe-backend
silently allowed AIGatewayRoutes to reference *any* AIServiceBackend of that
group/kind in the target namespace — not what the grant author meant. In
practice this widens cross-namespace permission surface beyond what the
policy declared.
Thread `targetName` through validateReference → isReferenceGrantValid →
matchesTo so the name comparison happens at the leaf. Wildcard behaviour
(name unset) is preserved.
Regression test added covering all three cases: name-unset wildcard,
name-matches (allowed), name-mismatch (denied).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…egatives
The upper-bound check on ContentBlockDelta/Stop was `idx < len(response.Content)`.
In Go, that condition is trivially satisfied by negative ints, so a malformed
or hostile upstream chunk with `Index: -1` slipped through and panicked on the
`response.Content[idx]` access below. The panic surfaces inside the tracing
pipeline (Anthropic messages recorder) and takes the request down when
OpenInference tracing is enabled.
Only ContentBlockStart had the non-negative guard. Extract the shared check
into `isValidContentBlockIndex(idx)` and apply it to every SSE index path
(Start / Delta / Stop). The upper bound (maxContentBlocks = 1000) is preserved
so a giant Index can't force a giant allocation either.
Regression test added: a chunk sequence with a valid Start followed by
ContentBlockDelta{Index:-5} and ContentBlockStop{Index:-1} now completes
without panic and produces the same content as if those events were absent.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting Disabled knowledge base sources:
📝 WalkthroughWalkthroughReferenceGrant가 대상 리소스 이름을 검증하도록 변경되었고, Anthropic 스트림 변환기가 잘못된 콘텐츠 블록 인덱스를 안전하게 건너뛰도록 보강되었다. 두 변경 모두 회귀 테스트가 추가되었다. ChangesReferenceGrant 이름 매칭
Anthropic 콘텐츠 블록 인덱스 검증
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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
1 participant
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.
Summary
Two carry-over defects flagged during earlier PR reviews (#4/#5/#6) that we consciously deferred as out-of-scope for those integration PRs. Both are upstream-inherited and low-blast-radius today, but real bugs — landing them explicitly so upstream can pick them up too.
1.
internal/controller/referencegrant.go— honorReferenceGrantTo.NamePer Gateway API spec,
ReferenceGrantTo.Nameis optional. When set the grant is scoped to exactly that named target; when unset it acts as a wildcard for the group/kind. The controller was ignoring the field entirely, so a name-scoped grantsilently allowed AIGatewayRoutes to reference any AIServiceBackend of that group/kind in the target namespace. Not what the grant author meant, and widens cross-namespace permission surface beyond the declared policy.
Thread
targetNamethroughvalidateReference → isReferenceGrantValid → matchesToand add the name comparison at the leaf. Regression test covers wildcard (name unset), name match (allowed), name mismatch (denied).2.
internal/tracing/openinference/anthropic/messages.go— negative index panicContentBlockDelta/ContentBlockStopguarded the upper bound (idx < len(response.Content)) but not the lower bound. In Go, negative ints satisfy that condition, so a malformed / hostile SSE chunk withIndex: -1slipped through and panicked on the slice access below. Panics inside the tracing pipeline take the whole request down when OpenInference tracing is enabled.Only
ContentBlockStarthad the non-negative guard. Extracted the shared check intoisValidContentBlockIndex(idx)and applied it to every SSE index path. Upper bound (maxContentBlocks = 1000) preserved.Regression test locks in the fix: Start(0) + Delta{-5} + Stop{-1} now completes cleanly.
Not in this PR
Third finding from the reviews —
processor_impl.goauth handler receiving an empty body on the CONTINUE branch when a signature-based backend needs the actual current body — is deferred. Reproducing it needs GCP-Vertex-AI or AWS-Bedrock signing under specific request-shape conditions and warrants its own investigation; landing a speculative fix here would just add another moving part.Verified
go build ./... && go vet ./...✓golangci-lint run→ 0 issuesgo test ./internal/controller/... ./internal/tracing/openinference/anthropic/...passes including new regression cases🤖 Generated with Claude Code
Summary by CodeRabbit
버그 수정
테스트