Repository navigation
Retain provider-local HTTP opt-in in the demo baseline - #1533
ChristianPavilonis wants to merge 7 commits into
Conversation
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The provider-local allow_insecure_http opt-in is correct and well covered: the flag gates only the http scheme, does not leak to other providers or schemes, is omitted from serialized output when false, and the scheme flows through to the backend spec so an opted-in endpoint is actually sent as plain HTTP. One blocking item: the current-head hosted check cargo test (axum native) fails from an inherited test fixture that this PR describes as corrected but which was corrected only in a sibling file.
1 of the inline comments below carries a one-click GitHub
suggestion. The blocking finding is body-level because its file is outside this PR's diff.
Blocking
🔧 wrench
startup.rsstill has the self-connection listener probe — see Cross-cutting below.
Non-blocking
⛏ nitpick
unwrap()in the new serialization test — see inline atcrates/trusted-server-core/src/auction/plan.rs:1186
Cross-cutting / body-level findings
- 🔧
startup.rsstill has the self-connection listener probe.crates/trusted-server-adapter-axum/tests/startup.rs:116(assert_failed_before_listen) treats any successfulTcpStream::connect_timeoutas "the server is listening". On Linux a connect can succeed with no listener when the ephemeral source port equals the target port. The PR description says this was corrected; the correction in70a894547addedconnected_to_listener(local address != peer address) tonative_acceptance.rsonly, andstartup.rskept the bareis_err()probe. At this head the Linux runner hit the case:missing_config_fails_before_listen_without_legacy_fallbackpanicked atstartup.rs:115("should not listen while required startup inputs are invalid"). The base branch passed with identical fixture content, so this is intermittent and inherited rather than introduced by the HTTP change. Minimal correction: apply the same local-vs-peer exclusion inassert_failed_before_listen(theconnected_to_listenerhelper fromnative_acceptance.rs). The file entered in the native unit and was last touched by the geo unit, so the fix belongs there and merges upward through the stack. Not a suggestion because the file is outside this diff. - 📝 Configuration guide now contradicts the new field.
docs/guide/configuration.md:2090still says all provider endpoints must be absolute HTTPS URLs, and the provider-field table at:2153has noallow_insecure_httprow, whileauction-orchestration.mddocuments the opt-in. Optional consistency fix. - 📝 Rollback note, no action needed.
ProviderConfighasdeny_unknown_fields, so a config blob withallow_insecure_httpset is rejected by any binary that predates this change; a demo config using it cannot roll back the binary alone. Default configs are unaffected because the field is omitted when false.
Verification notes
- Focused
auction::plansuite at head: 32 pass (host triple, native). - Mutation controls on the new guard: accepting
httpwithout the flag failsinsecure_http_requires_an_explicit_per_provider_opt_in; letting the flag accept any scheme failsinsecure_http_opt_in_does_not_allow_credentials_fragments_or_other_schemes. startupfixture on macOS: 3/3 pass (the Linux self-connection does not reproduce there).- The inline suggestion was applied in a scratch tree:
cargo fmt --check,cargo clippy -p trusted-server-core --lib --tests -D warnings, and the three focused tests pass.
CI Status
- cargo test (axum native): FAIL
- CLAUDE.md symlink guard: PASS
- browser integration tests: PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip2): PASS
- cargo fmt: PASS
- cargo test: PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- docs-proposal scripts: PASS
- format-docs: PASS
- format-typescript: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- vitest: PASS
| let legacy = serde_json::to_value(provider("prebid-server")).unwrap(); | ||
| assert!(legacy.get("allow_insecure_http").is_none()); | ||
| assert!( | ||
| !serde_json::from_value::<ProviderConfig>(legacy) | ||
| .unwrap() | ||
| .allow_insecure_http | ||
| ); | ||
| let mut opted_in = provider("prebid-server"); | ||
| opted_in.allow_insecure_http = true; | ||
| let encoded = serde_json::to_value(opted_in).unwrap(); | ||
| assert_eq!(encoded["allow_insecure_http"], true); | ||
| assert!( | ||
| serde_json::from_value::<ProviderConfig>(encoded) | ||
| .unwrap() | ||
| .allow_insecure_http | ||
| ); |
There was a problem hiding this comment.
⛏ nitpick — These are the only unwrap() calls in this file (the base had none), and AGENTS.md asks for expect("should ...") in tests too. Scratch-verified: cargo fmt --check, clippy -D warnings, and the three focused tests pass with this applied.
| let legacy = serde_json::to_value(provider("prebid-server")).unwrap(); | |
| assert!(legacy.get("allow_insecure_http").is_none()); | |
| assert!( | |
| !serde_json::from_value::<ProviderConfig>(legacy) | |
| .unwrap() | |
| .allow_insecure_http | |
| ); | |
| let mut opted_in = provider("prebid-server"); | |
| opted_in.allow_insecure_http = true; | |
| let encoded = serde_json::to_value(opted_in).unwrap(); | |
| assert_eq!(encoded["allow_insecure_http"], true); | |
| assert!( | |
| serde_json::from_value::<ProviderConfig>(encoded) | |
| .unwrap() | |
| .allow_insecure_http | |
| ); | |
| let legacy = serde_json::to_value(provider("prebid-server")) | |
| .expect("should serialize the default provider"); | |
| assert!(legacy.get("allow_insecure_http").is_none()); | |
| assert!( | |
| !serde_json::from_value::<ProviderConfig>(legacy) | |
| .expect("should deserialize the legacy provider shape") | |
| .allow_insecure_http | |
| ); | |
| let mut opted_in = provider("prebid-server"); | |
| opted_in.allow_insecure_http = true; | |
| let encoded = | |
| serde_json::to_value(opted_in).expect("should serialize the opted-in provider"); | |
| assert_eq!(encoded["allow_insecure_http"], true); | |
| assert!( | |
| serde_json::from_value::<ProviderConfig>(encoded) | |
| .expect("should deserialize the opted-in provider") | |
| .allow_insecure_http | |
| ); |
Summary
Draft baseline review unit. Not a current-main merge candidate, deployment approval or completed current-head CI claim.
Baseline and sequence
Head:
1cfba70a2efecda9ff522187c69359eda1fa6692. Base:baseline/protected-geo-20261110.Historical main anchor:
2dd968aeebeb2cc0d5110115f64e0ad58e2e1d13. Original demo: TS2423a2ed plus separately preserved dirty inputs. Matching EdgeZero: draft431, exact revision83d7a6296d285e135c4ea70021b19e6c8dca7e9f.The user selected compatible-baseline cleanup. Current-main integration with EdgeZero462 and the authenticated-forwarder incompatibility are deferred. Preserve current-main PR1525/1526 ready and1527/1528 draft, with their existing bases. No excluded1499 or independent1506 implementation, relay/privacy experiment, new transport fallback, group/codec/phase design or operational deployment is included.
Changes
Closes
Refs #1530. This unit completes one part of the baseline extraction and does not close the overall tracker on its own.
Spec
Not a new feature. This is extraction of existing demo behavior and bounded build/caller corrections. Original native and EC storage design/review records remain in the baseline where applicable; they are not presented as approval of later combined source. No new forwarding, consent, timer or provider policy is designed here.
Test plan
Focused auction-plan tests and strict Core lint pass. Fixture uses pbs.example.com, without changing opt-in assertions. Current-main PR1525 remains separate and ready; its hosted results are not relabeled as this baseline head's results.
The b67ffc6 baseline checkpoint has passing documented local gates: Fastly/default/reuse/native/Cloudflare/Spin suites, all adapter strict lint lanes, 17 cross-adapter parity cases, 1252 JS cases, CLI suites, formatting, clock/digest/proposal checks and changed-lines domain lint. Passing checks at121ebcc36 are reused only for unchanged relevant inputs; the final follow-up covers the CLI test-import and documentation-only corrections. These results are not a substitute for this PR's current-head hosted checks.
Checklist
Operational Terraform remains in trusted-server-deployer. Local checks and prototype tests do not qualify cloud access, fleet durability or a live publisher.
Hosted docs-build correction
The first hosted format-docs job caught an additional site-build gate that the original local formatter checkpoint had not run: a relative link from the native EC guide to a Terraform example outside the site. Native EC correction8b11dbad3 replaces it with the plain repository path; no dead-link check is disabled. That docs-only correction is carried through the existing upper PR identities. Native EC and final integration8985b111 both pass docs ESLint and VitePress build; the final docs formatter also passes. Runtime, dependency, image-build-context and test inputs are unchanged from b67ffc6, so their prior exact-input proofs are reused. Hosted current-head checks still control readiness.
Startup negative-probe correction
A hosted native EC case failed the no-listener probe while its child logged the expected startup validation failure. The old log has no socket endpoints, so the failure's exact subtype is not claimed confirmed. A separate controlled Linux reproduction demonstrates a successful TCP self-connection with no listening server. Native correction8490c691d distinguishes equal local/peer endpoints from a real listener; actual socket controls prove the self-connection is excluded and a real listener is still detected. Nonzero exit, expected diagnostic, secret-redaction and the original startup deadline remain required. This changes only the acceptance fixture, not production admission, transport or trust.
The earliest native unit and final9356c557 default/all-feature acceptance checks and strict lint pass, including both controls and the original failure case. Ignores remain explicit; earlier separately executed celld proof is not relabeled as a new ignored-case run. Correction is merged upward through existing PR identities without force-pushing or retargeting. Current-head hosted checks must pass; the old failure is retained rather than rerun-only waived. Runtime/API/dependency inputs remain unchanged. A fresh exact9356 producer build and real runtime smoke now pass: image
sha256:65a735d38e9dbc0573d0f6068a8b261bd202c43aea05085c2b2d6bccc91ebdbd, linux/amd64 UID/GID10001, including startup rejection and in-flight SIGTERM drain. Prior b67 image receipts remain separate; no image was pushed. All local parent runners are complete.Browser CI host compatibility
Preserved older browser failures show Playwright1.58.2 rejecting ubuntu-latest's new Ubuntu26.04 image before browser installation; later missing-executable failures are consequences, not passing browser tests. Nativeb1216763a pins only the browser job to ubuntu-24.04, leaving dependency versions, tests and assertions unchanged. Direct Actionlint1.7.12 passes at the owning native and final integration points; the first shim-only attempt could not run because no global version was configured, and is not counted as lint proof. No global tooling was changed. The only9356 ->ee7 delta is this workflow host/comment; runtime, Cargo, docs, tests and Docker-context inputs are unchanged. Actual browser execution on the supported hosted runner remains pending and must pass before readiness. No waiver or browser skip.