You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tooling.apache.org by "andrewmusselman (via GitHub)" <gi...@apache.org> on 2026/05/27 05:31:32 UTC
[I] ISSUE-001b - Remove get_current_user bypass and harden auth paths against accidental dev-stub leakage (tooling-gofannon)
andrewmusselman opened a new issue, #36:
URL: https://github.com/apache/tooling-gofannon/issues/36
## Summary
Issue #19 closed the production-fire portion of ISSUE-001 by adding
`AUTH_CONFIG_PATH` to the Settings class, which restored the bypass
branch to its intended behavior: when no session cookie is present
and no auth config is set, `get_current_user` returns a stub
`{"uid": "local-dev-user", "auth_mode": "dev_stub"}` instead of
crashing.
That stub is still a problem for hosted multi-tenant operation. Any
deployment that omits `AUTH_CONFIG_PATH` (intentionally or
otherwise) silently grants every request access as the same fake
user. The same is true for the `/auth/dev-stub-picker` route, which
renders an HTML page anyone can use to log in as any configured
user if the `dev_stub` provider gets into a production auth config.
This issue tracks the pre-launch hardening needed to fail closed
across the auth surface.
## Details
### What's still wrong
1. **Bypass branch in `get_current_user`** (`routes.py`, ~line 158):
Falls through to a `local-dev-user` stub when neither a session
cookie nor a Firebase token is present and the operator hasn't
set `AUTH_CONFIG_PATH`. Acceptable in dev, dangerous anywhere
else.
2. **`/auth/dev-stub-picker` is not env-gated** (`routes_auth.py`,
~line 258): The provider registry refuses to instantiate
`DevStubProvider` outside `local`/`dev`/`test` environments, but
the picker route itself has no env check. Defense in depth says
guard the route too.
3. **Integration tests rely on the bypass.** Every test in
`tests/integration/test_api_keys.py` and most of
`tests/integration/test_routes.py` uses `TestClient(app)` with
no session cookie, depending on `get_current_user` to return
`local-dev-user` so the test's dependency override
(`get_user_service_dep`) takes effect. Removing the bypass
without updating the tests would fail all 17 with 401.
### Why this matters before launch
A misconfigured deployment (forgot to mount `auth.yaml`, env var
not set, secret rotation gone wrong) currently fails open: the
service keeps serving requests as `local-dev-user`, every request
sees every resource until someone notices.
## Remediation
Three changes that should land together:
### 1. Remove the bypass in `routes.py:get_current_user`
Replace the `APP_ENV != "firebase"` branch so unauthenticated
requests raise `HTTPException(401)` instead of returning a stub.
Firebase fallback remains for legacy deployments that still set
`APP_ENV=firebase`. Drafted patch:
```python
async def get_current_user(request: Request, token: str = Depends(oauth2_scheme)):
"""
Dependency to authenticate a request. Supports two modes:
1. Session cookie (gofannon_sid) — checked first.
2. Legacy Firebase bearer token — fallback when APP_ENV == "firebase".
If neither is present, returns 401. There is no unauthenticated
fallthrough; dev users get a session cookie via the dev_stub
provider's picker page.
"""
sid = request.cookies.get("gofannon_sid")
if sid:
user = await _verify_session_cookie(request, sid)
if user:
return user
raise HTTPException(status_code=401, detail="Session expired or invalid")
if settings.APP_ENV == "firebase":
return await _verify_firebase_token(request, token)
raise HTTPException(
status_code=401,
detail="Not authenticated. Session cookie missing or invalid.",
)
```
### 2. Env-gate `/auth/dev-stub-picker` in `routes_auth.py`
Add an explicit env check at the top of `dev_stub_picker`:
```python
if settings.APP_ENV not in ("local", "dev", "test"):
raise HTTPException(status_code=404)
```
(Imports for `HTTPException` and `settings` may need adding to that
file.)
### 3. Update integration tests to override `get_current_user`
Every integration test that hits an auth-gated endpoint needs to
add an override for `get_current_user` alongside its existing
`get_user_service_dep` override:
```python
from routes import get_current_user
app.dependency_overrides[get_current_user] = lambda: {
"uid": "local-dev-user",
"auth_mode": "test",
}
app.dependency_overrides[get_user_service_dep] = lambda: FakeUserService()
```
Affected files (per current count): `tests/integration/test_api_keys.py`
(12 tests), `tests/integration/test_routes.py` (3 of 5 tests).
Probably worth a shared `authed_client` fixture in
`tests/integration/conftest.py` rather than repeating the override
in every test.
## Acceptance Criteria
- [ ] `get_current_user` bypass branch removed; unauthenticated
requests raise `HTTPException(401)` regardless of `APP_ENV`
- [ ] `/auth/dev-stub-picker` returns 404 when `APP_ENV` is not in
`{local, dev, test}`
- [ ] Integration tests in `tests/integration/test_api_keys.py` and
`tests/integration/test_routes.py` updated to override
`get_current_user`; all currently-passing tests still pass
- [ ] New test: unauthenticated `GET /users/me` returns 401 (not a
200 stub response) regardless of `APP_ENV`
- [ ] New test: `GET /auth/dev-stub-picker?...` returns 404 when
`APP_ENV=production`
- [ ] New test: expired session cookie returns 401 with a body that
includes "Session expired or invalid" (already true, but
worth locking in)
- [ ] E2E suite still passes against the updated backend (storage
state from `global-setup.js` provides a valid cookie, so the
removal shouldn't affect E2E)
## References
- File: `webapp/packages/api/user-service/routes.py:158`
- File: `webapp/packages/api/user-service/routes_auth.py:258`
- File: `webapp/packages/api/user-service/tests/integration/test_api_keys.py`
- File: `webapp/packages/api/user-service/tests/integration/test_routes.py`
- Predecessor: #19 (closed by commit defining `AUTH_CONFIG_PATH`)
- Tracker: FIXES.md item #1
## Priority
High — pre-launch blocker. Not a production fire today (the
deployment has `AUTH_CONFIG_PATH` set so the bypass doesn't fire),
but a misconfiguration that would otherwise be invisible would
silently fail open. Hosted multi-tenant launch should not ship
with this code path reachable. Estimated effort: ~50 LOC across
three files plus the test refactor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org
Re: [I] ISSUE-001b - Remove get_current_user bypass and harden auth paths against accidental dev-stub leakage (tooling-gofannon)
Posted by "andrewmusselman (via GitHub)" <gi...@apache.org>.
andrewmusselman closed issue #36: ISSUE-001b - Remove get_current_user bypass and harden auth paths against accidental dev-stub leakage
URL: https://github.com/apache/tooling-gofannon/issues/36
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tooling.apache.org
For additional commands, e-mail: dev-help@tooling.apache.org