fix: allow admin API key on /internal/* from non-loopback clients

[blackdeathdrow]

Closes #2109.

The loopback check in Server::authenticate_request() ran before the bearer token was extracted, so internal endpoints (/internal/config, /internal/shutdown, etc.) always returned 403 for any non-loopback client regardless of a valid LEMONADE_ADMIN_API_KEY.

Move auth_token extraction before the loopback check and allow the request through when the client presents a valid admin API key. A non-loopback request that does not present a valid admin key is rejected with 403 (not 401) — the response body states both options ("from localhost or with admin API key"). Loopback behavior is unchanged.

Changes

• src/cpp/server/server.cpp: Hoist httplib::get_bearer_token_auth(req) above the loopback check in authenticate_request(). The non-loopback rejection now checks for a valid admin API key before returning 403. Updated the error message to reflect both options.

Tests

• test/server_env_vars.py: Added TestInternalAdminKeyNonLoopback — binds the server to 0.0.0.0 and reaches it via a non-loopback IPv4 address discovered at runtime. Skips via unittest.SkipTest when no non-loopback IPv4 is available. Covers:

  • non-loopback + valid admin key → 200
  • non-loopback + no key → 403
  • non-loopback + invalid key → 403
  • loopback + valid admin key → 200 (unchanged)
  • loopback + no key → 401 (unchanged)

  All 5 tests pass locally; full test/server_env_vars.py suite (40 tests) still passes.

[fl0rianr]

Thanks for the focused fix — moving bearer-token extraction before the /internal/* loopback guard makes sense and matches the intended “admin key can access internal endpoints” model.

Before merging, I think this should have a regression test for the actual failing case. The existing admin-key tests appear to exercise /internal/config via localhost, so they do not cover the non-loopback path that previously returned 403 before the admin key was checked. Could you add coverage for a server bound to a non-loopback interface, e.g. LEMONADE_HOST=0.0.0.0, and verify that:

• /internal/config from a non-loopback client succeeds with a valid admin bearer token
• the same request without/with an invalid token is rejected
• existing loopback behavior remains unchanged

One semantic detail worth clarifying: the issue’s expected behavior says non-loopback /internal/* without a valid admin key should be rejected with 401, but this implementation still returns 403 from the loopback/admin-key gate before reaching the later admin-key check. If 403 is intentional for “non-loopback and not admin-authorized”, that’s fine, but it would be good to make that explicit in the PR/issue and test expectations.

[jeremyfowers]

Similar to #2100

Do people have a preference about which implementation? @fl0rianr ?

[fl0rianr]

I think I slightly prefer the #2100 direction if the intended product behavior is to support the many-clients-one-server topology.

#2110 is the narrower bugfix: non-loopback /internal/* is allowed only with a valid admin key. That’s conservative and directly addresses #2109.

#2100 is the cleaner policy change: internal endpoints can be reached remotely, key enforcement handles access control when a key is configured, and unsecured non-loopback binds get a clear warning/docs update. I like that better if we’re comfortable with the existing “no key = unauthenticated” model applying to internal endpoints too.

If we go with #2100 and close this one, it would be good to preserve credit for this fix as well — maybe add @blackdeathdrow as a co-author there, if appropriate.

Either way, I think the important part is making the auth/status-code matrix explicit so future changes don’t reintroduce ambiguity around loopback vs admin-key behavior.

[blackdeathdrow]

Thank you for the review. I used OpenCode to create and run the test in Docker. I hope this will be useful.

[bitgamma]

considering we have a bunch of security-related PR open, maybe the more conservative approach of this PR is more in line with the rest, but I am too fine with either implementation.