fix(cors): validate ACCESS_CONTROL_MAX_AGE and ALLOW_CREDENTIALS at startup (#3941) - #4052

Thanks for this — direction's right and the max-age half works well. Verified locally: with ACCESS_CONTROL_MAX_AGE=abc it now warns and drops the header instead of sending garbage.

Two things before merge:

1. The credentials check is too loose. strconv.ParseBool accepts 1, t, True, TRUE — but browsers honor only the exact string true. Since we store the value verbatim, ACCESS_CONTROL_ALLOW_CREDENTIALS=1 passes validation and goes out as Access-Control-Allow-Credentials: 1, which the browser silently discards — the exact failure this PR is meant to catch. Confirmed by booting examples/http-server with =1: no warning, header sent as-is. Please tighten to val == "true" and add a case like "1" or "TRUE" to the invalid-value test (right now it only covers "yes").

2. Changing the exported GetConfigs signature is a breaking API change — external callers of middleware.GetConfigs won't compile. Can we make the logger variadic (logger ...configLogger) so existing callers keep working?

Minor, not blockers: Atoi also accepts +600 / 0600 for max-age, and =false is emitted as : false (harmless but pointless — could just omit).

The credentials and max-age checks look right, and making GetConfigs variadic keeps it backward-compatible — nice call. Just lint left: goconst on the repeated "true" (it only counts config.go, tests are excluded). Two suggestions below and it's green.

LGTM. Verified locally at af66a7b: canonical Max-Age check correctly rejects +600/0600/-1/10m/600s and accepts 0/600, credentials matches only literal true (false omitted without a warning), nil/absent logger is panic-safe, and the variadic keeps GetConfigs backward-compatible (only factory.go calls it, updated here). Mutation-tested the guard to confirm the tests aren't vacuous, ran the http-server example with bad values and saw both WARN logs at startup + both headers dropped on the wire while a valid Allow-Origin passed through. gofmt/vet/build/golangci all clean.

Two nits, neither blocking.

Approving. I ran this against a live server rather than reading the diff, and both halves behave exactly as described.

The bug reproduces on the wire. Booting the same app with ACCESS_CONTROL_MAX_AGE=10m and ACCESS_CONTROL_ALLOW_CREDENTIALS=1, then sending a real preflight:

Both value tables, checked against a running server rather than the unit tests:

ACCESS_CONTROL_MAX_AGE — 600 and 0 are emitted with no warning; -1, +600, 0600, 10m, 600s, abc and a value that overflows int are all dropped and warned. The canonical-form check earns its keep: +600 and 0600 do parse with Atoi and would have gone out in a form the Fetch standard does not define.

ACCESS_CONTROL_ALLOW_CREDENTIALS — true is emitted; false is dropped silently, which is right since an absent header and false mean the same thing to a browser; TRUE, True, 1, t, yes and 0 are dropped and warned.

On the security question you raised in the description — the concern was that tightening this might start honouring credentials for configurations that are effectively disabled today. It does not, and that is worth stating plainly since it was the one thing that could have made this risky: 1 and TRUE were already discarded by the browser, so credentials were off before and are off after. The only change is that the operator now finds out. Reading the final code, you did take that call and landed on the literal true, which is the correct one, but the PR description still says you left it open — worth a quick edit so the description matches what merged.

The variadic is genuinely non-breaking, and I checked it from outside the package rather than assuming. A caller compiled against GetConfigs(c) still builds, and GetConfigs(c, myLogger{}) also builds even though configLogger is unexported, since Go matches the argument structurally. Only factory.go calls it in-repo. I also confirmed there is no coupling on the dropped entry: setMiddlewareHeaders writes the fixed headers verbatim and branches on nothing, so omitting the credentials header has no knock-on effect on origin handling.

One nit, not blocking and not a CI problem. A full golangci-lint run over the package now reports goconst for ACCESS_CONTROL_ALLOW_ORIGIN: development has two occurrences, and the new case at config_test.go:133 makes it three against a threshold of three. CI will not catch it and should not fail — the workflow lints with only-new-issues: true, the finding anchors to a line this PR does not touch, and running the equivalent --new-from-rev=origin/development gives zero issues. Since you already added keyAccessControlMaxAge and keyAccessControlAllowCredentials, folding the origin key into the same set would keep a full-repo lint quiet and the constants consistent. Fine to leave.

Worth flagging separately that only the Snyk check has run on this head — no Go CI has executed here, so the results above are from running it locally.

The docs change matches the implemented behaviour, including the false case. Good fix.

Withdrawing my approval — I checked the -1 case against the browser implementation after approving, and it turns out to be doing something deliberate that this PR silently takes away. Everything else in my earlier review stands.

-1 is a working idiom, not a malformed value. From Chromium's services/network/cors/preflight_result.cc:

base::TimeDelta ParseAccessControlMaxAge(const std::optional<std::string>& max_age) {
  if (!max_age)                                 return kDefaultTimeout;    // absent -> 5s
  int64_t seconds;
  if (!base::StringToInt64(*max_age, &seconds)) return kDefaultTimeout;    // garbage -> 5s
  if (seconds < 0)                              return base::TimeDelta();  // -1 -> 0, never cached
  if (seconds >= kMaxTimeout.InSeconds())       return kMaxTimeout;        // cap 2h
  return base::Seconds(seconds);
}

A negative value returns a zero duration, which means the preflight result is not cached at all. So ACCESS_CONTROL_MAX_AGE=-1 is how you say "preflight every request", and people do use it that way while a CORS policy is in flux.

Dropping the header does not preserve that. An absent header takes the first branch and Chromium applies its 5 second default, so the effect of this PR on a -1 configuration is:

That is a behaviour change in the opposite direction from the one this PR is written to prevent, and the warning tells the operator they misconfigured something they had actually set on purpose.

The fix is small, because the PR already accepts the right answer. Reading the same function, 0 and -1 are equivalent: seconds == 0 falls through to base::Seconds(0), which is the same zero duration as the negative branch returns. 0 is also spec-valid — MDN defines the value as an unsigned non-negative integer — and this PR already accepts it, as I confirmed on the wire.

So there is no need to start accepting -1. What is needed is that the operator is told where to go. Two options, either is fine:

• Keep rejecting -1, and special-case the message for negative values to name 0 as the spec-valid way to disable caching, rather than the generic "expected a non-negative number of seconds".
• Or accept -1 and normalise it to 0 on the way out, which preserves intent without emitting an off-spec value.

Whichever you pick, the -1 behaviour is worth a line in docs/advanced-guide/middlewares/page.md next to the note this PR already adds, since the docs now tell people the value must be non-negative without saying what to use instead.

None of this touches the credentials half, which is exactly right and which I would rather not hold up. Worth recording that it matches the normative guidance word for word — MDN's directives section for Access-Control-Allow-Credentials reads "This is the only valid value for this header and is case-sensitive. If you don't need credentials, omit this header entirely rather than setting its value to false." This PR does both of those things independently, including the silent omission for false, so that half needs no changes at all.

The rest of my earlier review is unchanged: the malformed cases are handled correctly, the tests are not vacuous, the variadic keeps GetConfigs source-compatible for external callers, and coverage is up. Happy to re-approve as soon as the -1 path is settled.