Skip to content

fix(tests): stop unconditionally mocking require_api_key in saved-views tests - #2193

Open
Subramaniyajothi6 wants to merge 4 commits into
utksh1:mainfrom
Subramaniyajothi6:fix/saved-views-auth-bypass-2192
Open

fix(tests): stop unconditionally mocking require_api_key in saved-views tests#2193
Subramaniyajothi6 wants to merge 4 commits into
utksh1:mainfrom
Subramaniyajothi6:fix/saved-views-auth-bypass-2192

Conversation

@Subramaniyajothi6

Copy link
Copy Markdown
Contributor

Summary

Fixes the backend-unit failure tracked in #2192 (test_saved_views.py::test_unauthenticated_request_rejected and test_wrong_api_key_rejected, both asserting 200 == 401).

Correction to #2192: after digging in, this is a test-fixture bug, not a live security regression. Production auth is fine — saved_views_router in backend/secuscan/saved_views.py still declares dependencies=[Depends(require_api_key)] at the router level, and require_api_key in auth.py is untouched and correctly rejects missing/invalid keys.

The actual cause: the app_client fixture in testing/backend/unit/test_saved_views.py did this unconditionally:

_app.dependency_overrides[require_api_key] = _mock_require_api_key

_mock_require_api_key always returns success, regardless of what's in the request. That override predates PR #2025/issue #1743. When #2025 added the two auth-negative tests (test_unauthenticated_request_rejected, test_wrong_api_key_rejected) alongside a real API key already being issued and sent via X-Api-Key, the override made those two tests structurally unable to pass — they always hit the mock, never the real check, so they always got 200 no matter what header was sent.

Fix

Remove the require_api_key dependency override entirely. The fixture already provisions a real API key (_auth_module.init_api_key(...)) and sends it via X-Api-Key on every client, so the real dependency is exercised for all existing happy-path tests too — nothing else needed mocking here.

Verification

  • pytest testing/backend/unit/test_saved_views.py -q → 47 passed (was 2 failed, 45 passed before the fix).
  • pytest testing/backend/unit -q -m "not benchmark" → same pass count as before plus these 2, no new failures.
  • ruff check backend testing/backend → clean.

Test plan

  • test_unauthenticated_request_rejected and test_wrong_api_key_rejected pass
  • Full testing/backend/unit suite has no new failures
  • ruff check backend testing/backend passes

@Subramaniyajothi6

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and pushed a second commit — flagging the added scope, since it wasn't in the original diff.

Why the extra commit is needed

1c33d644 ("Fix/docker network exposure", #2268, merged 2026-07-30) added @pytest.mark.skip to both auth tests in testing/backend/unit/test_saved_views.py:

@pytest.mark.skip(reason="pre-existing upstream issue: app_client overrides auth so 401 cannot be tested here")

That reason is accurate about the cause — the app_client fixture overrode require_api_key, so 401 could never be observed — and removing that override is exactly what this PR does. But the skip markers landed on main six days after this branch was pushed, so this branch didn't touch those lines.

Merging as-is would have kept them. Simulating the merge against current main:

$ git merge-tree --write-tree upstream/main b7e85efc
$ git cat-file -p <tree>:testing/backend/unit/test_saved_views.py | grep -c "pytest.mark.skip"
2

So the PR would have gone green, closed #2192, and left both auth tests skipped on main — the same coverage gap the issue was filed about. The second commit removes the markers so the fix actually takes effect.

Verification (rebased branch, 663cc7a2):

  • testing/backend/unit/test_saved_views.py — 47 passed, 0 skipped
  • test_unauthenticated_request_rejected and test_wrong_api_key_rejected both execute and pass
  • Full testing/backend/unit — 2323 passed, 19 skipped, 0 failed
  • ruff check backend testing/backend — clean

On the issue title: worth noting there was never a live auth bypass. saved_views.py declares dependencies=[Depends(require_api_key)] at the router level and enforces it correctly — the un-skipped tests passing is direct evidence of that. #2192 was a test-harness defect only, not a production vulnerability.

Diff is still one file: testing/backend/unit/test_saved_views.py.

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the updated test fixture and auth coverage. Removing the unconditional dependency override allows the unauthenticated and invalid-key tests to exercise the real router dependency, and the added commit removes the skips introduced on main. The branch still needs a fresh rebase and required checks before merge.

@utksh1
utksh1 force-pushed the fix/saved-views-auth-bypass-2192 branch from 663cc7a to fa68e3f Compare August 4, 2026 11:40
@utksh1

utksh1 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Fresh required CI is blocked at the shared frontend npm audit gate by high-severity undici advisory GHSA-4cwx-7wf7-3272. This PR is backend-test-only and does not change frontend dependencies; approval remains in place while the repository-level audit issue is unresolved.

…ws tests

The app_client fixture overrode require_api_key with a mock that always
succeeds, regardless of the X-Api-Key header. That made the fixture's
own negative-path tests (test_unauthenticated_request_rejected,
test_wrong_api_key_rejected) always observe a 200, since the real auth
check never ran. The fixture already issues a real API key and sends it
via X-Api-Key, so the mock was redundant for the happy-path tests and
actively broke the auth-negative ones. Production auth in
saved_views.py (router-level Depends(require_api_key)) was never
affected — this was a test-only gap, not a live auth bypass.
Commit 1c33d64 marked test_unauthenticated_request_rejected and
test_wrong_api_key_rejected as skipped, noting that app_client
overrode require_api_key so 401 could not be observed.

This branch already removes that override, so the tests can run for
real. Dropping the skip markers restores actual coverage of auth
enforcement on the saved-views router, which does declare
dependencies=[Depends(require_api_key)] and rejects both an empty and
an incorrect API key.
@utksh1
utksh1 force-pushed the fix/saved-views-auth-bypass-2192 branch from fa68e3f to d4d3aa8 Compare August 5, 2026 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work level:beginner 20 pts difficulty label for small beginner-friendly PRs type:testing Testing work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants