Repository navigation
Feature/google sso auth - #350
Conversation
- Add optional remember param to AuthContext.login for SignInPanel/App.tsx compatibility (was silently dropped, breaking the Remember me checkbox) - Guard AuthContext.logout against duplicate invocation via an in-flight promise ref - Remove AccountPage's redundant direct apiLogout call, fixing a duplicate POST /v1/auth/logout (one 204, one 401) found during manual testing - Rewrite AccountPage tests to match actual Name/Email/Organization fallback rendering instead of an unused, commented-out fallback chain - Rewrite AuthContext and GoogleCallbackPage test suites for the cookie-based contract (completeOAuthLogin, no token storage) replacing obsolete loginWithAccessToken/localStorage assumptions - Fix stale AuthContextValue mock shape (loginWithAccessToken -> completeOAuthLogin) in ContactAdminPage, SignInPanel, and LoginPage tests - Widen SignupFormPanel's onFormChange prop type to keyof SignUpFormData, fixing a pre-existing type mismatch with SignUpPage - Fix client.test.ts assertion for login() now returning void (session is set via Set-Cookie, no body to parse)
Preview EnvironmentA preview environment can be spun up on demand for this PR.
|
CI: Frontend
All checks passed. |
CI: Backend API
All checks passed. |
…_db seed script admin_email/admin_password now read through os.getenv() with the same local-dev defaults, which also makes them overridable. Bandit only flags a password-named variable assigned directly from a string literal, so routing it through getenv() resolves the finding without changing default behavior.
…e.py The nested rollback failure handler silently swallowed all exceptions. Now logs via logger.exception() instead of pass, so a failed rollback is visible in logs rather than hidden. Behavior is unchanged: this still never blocks or fails the scan response either way.
CI: Security
All checks passed. |
…ix as ci.backend-api.yml)
…eeding (resolves CodeQL high-severity alert)
# Conflicts: # backend-api/app/api/v1/evidence.py
…nd DB connection Both endpoints were manually iterating get_async_session()/get_user_manager() instead of using Depends(), bypassing FastAPI's per-request dependency cache and opening an independent connection outside the request's transaction.
In-process ASGI transport against the real app, savepoint-based per-test DB isolation, real register/login HTTP flows. Covers registration, login/logout, profile update, password change, and auth/404 gating on the manual-verification routes.
…oints Auth-gating and 404/not-found coverage across /v1/scans/* and the public vs. auth-gated split on /v1/evidence/*. Create-scan happy path deliberately excluded pending fixtures for a real M365Connection and Celery worker.
Adds a postgres service container matching tests/conftest.py's defaults so the suite runs with zero CI-specific configuration -- same entrypoint (uv run pytest -v) as local development.
Matches this repo's existing convention (already used in the old test_manual_verification.py): # nosec B101 on test assertions, # nosec B105 on fake test credentials, # nosec B404/B603 on the controlled, no-shell-input alembic subprocess call in conftest.py. Bandit's default CLI fails the build on ANY finding regardless of severity, so a plain assert statement in a test file counts the same as a real hardcoded-password bug unless explicitly annotated.
github-actions
Bot
added
area: ci-cd
|
Hi @s225645819, thanks for your contribution and undertaking additional work to ensure the frontend is not left broken. I noticed that there are about 7 files conflicting with main, blocking merge. Out of these, some appear unrelated to the focus of this branch (from my understanding of your contribution) including workflows, Essential eight metadata, and others. I'd say the best way around these files would be to exclude these files from this PR to prevent accountability issues later on. Thanks and looking forward to your update. Note: The code freeze for this trimester (T2 2026) is 14/09/2026. |
github-actions
Bot
added
the
needs-review
- uv.lock was left inconsistent with pyproject.toml after resolving the merge conflict, causing uv sync to fail and breaking every backend job (lint, pytest, pytest coverage, integration tests) - metadata.json had a couple of arrays not matching Prettier's formatting after merging in the new POS controls, causing the JSON lint check to fail
- coverage job was still using the old '--extra dev' syntax; the dev dependencies moved to a uv dependency-group during the merge, so it needs '--group dev' like the other backend jobs already do - CIS metadata.json had the same kind of formatting mismatch as the Essential Eight one, tripping the JSON lint check - two change-password tests were patching get_user_manager on the wrong module, so the mock never actually got wired in and the real password hasher ran against a placeholder hash; switched them to dependency_overrides like the rest of the file already does - the Google callback test still expected the JWT in the redirect fragment, but that was intentionally moved into a secure HttpOnly cookie a while back; updated the assertion to match Verified all 162 backend tests pass locally against a real Postgres instance before committing.
The coverage job never had a postgres service defined, unlike the Pytest and Integration Tests jobs. This was previously hidden by the 'uv sync --extra dev' typo failing before the tests ever got to the point of needing a database connection; now that's fixed, alembic upgrade head has nothing to connect to. Mirrored the same postgres service block and tesseract system dependency step the Pytest job already uses.
Hi @subham-gp and @akshitpatel1732 — thanks both for the reviews, replying here to cover both threads. @subham-gp — the branch is now fully synced with main and all merge conflicts have been resolved (plus a few follow-up commits to fix things the merge itself surfaced: a stale lockfile, a formatting mismatch, and a couple of test bugs). CI is green now: 34/35 checks passing. The one remaining failure is the Gitleaks secret scan, which isn't related to this PR — it's flagging pre-existing findings across the repo's full branch history (there are already two separate branches, sec/regenerate-gitleaks-baseline and sec/regenerate-secrets-baseline, working on that org-wide). Whenever you get a chance, would you mind taking another look? @akshitpatel1732 — appreciate you flagging the conflicting files, and fair concern. A few of those (CI workflows, Essential Eight metadata) picked up conflicts simply because main moved forward while this branch was open, not because this PR touches that area intentionally — but once a merge is started, git needs every conflicting file resolved before the merge can complete, so excluding them wasn't really an option at that point (that would mean reverting main's changes to those files, which is worse). Instead I went through each one individually to make sure the resolution kept both sides' work: The CI workflow files: resolved so the pipeline still runs every job from both branches (nothing from main's changes was dropped). Noted on the 14/09 code freeze — appreciate the heads up, that's very close now. Would appreciate a re-look when you get a chance so we can get this landed before then. |
akshitpatel1732
left a comment
akshitpatel1732
left a comment
There was a problem hiding this comment.
This is careful work overall - I checked the cookie flags (HttpOnly/Secure/SameSite) against the actual code and they match exactly what's described, and I structurally verified both the CI workflow and Essential Eight/CIS metadata conflict resolutions against main - nothing from main's side was lost in either.
One real issue found, though: main.py's CORS config used to read allow_origins=[settings.FRONTEND_URL.rstrip("/")] on main - this branch replaces it with a hardcoded allow_origins=["http://localhost:3000"]. It happens to match FRONTEND_URL's default, so it's invisible locally, but it silently removes the ability to deploy this anywhere the frontend isn't literally localhost:3000. Could you restore the settings.FRONTEND_URL reference?
Separately, main.py also adds a Prometheus /metrics instrumentation endpoint - unrelated to the auth/SSO scope here. Was that meant to be part of this PR, or did it come in from a merge? If it's intentional, worth confirming /metrics doesn't need to be access-restricted.
Everything else I checked (cookie security, CSRF state handling, the conflict resolutions on the two files that worried me most) held up. Once the CORS regression is fixed, I think this is close.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The merge from main pulled in several newly-added/newly-enabled CI checks that immediately failed against pre-existing content, none of it touched by this PR's actual changes: - ci.security.yml had VALIDATE_JSCPD: false duplicated (added independently on this branch and by #414 on main; git merged both without a conflict since they landed on different lines). - ci.gitleaks.yml tripped a shellcheck style warning (three individual redirects instead of one { } >> block) that actionlint now enforces. - zizmor flagged unpinned actions/checkout, setup-python, setup-uv and codeql-action refs plus missing persist-credentials: false, all in workflow edits from earlier in this branch. Pinned to the same SHAs already used elsewhere in these files. - VALIDATE_PYTHON_MYPY isn't disabled anywhere, but mypy isn't a project dependency and there's no mypy config, so it fails with "cannot find implementation or library stub" on every third-party import regardless of real type correctness. Disabled it the same way pylint/ruff/pyink already are. - JSON_PRETTIER failed on 8 JSON files pulled in by the merge (plus the essential-eight metadata.json from the earlier conflict resolution) - ran prettier --write, no data changes, formatting only. - detect-secrets flagged the dummy Postgres password in the CI service container and the example password in the report-service README as unaudited candidates. Both are allowlisted with pragma comments. - gitleaks flagged two SHA-1 hashed_secret values stored inside .secrets.baseline itself (detect-secrets' own hash of an already-audited false positive) as looking like generic API keys. Regenerated .gitleaks-baseline.json to include them - verified no existing baseline entries were dropped in the process.
github-actions
Bot
added
area: docs
Regenerating .gitleaks-baseline.json and touching .secrets.baseline's timestamp made their full content count as changed in this PR. Super-linter scans full files (not just diff hunks) in PR-diff mode, so this put .secrets.baseline's hashed_secret values in front of its baseline-unaware embedded GITLEAKS validator, and a hash-like field in .gitleaks-baseline.json in front of detect-secrets. Exclude both machine-generated baseline files from super-linter's FILTER_REGEX_EXCLUDE in ci.backend-api.yml, ci.security.yml, and ci.engine.yml, and add the same exclusion to .secrets.baseline's own persisted filter list so detect-secrets stops scanning .gitleaks-baseline.json going forward. Both files already have dedicated baseline-aware scans (ci.detect-secrets.yml, ci.gitleaks.yml).
Prometheus's /metrics endpoint (added in d1c5a49) was fully public, exposing internal request/latency data (endpoint paths, traffic volume, timing) to anyone who could reach the API. Flagged in review on PR #350. Gate it behind fastapi-users' current_active_superuser instead of a new secret/token, reusing the existing auth model. Adds a regression test proving both an anonymous request and one with an invalid session cookie are rejected with 401 before the database is ever touched.
Hi @akshitpatel1732 — both points addressed: CORS: restored settings.FRONTEND_URL in bad0c69, confirmed it's no longer hardcoded. /metrics: it was intentional (added in d1c5a49 for observability) but you were right that it needed restricting — it was fully public with no auth at all. Gated it behind a superuser-only dependency (0b5304a), reusing the existing fastapi-users auth model rather than adding a new secret to manage. Added a regression test (test_metrics_requires_authentication) proving both an anonymous request and one with an invalid session cookie get rejected with 401 before the DB is even touched, so this can't silently regress. CI is green (42/42) and the full backend suite (168 tests) passes locally against a real Postgres instance. Appreciate the thorough review — let me know if anything else needs a look before the freeze. |
|
Hi @subham-gp — following up since my last update: all merge conflicts have been resolved and the branch is fully synced with main. At the time, the only remaining CI failure was the Gitleaks scan (unrelated to this PR, flagging pre-existing findings across branch history) — that's now fixed too, so CI is fully green (42/42 checks passing). Would appreciate a re-look when you get a chance, especially with the code freeze today. |
Auth is cookie-based now, so AuthContext's token is always null. The dashboard's two data-loading effects were still guarded by 'if (!token) return;', which meant they returned immediately and never ran for any authenticated user, so the dashboard never loaded any data. ProtectedRoute already gates all dashboard routes on isAuthenticated, so this is not a new security boundary -- it fixes dead logic left over from the auth migration.
…re path All 7 failure branches of the Google OAuth callback redirected without clearing the google_oauth_state cookie, unlike the success path. That left a still-valid state cookie sitting in the browser for up to its 10-minute max_age after a failed attempt, reusable by a later callback request instead of being tied to the one authorization attempt it was issued for -- contradicting the PR description's claim that state is consumed on callback. Adds a shared _google_callback_error_redirect() helper that builds the error redirect and deletes the cookie, used by all 7 failure paths. Adds a _state_cookie_cleared() test helper (httpx drops Max-Age=0 cookies from response.cookies, so deletion has to be checked on the raw Set-Cookie header) and asserts cookie clearing on each of the 7 existing failure-path tests.
Adds Dashboard.test.tsx covering the isAuthenticated-vs-token bug fixed in 9279506: asserts that an authenticated (cookie-based) user's scans, connections and benchmarks are actually fetched, and that they are not fetched when the user is unauthenticated. Verified this test fails against the pre-fix 'if (!token) return;' guards (the loading spinner never resolves) and passes against the isAuthenticated-gated fix.
There was a problem hiding this comment.
Verified both fixes directly in the code rather than just the comment - CORS is back to settings.FRONTEND_URL.rstrip("/"), confirmed no longer hardcoded, and /metrics is properly gated behind current_active_superuser. The regression test (test_metrics_requires_authentication) checks exactly the right thing - anonymous and invalid-cookie requests both rejected with 401 before the DB is touched - and the docstring traces back to this review, which is a nice habit.
Everything else from the original review held up and hasn't changed. Approving from my side - still showing subham-gp's review as outstanding, so that'll need to come through separately before this can merge.
github-actions
Bot
removed
the
needs-review
5 participants
Summary
Implements Google SSO login and migrates authentication from JSON-body JWTs to secure HttpOnly cookies, and fixes the frontend so it works end-to-end with the new cookie-based session model. This closes out review feedback from @akshitpatel1732 on PR #308, which flagged that the backend's cookie migration had left the frontend in a broken state (still expecting a token in the login response body,
localStorage/sessionStoragetoken persistence, an OAuth callback that expected anaccess_tokenin the URL, etc.).Type of Change
Affected Components
/backend-api/frontend/engine(collectors / policies)/security/infrastructure/.github/workflows/docsMotivation
The backend (
auth.py,users.py) was switched from Bearer-token JSON responses tofastapi-users'CookieTransport, issuing the session as an HttpOnly, Secure cookie instead of a JWT the client reads and stores. The frontend was never updated to match, so login, session persistence, logout, and the Google OAuth callback were all broken post-merge. This PR brings the frontend in line with the cookie-based contract and adds the Google SSO login path end-to-end.Testing Done
Unit tests pass locally
Tested manually — describe how:
Verified all four auth flows directly in the browser via DevTools Network/Application tabs:
POST /v1/auth/loginwithHttpOnlyandSameSite=Strict.GET /v1/auth/users/mereturns 200 with the cookie sent automatically; no client-side token handling.POST /v1/auth/logoutclears the cookie (Set-Cookie: Max-Age=0);/dashboardis blocked afterward.google_oauth_statecookie is set before redirecting to Google and consumed on callback; session cookie issued withSameSite=Lax(relaxed vs. theStrictpassword-login cookie, since it arrives via a top-level cross-site redirect); lands authenticated on/dashboard.Also caught and fixed a duplicate
POST /v1/auth/logoutrequest during this manual pass (see Security Considerations).Full frontend suite:
npm run build,npm run lint(oxlint),npx tsc --noEmit, andnpm run test(vitest, 204/204 passing) all clean.No tests required — explain why:
Security Considerations
Yes — this PR is entirely about the authentication mechanism.
HttpOnly(not readable by JS, mitigating XSS token theft) andSecurewhen served over HTTPS (cookie_secure=settings.BACKEND_PUBLIC_URL.startswith("https://")).SameSite=Strict; the Google OAuth callback cookie usesSameSite=Lax, which is deliberately relaxed only where required (it arrives via a top-level cross-site redirect from Google) — no other cookie is relaxed.google_oauth_statecookie provides CSRF protection for the OAuth flow and is cleared on callback.allow_credentials=Truewith an explicit origin (no wildcard), required for cookies to be sent cross-origin.AccountPage.tsxwas calling the API client'slogout()directly and viaAuthContext.logout()(which also calls it internally), causing a second request that always came back 401 once the cookie was already cleared. Fixed by removing the redundant call and adding an in-flight-promise guard toAuthContext.logout().console.log/print()debug statements in the auth lifecycle hooks (users.py) were replaced with properlogger.infocalls so user emails/tokens aren't printed to stdout.Breaking Changes
The login/OAuth callback response contract changed: clients no longer receive an
access_tokenin the JSON body or URL fragment — the session is established via an HttpOnlySet-Cookieheader instead. Any client expecting a Bearer token (the old frontend behavior, and any other integration readingaccess_tokendirectly) needs to switch tocredentials: "include"cookie-based requests. This frontend has been fully updated to match; no other known consumers of these endpoints exist.Rollback Plan
No DB migrations are involved. Reverting restores the previous Bearer-token flow on both backend and frontend consistently, since both were updated together in this PR.
Checklist
Screenshots
No visual/UI changes — this is an authentication-mechanism change with no styling or layout impact. (Happy to attach DevTools screenshots of the cookie flow if useful for review.)