feat: CORS implementation#159
Merged
Merged
Conversation
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. WalkthroughAdds optional CORS middleware support for browser-direct client connections to the Olla API server. Includes configuration contracts with validation, ChangesCORS Middleware and Configuration
Sequence Diagram(s)sequenceDiagram
participant Client as Browser Client
participant HTTP as HTTP Service
participant CORS as CORS Middleware
participant Handler as Route Handler
Client->>HTTP: OPTIONS /api/models (preflight)
HTTP->>CORS: wrapped mux
CORS->>CORS: check origin allowed
CORS-->>Client: 204 No Content + Access-Control-* headers
Client->>HTTP: GET /api/models (with Origin header)
HTTP->>CORS: wrapped mux
CORS->>Handler: forward request
Handler-->>CORS: response + X-Olla-Model header
CORS->>CORS: add Access-Control-Allow-Origin<br/>add Access-Control-Expose-Headers
CORS-->>Client: response + CORS headers
Client->>HTTP: GET /api/models (no Origin header)
HTTP->>CORS: wrapped mux
CORS->>Handler: forward request
Handler-->>CORS: response
CORS-->>Client: response (no CORS headers added)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/content/configuration/overview.md`:
- Around line 150-158: Replace the fenced YAML code block with an indented code
block to satisfy MD046: remove the triple-backtick fence and indent each line of
the YAML snippet (starting with "server:" and including nested "cors:",
"enabled:", "allowed_origins:", etc.) by at least four spaces so the YAML
remains formatted as an indented code block within
docs/content/configuration/overview.md.
In `@internal/config/types.go`:
- Around line 75-100: The CorsConfig.Validate method correctly enforces that
Enabled CORS cannot have an empty AllowedOrigins (to match rs/cors treating
nil/empty as allow-all) and checks MaxAge and AllowCredentials vs
AllowedOrigins; keep these guards but add a short note to project docs and CI:
update README or docs to state the minimum supported Go version (matching
dependencies like rs/cors v1.11.1 which declares go 1.23) and either add CI jobs
to run tests/build on Go 1.24 or explicitly document that Go 1.23 is the minimum
supported runtime; reference CorsConfig.Validate, AllowedOrigins,
AllowCredentials, and MaxAge when adding the documentation/CI change so
reviewers can verify the compatibility rationale.
🪄 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
Run ID: ffe38626-58f9-41f9-adc3-068bff46761f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
CLAUDE.mdconfig/config.yamldocs/content/api-reference/overview.mddocs/content/configuration/overview.mddocs/content/configuration/practices/security.mddocs/content/configuration/reference.mddocs/content/faq.mddocs/content/index.mddocs/content/integrations/frontend/openwebui.mdgo.modinternal/app/middleware/cors.gointernal/app/middleware/cors_test.gointernal/app/services/http.gointernal/app/services/http_cors_test.gointernal/config/config.gointernal/config/config_test.gointernal/config/types.goreadme.md
Comment on lines
+150
to
+158
| ```yaml | ||
| server: | ||
| cors: | ||
| enabled: true | ||
| allowed_origins: | ||
| - "https://my-dashboard.example.com" | ||
| allow_credentials: true | ||
| max_age: 600 | ||
| ``` |
There was a problem hiding this comment.
Use indented code block style to satisfy markdownlint (MD046).
This fenced YAML block breaks the configured code-block-style rule and may keep docs lint noisy or failing. Convert this block to indented style to match the repository convention.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 150-150: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 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 `@docs/content/configuration/overview.md` around lines 150 - 158, Replace the
fenced YAML code block with an indented code block to satisfy MD046: remove the
triple-backtick fence and indent each line of the YAML snippet (starting with
"server:" and including nested "cors:", "enabled:", "allowed_origins:", etc.) by
at least four spaces so the YAML remains formatted as an indented code block
within docs/content/configuration/overview.md.
Source: Linters/SAST tools
Comment on lines
+75
to
+100
| // Validate checks CORS configuration for spec-violating combinations that would | ||
| // silently break browser clients at runtime rather than failing loudly at startup. | ||
| func (c *CorsConfig) Validate() error { | ||
| if !c.Enabled { | ||
| return nil | ||
| } | ||
| // rs/cors treats an empty AllowedOrigins list as allow-all, which is the | ||
| // opposite of what an operator expects when they write allowed_origins: []. | ||
| // Require an explicit ["*"] to opt into allow-all so the intent is unambiguous. | ||
| if len(c.AllowedOrigins) == 0 { | ||
| return errors.New("cors: allowed_origins must not be empty when cors is enabled; set [\"*\"] to allow all origins explicitly") | ||
| } | ||
| if c.MaxAge < 0 { | ||
| return fmt.Errorf("cors.max_age must be non-negative, got %d", c.MaxAge) | ||
| } | ||
| // The CORS spec forbids Access-Control-Allow-Origin: * when credentials are in play. | ||
| // Browsers will refuse the response, so this is a startup error rather than a warning. | ||
| if c.AllowCredentials { | ||
| for _, origin := range c.AllowedOrigins { | ||
| if origin == "*" { | ||
| return errors.New("cors: allow_credentials=true is incompatible with allowed_origins=[\"*\"]; list explicit origins instead") | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check rs/cors documentation for AllowedOrigins behavior and Go version support
# Verify the current version and its Go compatibility
gh api repos/rs/cors/tags --jq '.[] | select(.name == "v1.11.1") | .name'
# Check go.mod in the library for minimum Go version
gh api repos/rs/cors/contents/go.mod --jq '.content' | base64 -d | grep '^go 'Repository: thushan/olla
Length of output: 74
🌐 Web query:
rs/cors library empty AllowedOrigins behavior
💡 Result:
In the rs/cors Go library, the behavior of an empty AllowedOrigins slice depends on the version being used, though the documented default is to allow all origins [1][2]. Historically, if AllowedOrigins was left empty or nil, the library treated this as a request to allow all origins (equivalent to [""]) [3][4]. However, users should consult the documentation for their specific version. While the configuration field is defined as a []string, modern best practices and the library's official documentation explicitly state that the default value is [""] [1][2]. If you intend to restrict origins, you must explicitly provide a non-empty slice of allowed origins [1]. If you require dynamic validation, you should use the AllowOriginFunc, AllowOriginRequestFunc, or AllowOriginVaryRequestFunc, which take precedence over the AllowedOrigins configuration [1][5][2].
Citations:
- 1: https://github.com/rs/cors/blob/master/README.md
- 2: https://pkg.go.dev/github.com/rs/cors
- 3: https://github.com/rs/cors/blob/abddb2fca308f1d4a4ab6350ba89b3785613f774/cors.go
- 4: https://github.com/rs/cors/blob/bf64c5349c0f7e5e1554921e798e052958e0fe2f/cors.go
- 5: https://github.com/rs/cors/blob/master/cors.go
rs/cors behaviour matches — keep the empty-origins guard; clarify Go 1.24 support
- rs/cors (v1.11.1) treats an empty/nil
AllowedOriginsas allow-all (equivalent to["*"]), so returning an error whencorsis enabled withallowed_origins: []aligns with rs/cors runtime behaviour. - rs/cors v1.11.1 declares
go 1.23.0in itsgo.mod; if Go 1.24 is a supported target, add CI/build coverage (or document the minimum Go version).
🤖 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 `@internal/config/types.go` around lines 75 - 100, The CorsConfig.Validate
method correctly enforces that Enabled CORS cannot have an empty AllowedOrigins
(to match rs/cors treating nil/empty as allow-all) and checks MaxAge and
AllowCredentials vs AllowedOrigins; keep these guards but add a short note to
project docs and CI: update README or docs to state the minimum supported Go
version (matching dependencies like rs/cors v1.11.1 which declares go 1.23) and
either add CI jobs to run tests/build on Go 1.24 or explicitly document that Go
1.23 is the minimum supported runtime; reference CorsConfig.Validate,
AllowedOrigins, AllowCredentials, and MaxAge when adding the documentation/CI
change so reviewers can verify the compatibility rationale.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds CORS support back into Olla via rs/cors. Addresses #156 .
CORS is opt in via:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores