fix(streaming): validate the stream descriptor before using it

[arpitjain099]

MainAvailableStreamV2 has four fields the spec marks required, and the generated model renders all four as pointers. newStream takes the descriptor that ListAvailableStreamsOAuth2 returned and goes straight to reading them, so anything the API leaves out is a nil dereference rather than an error. Following examples/falcon_event_stream, a descriptor with no refreshActiveSessionInterval panics here:

panic: runtime error: invalid memory address or nil pointer dereference
    falcon/api_streaming.go:89 maintainSession

sessionToken and dataFeedURL go the same way a little later, and an interval of 0 gets past a nil check only to panic inside time.NewTicker with a non-positive interval.

The model already carries a Validate that checks exactly those fields, so newStream now calls it and returns the error, with a separate check for the interval since being present is not the same as being usable. Added a table test over the four shapes plus a nil descriptor, which panics on main and passes now.

[redhatrises]

Thanks for picking this up. Returning an error from newStream, instead of panicking on a bad server response, is the right approach. A few things still need to change before this is ready.

1. An interval of 1 still panics

maintainSession calculates the refresh period as nine tenths of refreshActiveSessionInterval, using integer math. For an interval of 1 that works out to 0. The new <= 0 check lets it through, and then time.NewTicker panics with non-positive interval for NewTicker. Very large values overflow the same way: math.MaxInt64 produces a negative period of -1.8s.

Suggestion: calculate the refresh period once, in time.Duration, and check the result instead of the raw interval. Then pass that period into maintainSession, so the value that was checked is the value that gets used. Working in time.Duration also removes the truncation, so a 1-second lifetime refreshes every 900ms instead of 0. Typical values stay the same: 1800 still gives 27 minutes.

2. The test fixture is invalid, so each case fails for the wrong reason

completeStream() doesn't set SessionToken.Expiration, but MainSessionToken.Validate requires it. So the "complete" fixture fails validation by itself:

validation failure list:
sessionToken.expiration in body is required

Every case therefore fails on that same error before it ever reaches the field it's named after. The <= 0 check never runs, and if you delete it the suite still passes. The test also only checks that some error came back, and no case shows that a valid descriptor is accepted.

3. Validate requires fields the stream never reads

The generated Validate also requires refreshActiveSessionURL and sessionToken.expiration. Nothing in api_streaming.go reads either one, because refresh goes through client.EventStreams.RefreshActiveStreamSession. A descriptor without those two fields works on main today but would be rejected after this PR. That affects callers who build descriptors by hand, and any API response that leaves those fields out.

Suggestion: replace Validate with explicit nil checks on only the fields newStream, open and url actually dereference:

• dataFeedURL
• sessionToken and sessionToken.token
• refreshActiveSessionInterval

Give each one its own clear error message. This also removes the need for the strfmt import.

Did you see the API leave out refreshActiveSessionInterval in practice, or is this a defensive fix? Either is fine; knowing what the API really returns helps decide how strict the checks should be.

4. Test suggestions

• Use a slice-based table, as api_client_test.go does, instead of a map, so the cases run in a fixed order. Add t.Parallel() too.
• go-openapi/swag is already a direct dependency, so swag.String and swag.Int64 can replace strPtr and i64Ptr.
• Make each negative case check its own specific error message, so it's clear which check fired.
• Add a case where a valid descriptor succeeds. The fakeTransport in api_client_test.go lets that run without the network.
• Add a few boundary cases for the refresh-period calculation: 1, a typical value such as 1800, 0, a negative number, and a value large enough to overflow.

I tried these changes locally, and gofmt, go vet and go test -race ./falcon/ all pass. Putting back either stream.Validate or the nine-tenths integer truncation makes the new tests fail. I'm happy to share the patch if that's useful.

Separately, for a follow-up issue rather than this PR: open() never checks the data-feed response status. So a 401 or 403 from the data feed is fed to the JSON decoder instead of being returned as an error.

[arpitjain099]

All four confirmed and fixed in fe182bb.

The fixture was broken exactly as you said: every case failed on sessionToken.expiration in body is required before reaching the field it was named for, and deleting the <= 0 check left the suite green.

On the period, one addition to your suggestion. time.Duration alone does not remove the overflow: time.Duration(math.MaxInt64) * time.Second * 9 / 10 wraps to -900ms, and some large intervals wrap back to a small positive period (20496382305 gives 790.448384ms), which would pass a check on the result and then fire the ticker every 790ms. So the interval is also bounded by math.MaxInt64 / (9 * time.Second / 10), 10248191152 seconds. The period is computed once and passed into maintainSession. 1800 still gives 27m, 1 gives 900ms.

Validate is replaced by nil checks on the four fields that get dereferenced, and the strfmt import went with it. Tests are now a slice table with t.Parallel(), swag helpers, a specific expected message per negative case, a success case through fakeTransport, and boundaries at 1, 1800, 0, -5, math.MaxInt64, the largest accepted interval and one past it. Putting the truncation back fails two cases, putting Validate back fails five. gofmt, go vet and go test -race ./falcon/ are clean.

To your question: defensive, not observed. I read the pointer dereferences in newStream, open and url rather than seeing the API omit the field, and I have not run this against a live tenant. The period arithmetic is a real bug either way, since it only needs a small interval.

Happy to open the open() response-status issue separately if you want it.

[redhatrises]

Ack. Thanks for the fix! I am going to add a follow up PR to address some other issues.