mlevkov commented on PR #3804: URL: https://github.com/apache/iggy/pull/3804#issuecomment-5444560821
Rebased onto master first, which was 24 commits ahead. That turned out to matter: `RuntimeContext::state_path` became a `StateStorageFactory` while this branch sat, so the API test context no longer compiled. Fixed as its own commit before anything else, so the rest of the diff is only about your comments. Of the 23 inline comments, 13 are from the first round and 12 of those were already answered by the three follow-up commits. I checked each against the code rather than against my own commit messages before deciding not to touch it. The thirteenth is the `ip_nonlocal_bind` one, which your later comment partly retracts, so it is answered below with the newer version. ## The ten from this round **`warn_on_weak_containment` ignores `allowed_origins`.** Correct, and the covering test locked it in by leaving the list empty. The predicate now lives on `HttpCorsConfig` and `configure_cors` calls it too, so the warning cannot drift from the mapping it describes. The test sets `["*"]`, and a new one asserts a pinned list produces no warning. While fixing the false positive I found the false negative: the predicate also has to catch `null`, which is what a browser sends from a sandboxed iframe, a `data:` URL and a `file://` page. `AllowOrigin::list` echoes any listed value on a match, so pinning `null` buys nothing an attacker cannot produce at will. It is a narrower set than `*`, just as worthless to pin. That could not widen `allows_any_origin`, since `configure_cors` still has to build a one-entry list for it, so the warning gets `allows_unowned_origin`, defined in terms of the other so the two cannot disagree about `*`. **CORS allows writes, not just reads.** Fixed in the warn string and in the README, with the mechanism named: the preflight is answered inside the CORS layer before `resolve_api_key` runs. The README also now says the rewrite half needs the method on `allowed_methods` and `content-type` on `allowed_headers`, both of which the shipped block grants. I kept the warn string at "read and rewrite" rather than hedging it, because that is true of the shipped block and hedging it cost more characters than your next comment allows. **`POST .../restart` is CORS-simple.** Documented as its own item, separate from the three edits, because it is true of the defaults rather than of a change an operator made. No startup warning covers it for that reason, and the README says so. Also raised on #3802, whose first suggested fix would close it. **`dlopen` reads like restart does it.** Split into its own paragraph: `start_connector` reuses the container loaded at boot and only re-runs the plugin's init, so a hostile `path` waits for the next process start. Deferred rather than immediate. **Restart uses the highest stored version.** Both endpoint rows now say so and point at #3848. The notice paragraph carries the same qualifier, since it only holds on the local provider. **`config.toml` is weaker than the example.** All three copies now carry the three-line version, byte-identical. The README's own TOML sample was the third copy and had the same gap. I also put a caution above `[http.cors] enabled`, in all three, since flipping that one flag is what voids the containment the `[http]` comment leans on. **Disabled-API test has no positive control.** It now asserts the "HTTP API is disabled" line landed first. **"Outlives the run" is wrong, and `ip_nonlocal_bind`.** Clause dropped from the assertion message and from the const doc. Both tests pre-flight the bind, and on a host that binds a documentation-range address anyway only the two bind-dependent assertions stand down. They do not skip: a skipped test reports `ok` with no signal, and the warning itself is required on every host. The test at the second site was discarding its join result entirely, so it now asserts the same thing as the first. **Three warnings of 246/208/221 characters.** Now 197/212/206, one sentence each plus a pointer, and each names the exact key to change. The pointer is a URL rather than a repo path, because the published image carries the binary and its licences and no source tree, so the reader most likely to see these lines has nowhere to look one up. That URL is 76 characters, which is why the CORS warning lands 4 over the 208 you measured while the other two drop by 49 and 15. Say the word if you would rather have the four characters back than a fetchable link. Two of them still interpolate the same address, which I left deliberately: they fire independently on different runs, and dropping the address from either makes that one useless on its own. **Capture guard installed before the fixture.** Moved after `context()` in every test. ## Found while doing the above, not by you `Ipv6Addr::is_loopback` matches only `::1`, so `[::ffff:127.0.0.1]` bound to 127.0.0.1 and still warned. The classifier unmaps first now. The README claimed "no redaction layer anywhere in the runtime", which one grep disproves. The true and narrower claim is that nothing redacts plugin configuration; the runtime does redact its own secrets. Scoped accordingly. The `DELETE .../configs` rows I added in the first round said "configuration versions". Each call removes exactly one, defaulting to the active version, which is the more dangerous misreading of the two. The TLS warning is the one that survives setting a key, and it was the one warning the README never mentioned. ## Left alone on purpose `configure_cors` panics on a wildcard anywhere but first, on an unparseable origin or header, on an unknown method, and on `allow_credentials = true` with `["*"]`. That is consistent with bad config being fatal in this runtime, and making it recoverable is a signature change on a function with existing callers. Happy to take it as a follow-up if you would rather it produced an error. ## What a second review round changed Worth saying plainly, because it caught things I would have shipped. A mutation run found three conjuncts in `warn_on_weak_containment` with no negative coverage at all: dropping `unauthenticated`, `cors.enabled` or `!tls.enabled` each left the whole suite green. The middle one is the shipped block, a wildcard origin list that is not switched on, so losing that conjunct would have warned on every stock deployment, which is the exact false positive your comment was about. All three are covered now, and `configure_cors` has its first tests: the shapes the predicates classify all build, and a wildcard past the first position panics rather than allowing nothing, which is what the doc comment claims. The same round found the README claiming `["null"]` is "no narrower than `*`" (it is narrower, it is just as worthless to pin), the redaction sentence asserting more than the code does, and the DELETE rows describing the local provider's fallback as if every provider had it. 195 unit tests and the 8 `connectors::api` integration tests pass. fmt, sort, clippy `--all-features --all-targets`, and the `RUSTDOCFLAGS=-D warnings` gate from #3925 are all clean. Every guard was mutation-checked before the commit that introduced it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
