gofr-dev / gofr-dev/gofr

CORS: invalid ACCESS_CONTROL_MAX_AGE is passed through unvalidated with no warning

Open
#3,941 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
20.9k
Forks
1.8k
Avg merge
5d 18h
Merged PRs (30d)
39

Description

### What

`middleware.GetConfigs` copies `ACCESS_CONTROL_MAX_AGE` into the CORS header map with only a non-empty check ([`pkg/gofr/http/middleware/config.go#L29-L42`](https://github.com/gofr-dev/gofr/blob/development/pkg/gofr/http/middleware/config.go#L29-L42)), and `setMiddlewareHeaders` writes it to the response verbatim via the "additional custom headers" loop ([`pkg/gofr/http/middleware/cors.go#L69-L74`](https://github.com/gofr-dev/gofr/blob/development/pkg/gofr/http/middleware/cors.go#L69-L74)).

A value like `10m`, `600s`, `-1` or `abc` therefore produces a malformed `Access-Control-Max-Age` response header.

### Impact

Browsers discard the malformed header and fall back to their own default preflight cache (5s in Chrome), so the preflight caching the user configured silently never happens. Nothing in the logs indicates the misconfiguration — the app starts clean and the header looks present if you only eyeball the response.

The same class of gap applies to `ACCESS_CONTROL_ALLOW_CREDENTIALS`, where any non-boolean string is passed through unchecked.

### To reproduce

```bash
ACCESS_CONTROL_ALLOW_ORIGIN=* ACCESS_CONTROL_MAX_AGE=10m go run ./main.go
curl -i -X OPTIONS localhost:8000/some-route
# Access-Control-Max-Age: 10m <- invalid, silently ignored by the browser
```

### Expected

Validate the value at startup:

- parse `ACCESS_CONTROL_MAX_AGE` as a non-negative integer (seconds);
- on failure, log a warning naming the config key and the offending value, and drop the header rather than emitting an invalid one;
- consider the same treatment for `ACCESS_CONTROL_ALLOW_CREDENTIALS` (should parse as a bool).

Not proposing a default value for `ACCESS_CONTROL_MAX_AGE` — omitting the header and letting the browser apply its own default is reasonable, and picking a framework-wide TTL would be a surprising behaviour change on upgrade. This issue is only about failing loud on an invalid value.

### Note for the implementer

`GetConfigs(c config.Config)` currently has no logger available. The call site in [`pkg/gofr/factory.go#L31`](https://github.com/gofr-dev/gofr/blob/development/pkg/gofr/factory.go#L31) does have `app.container`, so the fix either threads a logger into `GetConfigs` or returns the validation problems for the caller to log.

Contributor guide

Open the contributing guide

Research direction

Start with pkg/gofr/http/middleware/config.go and cors.go to trace how ACCESS_CONTROL_MAX_AGE and ACCESS_CONTROL_ALLOW_CREDENTIALS reach response headers, then inspect the call site in pkg/gofr/factory.go. Done means invalid values are identified at startup, warnings name the key and offending value, and invalid headers are omitted without introducing a default max-age.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.