feat: add schema_url/BIFROST_SCHEMA_URL support for mirrored schema locations in isolated deployments - #4614
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
📝 WalkthroughWalkthroughAdds configurable ChangesConfigurable schema location
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4f3f4cb to
d47aca4
Compare
1055b8e to
87c11b8
Compare
87c11b8 to
7653b19
Compare
0b20972 to
8c7f9db
Compare
7653b19 to
f81a020
Compare
f81a020 to
7c1c332
Compare
8c7f9db to
5e42852
Compare
9c8cc92 to
eaea90d
Compare
eaea90d to
2e6e974
Compare
f3bf8fb to
722067a
Compare
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "feat: Allow custom schema URL overrides ..." | Re-trigger Greptile |
ab9906f to
08f6ba2
Compare
08f6ba2 to
15ef1ea
Compare
15ef1ea to
4f5dd0d
Compare
Merge activity
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@transports/bifrost-http/lib/validator.go`:
- Around line 32-46: Cap the HTTP schema response body in loadSchemaFromLocation
before reading it. Keep the existing timeout and status checks, but wrap
resp.Body with a size limit and return an error if the schema exceeds that bound
instead of calling io.ReadAll directly. Apply this only in the http/https branch
of loadSchemaFromLocation, preserving the local file path behavior via
filePathFromSchemaLocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93e497ca-f2f6-4ef4-808b-5176178a21e5
📒 Files selected for processing (18)
docs/deployment-guides/config-json/schema-reference.mdxhelm-charts/bifrost/Chart.yamlhelm-charts/bifrost/README.mdhelm-charts/bifrost/templates/_helpers.tplhelm-charts/bifrost/templates/deployment.yamlhelm-charts/bifrost/templates/stateful.yamlhelm-charts/bifrost/values.schema.jsonhelm-charts/bifrost/values.yamlterraform/modules/bifrost/README.mdterraform/modules/bifrost/main.tfterraform/modules/bifrost/tests/config_merging.tftest.hclterraform/modules/bifrost/tests/setup/main.tfterraform/modules/bifrost/tests/setup/variables.tfterraform/modules/bifrost/variables.tftransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/validator.gotransports/bifrost-http/lib/validator_test.gotransports/config.schema.json
| func loadSchemaFromLocation(location string) ([]byte, error) { | ||
| if strings.HasPrefix(location, "http://") || strings.HasPrefix(location, "https://") { | ||
| client := http.Client{Timeout: schemaFetchTimeout} | ||
| resp, err := client.Get(location) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { | ||
| return nil, fmt.Errorf("unexpected HTTP status %d fetching schema", resp.StatusCode) | ||
| } | ||
| return io.ReadAll(resp.Body) | ||
| } | ||
| return os.ReadFile(filePathFromSchemaLocation(location)) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Cap the HTTP schema response size.
loadSchemaFromLocation sets a fetch timeout but never bounds the response body size before io.ReadAll(resp.Body). A misconfigured or malicious schema location (env var, or $schema sourced from config data) can return an arbitrarily large body, exhausting memory.
🛡️ Proposed fix to cap the response size
- defer resp.Body.Close()
- if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
- return nil, fmt.Errorf("unexpected HTTP status %d fetching schema", resp.StatusCode)
- }
- return io.ReadAll(resp.Body)
+ defer resp.Body.Close()
+ if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
+ return nil, fmt.Errorf("unexpected HTTP status %d fetching schema", resp.StatusCode)
+ }
+ const maxSchemaBytes = 1 << 20 // 1MB
+ return io.ReadAll(io.LimitReader(resp.Body, maxSchemaBytes+1))As per path instructions, "enforce timeouts and size limits" for untrusted input handling in Go files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func loadSchemaFromLocation(location string) ([]byte, error) { | |
| if strings.HasPrefix(location, "http://") || strings.HasPrefix(location, "https://") { | |
| client := http.Client{Timeout: schemaFetchTimeout} | |
| resp, err := client.Get(location) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { | |
| return nil, fmt.Errorf("unexpected HTTP status %d fetching schema", resp.StatusCode) | |
| } | |
| return io.ReadAll(resp.Body) | |
| } | |
| return os.ReadFile(filePathFromSchemaLocation(location)) | |
| } | |
| func loadSchemaFromLocation(location string) ([]byte, error) { | |
| if strings.HasPrefix(location, "http://") || strings.HasPrefix(location, "https://") { | |
| client := http.Client{Timeout: schemaFetchTimeout} | |
| resp, err := client.Get(location) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { | |
| return nil, fmt.Errorf("unexpected HTTP status %d fetching schema", resp.StatusCode) | |
| } | |
| const maxSchemaBytes = 1 << 20 // 1MB | |
| return io.ReadAll(io.LimitReader(resp.Body, maxSchemaBytes+1)) | |
| } | |
| return os.ReadFile(filePathFromSchemaLocation(location)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transports/bifrost-http/lib/validator.go` around lines 32 - 46, Cap the HTTP
schema response body in loadSchemaFromLocation before reading it. Keep the
existing timeout and status checks, but wrap resp.Body with a size limit and
return an error if the schema exceeds that bound instead of calling io.ReadAll
directly. Apply this only in the http/https branch of loadSchemaFromLocation,
preserving the local file path behavior via filePathFromSchemaLocation.
Source: Path instructions
4f5dd0d to
79ac94a
Compare
…ma locations in isolated deployments (#4614) ## Summary Bifrost deployments in air-gapped or isolated environments cannot reach `https://www.getbifrost.ai/schema` to fetch the JSON schema for `config.json` validation. This PR introduces a `schema_url` / `schemaUrl` override across the Go transport, Helm chart, and Terraform module so that operators can point schema resolution at a mirrored HTTP(S) URL, a `file://` URL, or a local filesystem path instead of the hardcoded public URL. ## Changes - **Go transport (`validator.go`, `config.go`):** Replaced the hardcoded `https://www.getbifrost.ai/schema` fetch with a `loadSchemaFromLocation` helper that handles HTTP(S), `file://`, and filesystem paths. Schema resolution now follows this priority: `BIFROST_SCHEMA_URL` env → `SCHEMA_URL` env (legacy) → `$schema` value in `config.json` (if not the default URL) → local source-checkout candidate → default public URL. The startup warning for a missing `$schema` field now checks for an empty/absent value rather than an exact URL match. - **`config.schema.json`:** Removed the `const` constraint on `$schema` (which would have rejected any non-default value) and replaced it with a `default`, allowing mirrored locations to pass schema validation. - **Helm chart:** Added `bifrost.schemaUrl` to `values.yaml` and `values.schema.json`. The value is written into the generated `config.json` `$schema` field via `_helpers.tpl` and exported as `BIFROST_SCHEMA_URL` in both `deployment.yaml` and `stateful.yaml`. - **Terraform module:** Added a `schema_url` variable (defaulting to the public URL) that is injected into the `$schema` field of the generated `config.json`. A new `schema_url_override` test case validates that a custom value is correctly written through. - **Docs:** Updated the schema reference page and Helm/Terraform READMEs to document the new override options. ## Type of change - [ ] Bug fix - [x] Feature - [ ] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [ ] Plugins - [ ] UI (React) - [x] Docs ## How to test ```sh # Unit tests for the validator and schema loading logic cd transports/bifrost-http go test ./lib/... -run TestValidateConfigSchema # Test filesystem path override via env BIFROST_SCHEMA_URL=/path/to/config.schema.json go test ./lib/... -run TestValidateConfigSchema_CustomSchemaFilePath # Test legacy env var SCHEMA_URL=/path/to/config.schema.json go test ./lib/... -run TestValidateConfigSchema_LegacySchemaURLEnv # Helm: render and inspect the generated config.json $schema field helm template bifrost ./helm-charts/bifrost \ --set bifrost.schemaUrl="https://schema.internal/bifrost" \ | grep '\$schema' # Terraform: run the config merging tests cd terraform/modules/bifrost terraform test -filter=config_merging.tftest.hcl ``` **New environment variables / Helm values / Terraform variables:** | Surface | Name | Default | |---|---|---| | Go / Docker env | `BIFROST_SCHEMA_URL` | `https://www.getbifrost.ai/schema` | | Go / Docker env (legacy) | `SCHEMA_URL` | — | | Helm | `bifrost.schemaUrl` | `https://www.getbifrost.ai/schema` | | Terraform | `schema_url` | `https://www.getbifrost.ai/schema` | All accept an HTTP(S) URL, `file://` URL, or filesystem path. ## Breaking changes - [x] No The `const` constraint on `$schema` in `config.schema.json` has been relaxed to a `default`. Existing configs using the public URL continue to validate correctly. Configs that previously set a non-default `$schema` value would have failed schema validation before this change, so removing the constraint is strictly additive. ## Security considerations Schema content loaded from a `file://` or filesystem path is read from the local filesystem using the process's existing permissions — no new network surface is introduced for isolated deployments. Operators using HTTP(S) mirrors should ensure the mirror endpoint is trusted, as the fetched schema is used to validate the entire `config.json`. ## Checklist - [ ] I read `docs/contributing/README.md` and followed the guidelines - [x] I added/updated tests where appropriate - [x] I updated documentation where needed - [ ] I verified builds succeed (Go and UI) - [ ] I verified the CI pipeline passes locally if applicable

Summary
Bifrost deployments in air-gapped or isolated environments cannot reach
https://www.getbifrost.ai/schemato fetch the JSON schema forconfig.jsonvalidation. This PR introduces aschema_url/schemaUrloverride across the Go transport, Helm chart, and Terraform module so that operators can point schema resolution at a mirrored HTTP(S) URL, afile://URL, or a local filesystem path instead of the hardcoded public URL.Changes
validator.go,config.go): Replaced the hardcodedhttps://www.getbifrost.ai/schemafetch with aloadSchemaFromLocationhelper that handles HTTP(S),file://, and filesystem paths. Schema resolution now follows this priority:BIFROST_SCHEMA_URLenv →SCHEMA_URLenv (legacy) →$schemavalue inconfig.json(if not the default URL) → local source-checkout candidate → default public URL. The startup warning for a missing$schemafield now checks for an empty/absent value rather than an exact URL match.config.schema.json: Removed theconstconstraint on$schema(which would have rejected any non-default value) and replaced it with adefault, allowing mirrored locations to pass schema validation.bifrost.schemaUrltovalues.yamlandvalues.schema.json. The value is written into the generatedconfig.json$schemafield via_helpers.tpland exported asBIFROST_SCHEMA_URLin bothdeployment.yamlandstateful.yaml.schema_urlvariable (defaulting to the public URL) that is injected into the$schemafield of the generatedconfig.json. A newschema_url_overridetest case validates that a custom value is correctly written through.Type of change
Affected areas
How to test
New environment variables / Helm values / Terraform variables:
BIFROST_SCHEMA_URLhttps://www.getbifrost.ai/schemaSCHEMA_URLbifrost.schemaUrlhttps://www.getbifrost.ai/schemaschema_urlhttps://www.getbifrost.ai/schemaAll accept an HTTP(S) URL,
file://URL, or filesystem path.Breaking changes
The
constconstraint on$schemainconfig.schema.jsonhas been relaxed to adefault. Existing configs using the public URL continue to validate correctly. Configs that previously set a non-default$schemavalue would have failed schema validation before this change, so removing the constraint is strictly additive.Security considerations
Schema content loaded from a
file://or filesystem path is read from the local filesystem using the process's existing permissions — no new network surface is introduced for isolated deployments. Operators using HTTP(S) mirrors should ensure the mirror endpoint is trusted, as the fetched schema is used to validate the entireconfig.json.Checklist
docs/contributing/README.mdand followed the guidelines