fix(ofrep): new SSE event stream endpoint and conditional request support#5957
Conversation
|
The latest Buf updates on your PR. Results from workflow Proto / proto-lint (pull_request).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: comment
Clean PR fixing OFREP cross-origin caching and removing unused response fields. Add reserved for the deleted proto field to stay consistent with project conventions.
rpc/flipt/ofrep/ofrep.proto
- minor (L40): Reserve the deleted
originfield (reserved 2; reserved "origin";) before the closing brace to follow the project's protobuf conventions (e.g.,rpc/v2/evaluation/evaluationv2.proto) and prevent accidental reuse of the field number.
🤖 Automated review by the Flipt PR review agent.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v2 #5957 +/- ##
==========================================
+ Coverage 61.29% 61.41% +0.12%
==========================================
Files 141 142 +1
Lines 14211 14265 +54
==========================================
+ Hits 8710 8761 +51
+ Misses 4772 4770 -2
- Partials 729 734 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
size:M
| string request_uri = 1 [(google.api.field_behavior) = REQUIRED]; | ||
| // The scheme + host + optional port portion of the endpoint URL. | ||
| string origin = 2 [(google.api.field_behavior) = OPTIONAL]; | ||
| } |
There was a problem hiding this comment.
we should add this field to reserved like the review says so we dont accidentally reuse it in the future
There was a problem hiding this comment.
Actually, I am blind... I've missed optional in definition which causes the issue. Thanks for pointing on this.
9461496 to
88d0a4f
Compare
There was a problem hiding this comment.
Verdict: comment
Clean PR fixing OFREP cross-origin conditional request support by exposing Etag and allowing If-None-Match through CORS, while correcting the bulk evaluation response format. One clarification needed: the origin proto field was made explicitly optional, not deleted, so adding reserved for it would conflict with the still-active field and break protobuf compilation.
rpc/flipt/ofrep/ofrep.proto
- minor (L40): The
originfield was changed fromstringtooptional string, not removed. Field number 2 is still actively used byorigin, so addingreserved 2;orreserved "origin";would cause a protobuf compilation error. No reservation is needed—this is a wire-compatible presence-tracking change that keeps the field in place.
🤖 Automated review by the Flipt PR review agent.
- The origin field was marked as optional in grpc and it was always returned as an empty string, which breaks the ofrep-web JS client logic for constructing the streaming URL. - The snapshotDigest was removed to align with current implementation details in the ofrep-web JS client. Removing it causes an extra re-evaluation fetch to the bulk endpoint on each (re)connect. Ideally, it should be avoided but I don't see a good solution at the moment. - Include Etag in CORS exposed headers for cross-origin reads. The Access-Control-Expose-Headers response header only listed Link, preventing browser JS from reading the Etag header cross-origin. This broke conditional request caching via If-None-Match. Added Etag to the exposed headers list. related #5669 61aa322 Signed-off-by: Roman Dmytrenko <[email protected]>
88d0a4f to
85a0cf4
Compare
There was a problem hiding this comment.
Verdict: approve
Clean, focused PR. The CORS changes correctly expose ETag and allow If-None-Match for cross-origin conditional requests, the proto origin field is appropriately made optional for presence tracking, and the snapshotDigest removal aligns the response with current client expectations. Tests and schemas are consistently updated. No issues found.
🤖 Automated review by the Flipt PR review agent.
dosubot
Bot
added
size:L
There was a problem hiding this comment.
Verdict: request changes
There's a critical authentication bypass: when authentication.exclude.ofrep is enabled, any external HTTP client can skip authentication for all client evaluation endpoints by sending the X-Flipt-OFREP-Stream header. The inproc peer check doesn't distinguish internal alias requests from direct external requests because all HTTP traffic goes through the in-process gRPC channel. Use a context-bound signal that external clients cannot spoof instead of forwardable HTTP metadata.
internal/server/evaluation/client/server.go
- critical (L197): Authentication bypass for all client evaluation endpoints when
authentication.exclude.ofrepis enabled.SkipsAuthenticationreturnstruewhenX-Flipt-OFREP-Streammetadata equals"true"and the peer network isinproc. Both conditions are trivially met by any external HTTP client:
ForwardFliptOFREPStream(ininternal/server/middleware/grpc/middleware.go) forwards this header by reading the raw*http.Requestheader map, bypassing theisReservedHeaderblock in the gateway's incoming header matcher.- Every HTTP request goes through the in-process gRPC channel (
inprocgrpc), so the peer is alwaysinprocregardless of whether the request came from the internalofrepStreamHandleralias or a direct external call to/client/v2/....
An attacker can therefore evaluate flags or stream snapshots without authentication by sending X-Flipt-OFREP-Stream: true to any client evaluation path.
Fix: Stop relying on a client-controllable HTTP header. Have ofrepStreamHandler set a context value (e.g. via context.WithValue) that external clients cannot inject, and check for that context value in SkipsAuthentication instead of gRPC metadata. This removes the spoofing surface entirely.
🤖 Automated review by the Flipt PR review agent.
Adds an HTTP alias for the client evaluation SSE stream at
/ofrep/v1/_stream/{environmentKey}/{namespaceKey}/events that rewrites
to the existing /client/v2/environments/{key}/namespaces/{key}/stream.
The handler accepts only text/event-stream requests (406 otherwise),
sets an internal marker header for auth exclusion, and forces the
marshaler to eventSourceMarshaler.
Authentication respects authentication.exclude.ofrep: the stream
endpoint skips auth only when the config is enabled, the internal
marker is present, and the call originated through the in-process
gRPC gateway channel (not direct gRPC). A global incoming header
matcher rejects the marker header from clients to prevent spoofing.
The OFREP bulk evaluation response now advertises the new path.
Signed-off-by: Roman Dmytrenko <[email protected]>
af10b31 to
08903c7
Compare
There was a problem hiding this comment.
Verdict: approve
Clean, focused PR. The OFREP stream endpoint alias correctly uses an unspoofable context-bound signal to conditionally skip authentication, the CORS changes properly expose ETag and allow If-None-Match for cross-origin conditional requests, and the wire-compatible proto change to make origin optional aligns with the client expectations. Tests cover the new alias routing, auth skipping logic, and CORS behavior. No issues found.
🤖 Automated review by the Flipt PR review agent.
erka
changed the title
2 participants
related #5669 61aa322