Skip to content

fix: correctness and security fixes from audit - #30

Open
mostafa wants to merge 11 commits into
mainfrom
fix/audit-correctness
Open

mostafa wants to merge 11 commits into
mainfrom
fix/audit-correctness

Conversation

@mostafa

@mostafa mostafa commented Oct 26, 2026

Copy link
Copy Markdown
Member

Summary

Fixes correctness and security issues found in an audit of the poller, output, and state paths. Each commit is one fix with its own test.

Data loss and duplication:

  • HTTP output no longer drops events after the cursor moves. The HTTP sink retried a failed batch three times (about 1.4s) and then dropped it, while the poller had already saved the cursor, so a short collector outage or a 401 lost events for good. Transient failures (5xx, 408, 429, timeout, connect) now retry until delivery with backoff capped at 30s. A non-retryable drop makes every later flush fail. Each strategy flushes the output before writing a checkpoint.
  • Dedupe forgets a failed poll's IDs. IDs were recorded before delivery, so a refetch after a failed poll was dropped as duplicates.
  • Hook pagination (Andromeda) fetched page one max_pages times. The buildRequest body always won over the getNextPage body, so each poll re-emitted the first page 100 times and commitState advanced skip by 5000, skipping real events. getNextPage now wins on later pages. An empty parseResponse array was treated as "no hook" and the whole GraphQL response was emitted as one event. A query-only buildRequest result was ignored.
  • page_offset and offset pagination resume from state. Both stored a watermark but never sent it, so every poll re-emitted from page 1. The watermark, incremental_from, or from value and query_params are now sent on every page.
  • Watermark and incremental_from write errors were discarded, and the per-page Link next_url write bypassed on_state_write_error.

Security:

  • GET /api/v1/config and /api/v1/sources/:id/config returned expanded secrets (for example output.http.headers.Authorization and the Postgres password in state.url). Header, query param, and body values, plus URL passwords and query values, are now redacted.
  • Postgres state uses verified TLS. It connected with NoTls, so sslmode=require (managed Postgres) failed and prefer sent credentials in plaintext. TLS now goes through rustls with the platform verifier, like the HTTP client. A dropped connection is re-established instead of failing every state call until restart.
  • Link next URLs must stay on the source origin, since every request carries the source's credentials.
  • DPoP jti is random. It was source_id plus the iat second, and the resource-request variant appended Instant::now().elapsed() (always about zero), so proofs within one second reused the same jti.

Scheduling and resilience:

  • Each source is polled on its own interval_secs. The scheduler slept for the smallest interval and then polled every source, so a 3600s source ran every 60s next to a 60s one. Ticks now run on a config snapshot instead of holding the config read lock.
  • The circuit breaker failure rate divided consecutive failures by every request since startup; it is now evaluated over windows of minimum_requests.
  • A rate limit of 0.5 rps was rounded up to 1 rps.
  • Retry backoff panicked on a negative or NaN multiplier, and jitter could exceed max_backoff_secs. A huge expires_in from a token server overflowed Instant and panicked.
  • max_line_bytes_behavior: truncate produced invalid JSON and counted characters rather than bytes. The line now stays valid NDJSON, with event replaced by a prefix of its JSON text and meta.truncated: true.
  • Link headers with commas inside URIs or multi-value rel now parse.

Behavior changes to note on upgrade:

  • A Postgres server that offers TLS with a certificate not trusted by the system (for example a Debian snakeoil certificate) now fails to connect under the default sslmode=prefer. Add the CA to the trust store or set sslmode=disable.
  • When the HTTP output endpoint is down, polling pauses at the next checkpoint instead of continuing and dropping events. After a non-retryable rejection (for example 401), checkpoints stay paused until restart.

New dependencies: tokio-postgres-rustls 0.14, rustls-platform-verifier 0.7, and rustls 0.23 (the last two were already in the tree through reqwest). cargo audit reports nothing new.

Test plan

  • cargo fmt -- --check
  • cargo clippy --all-targets --all-features -- -D warnings and without features
  • cargo doc --all-features --no-deps
  • cargo test --all-features (273 tests) and cargo test with default features
  • cargo test --features testcontainers --test integration_testcontainers
  • New tests fail on main and pass here: hook pagination, page/offset resume, HTTP output failure keeps the watermark, cross-origin Link refusal
  • Live: two sources at 2s and 8s are polled 5 and 2 times over 9s
  • Live: /api/v1/config and the source config endpoint return no secrets with HTTP_SINK_TOKEN and a source key set
  • Live: Postgres with ssl=on and sslmode=require performs the TLS handshake and rejects the self-signed certificate; with sslmode=disable, polling continues across docker restart of Postgres after one reconnect

Also treat an empty parseResponse array as an empty page instead of
falling back to default parsing, and send buildRequest query params
when the hook returns only a query.
The scheduler slept for the smallest interval across all sources and
then polled every source, so a 3600s source ran every 60s next to a
60s one. Track a next-due time per source and run only due sources.
Each tick now runs on a config snapshot instead of holding the config
read lock, so reload no longer waits for an in-flight poll.
Both strategies stored a watermark but never sent it, so every poll
restarted from the first page and re-emitted the same events. Send the
watermark, incremental_from, or from value, plus query_params, on every
page.
The HTTP sink queued lines and dropped a batch after a few retries or
on any non-5xx response, while the poller had already saved the
cursor, so a short collector outage lost events for good. Retry
transient failures until delivery, make every later flush fail once a
batch is dropped, and flush the output before each checkpoint.
Watermark and incremental_from write errors are now propagated, and the
per-page Link next_url write honors on_state_write_error.
Refuse to follow a next URL on another origin, since every request
carries the source's auth. Parse Link headers by <uri> segments so
commas inside URIs and multi-value rel attributes work.
DPoP proofs used source_id plus the iat second as jti, and the
resource-request variant appended Instant::now().elapsed(), which is
always about zero, so proofs within one second reused the same jti and
servers that track jti reject the replay. A huge expires_in from the
token server overflowed Instant and panicked.
…, and backoff math

The circuit failure rate divided consecutive failures by every request
since startup, so it almost never opened on mixed failures; evaluate
it over windows of minimum_requests. A rate limit of 0.5 rps was
rounded up to 1 rps. Retry backoff panicked on a negative or NaN
multiplier, and jitter could exceed max_backoff_secs.
Truncation cut the serialized line and appended "...", producing
invalid JSON, and counted characters instead of bytes, so multibyte
events still exceeded the limit. Replace event with the longest prefix
of its JSON text that fits and mark meta.truncated.
Dedupe recorded each ID before the event was delivered. When the poll
then failed (output flush, write error, timeout) the next tick refetched
the same events from the unchanged checkpoint and dedupe dropped them as
duplicates. IDs added during a poll are now kept only if it succeeds.
The Postgres state store connected with NoTls, so sslmode=require URLs
(managed Postgres) could not connect and prefer sent credentials in
plaintext. Connect through rustls with the platform verifier, matching
the HTTP client. A dropped connection also failed every later state
call until restart; reconnect when the client reports it closed.

This branch has not been deployed

No deployments
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