feat: implement HTTP allowed hosts/origins checking#49
Conversation
hugodutka
force-pushed
the
hugodutka/allowed-hosts
branch
3 times, most recently
from
87cefa1 to
85834d5
Compare
hugodutka
changed the title
hugodutka
force-pushed
the
hugodutka/allowed-hosts
branch
from
85834d5 to
e2c72e6
Compare
hugodutka
force-pushed
the
hugodutka/allowed-hosts
branch
from
e2c72e6 to
f1b18a2
Compare
hugodutka
changed the title
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
As far as I can tell, --allowed-hosts currently performs strict matching of both the host and port. This is unintuitive and as far as I can tell other frameworks match just on the hostname and not the port. For example, Rails' ActionDispatch::HostAuthorization middleware appears to strip the port before matching the host header.
hugodutka
force-pushed
the
hugodutka/allowed-hosts
branch
from
8f2d898 to
d2400b9
Compare
cmd/server/server_test.go
Outdated
| {"chat-base-path default", FlagChatBasePath, "/chat", func() any { return viper.GetString(FlagChatBasePath) }}, | ||
| {"term-width default", FlagTermWidth, uint16(80), func() any { return viper.GetUint16(FlagTermWidth) }}, | ||
| {"term-height default", FlagTermHeight, uint16(1000), func() any { return viper.GetUint16(FlagTermHeight) }}, | ||
| {"allowed-hosts default", FlagAllowedHosts, []string{"localhost"}, func() any { return viper.GetStringSlice(FlagAllowedHosts) }}, |
There was a problem hiding this comment.
Should we also allow 127.0.0.1 and [::1] by default?
cmd/server/server.go
Outdated
| if len(input) == 0 { | ||
| return fmt.Errorf("the list must not be empty") | ||
| } | ||
| for _, item := range input { | ||
| for _, r := range item { | ||
| if unicode.IsSpace(r) { | ||
| return fmt.Errorf("'%s' contains whitespace characters, which are not allowed", item) | ||
| } | ||
| } | ||
| if strings.Contains(item, ",") { | ||
| return fmt.Errorf("'%s' contains comma characters, which are not allowed", item) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Why do we validate AllowedOrigins in the HTTP layer but AllowedHosts here?
There was a problem hiding this comment.
We validate both in both the HTTP layer and the cmd layer. Originally because I wanted to keep cmd-specific validation, like whitespace and comma detection, to the cmd layer. But it did lead to a bunch of duplication, which I'm not happy about. I'll refactor to keep validation in a single place.
johnstcn
left a comment
johnstcn
left a comment
There was a problem hiding this comment.
I don't have any further blocking comments.
Since coder/agentapi#49 was merged, agentapi by default only accepts requests with the `Host` header set to localhost, 127.0.0.1, or [::1]. In Coder, agentapi is served behind a reverse proxy as a workspace app, so we need to use a wildcard `AGENTAPI_ALLOWED_HOSTS` for agentapi-based modules to continue working. This PR updates the claude code and agentapi modules, and a subsequent PR will update modules that are based on the agentapi module.
3 participants
This PR adds 2 new CLI flags:
--allowed-hostsand--allowed-origins. They control what kinds ofHostandOriginheaders the server accepts.