Harden Fireworks provider integration - #8693
Conversation
This squashes the Fireworks branch into one decision record on top of current main. It keeps Fireworks aligned with the existing API-key provider seams across auth, model catalogs, setup, CLI selection, runtime resolution, and the rebased test baselines needed to stay compatible with current upstream behavior. Constraint: Must preserve current main's provider architecture while keeping Fireworks usable end to end Rejected: Keep four-commit branch history | unnecessary review noise after repeated rebases Confidence: high Scope-risk: moderate Reversibility: clean Directive: On future rebases, keep current main behavior and reapply only Fireworks-specific provider deltas Tested: Fireworks provider pytest slice after squash Not-tested: Full pytest suite; known upstream failures remain outside the Fireworks slice
alt-glitch
added
type/security
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the hardening work. Current main has moved Fireworks onto the shared model-provider path, so this needs a focused salvage rather than a direct application.
Problems
providers/base.py:216on current main returns remote catalogidvalues verbatim, andhermes_cli/models.py:2485consumes that generic result for Fireworks. The PR's sanitizer only protects the retired Fireworks-specific control-plane fetch inhermes_cli/models.py, so it would not cover the active path.- The added
FIREWORKS_BASE_URLsetting conflicts with the secret/config split. Commit31152ae108e84325db1d75a2c7f0f49037c866eedeliberately removed it; current docs usemodel.base_urlinconfig.yaml. - The Fire Pass router list resurrects scope from issue #3510, which was closed as not planned; current Fireworks defaults are PAYG
/models/IDs.
Suggested changes
- Move ID validation to the shared
ProviderProfile.fetch_models()boundary and test it throughprovider_model_ids("fireworks"). - Drop the base-URL env var and Fire Pass catalog additions.
Automated hermes-sweeper review.
| @@ -784,6 +792,14 @@ def _ensure_hermes_home_managed(home: Path): | |||
| "category": "provider", | |||
There was a problem hiding this comment.
Please remove this non-secret endpoint override. Current main deliberately removed FIREWORKS_BASE_URL in 31152ae108e84325db1d75a2c7f0f49037c866ee; endpoint overrides belong in model.base_url in config.yaml.
| @@ -68,6 +73,10 @@ def _codex_curated_models() -> list[str]: | |||
| return _add_forward_compat_models(list(DEFAULT_CODEX_MODELS)) | |||
There was a problem hiding this comment.
Please do not add the Fire Pass router catalog here. Issue #3510 was closed as not planned, and current main's Fireworks profile intentionally uses PAYG accounts/fireworks/models/... defaults only.
teknium1
added
sweeper:risk-security-boundary
|
Closing alongside #8632 (same scope, this is the subset) — see the explanation there: these harden the never-merged #3564 Fireworks integration; the plugin-based integration that shipped doesn't contain the code paths being hardened, and the autodetect-priority concern doesn't reproduce on current main. Thanks for the work, and a fresh PR against the current integration is welcome if any gap remains. |
5 participants
Security hardening follow-up for Fireworks provider support.
This patch:
Validation: