Skip to content

fix(auth): don't crash on public endpoints when the user row is missing - #665

Open
Zaidusyy wants to merge 2 commits into
hatnote:masterfrom
Zaidusyy:fix/608-debug-login-crash
Open

Zaidusyy wants to merge 2 commits into
hatnote:masterfrom
Zaidusyy:fix/608-debug-login-crash

Conversation

@Zaidusyy

Copy link
Copy Markdown

In debug mode a request without a session cookie looks up the sentinel userid 0, which has no row. For a public endpoint the user is then None, but the middleware went on to read user.username (when a superuser is configured) and user.last_active_date, so the first visit to /complete_login ended in "AttributeError: 'NoneType' object has no attribute 'username'" and an error page. That is the step dev.md tells developers to start with, and it fails with debug_userid and debug_username set as well.

Continue as an anonymous request instead, the same as the non-debug branch above already does. Endpoints that need a user are still refused with the "unknown cookie userid" error.

Refs #608

In debug mode a request without a session cookie looks up the sentinel
userid 0, which has no row. For a public endpoint the user is then None,
but the middleware went on to read user.username (when a superuser is
configured) and user.last_active_date, so the first visit to
/complete_login ended in "AttributeError: 'NoneType' object has no
attribute 'username'" and an error page. That is the step dev.md tells
developers to start with, and it fails with debug_userid and
debug_username set as well.

Continue as an anonymous request instead, the same as the non-debug
branch above already does. Endpoints that need a user are still refused
with the "unknown cookie userid" error.

Refs hatnote#608

@lgelauff lgelauff left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @Zaidusyy, nice find, and a clean fix. The middleware change looks right to me: an unknown user on a public endpoint now goes on as an anonymous request, which is what the no-cookie branch just above already does. Protected endpoints still refuse, and the superuser / su_to branch still needs a real user row, so nothing gets more access than before. It also covers the non-debug case of a valid cookie whose user row is gone.

Claude was able to run the suite locally: everything passes, and with the mw/__init__.py change reverted test_debug_complete_login_works_before_first_login fails with the 500, so the test catches the bug.

One change before merging: the debug_app fixture sets config['__env__'] = 'dev'. In create_app, that means debug_errors is False, so it calls sentry_sdk.init(...) with Montage's real DSN (montage/app.py, the if not debug_errors: block) and adds SentryMiddleware. Any exception in these tests is then reported to the project's Sentry, and because sentry_sdk.init is process-wide it stays on for the rest of the test run. That happened to us locally while a dependency was out of date ("Sentry is attempting to send 2 pending events"). Setting MONTAGE_PDB in the fixture keeps the dev behaviour you need and skips Sentry, as the devtest env already does:

@pytest.fixture
def debug_app(tmpdir, monkeypatch):
    """App as run for local development: debug mode, and no user row for the sentinel id."""
    monkeypatch.setenv('MONTAGE_PDB', '1')  # no Sentry from tests
    ...

Small thing: the PR says "Refs #608". If you consider #608 fixed by this, "Fixes #608" closes it on merge. The second part of #608 (still not logged in after clicking Login) may be something else, so "Refs" is fine too.

Written by Claude (Claude Code) on behalf of @lgelauff.

The fixture runs the app as the dev environment, and outside devtest
create_app starts Sentry with the project's DSN unless MONTAGE_PDB is
set. An exception in these tests would then have been reported to the
project's Sentry, and sentry_sdk.init stays on for the rest of the test
run. Set MONTAGE_PDB, as the devtest environment already implies.
@Zaidusyy

Copy link
Copy Markdown
Author

Thanks @lgelauff, good catch. I've pushed a second commit that sets MONTAGE_PDB in the debug_app fixture. I checked it with a throwaway test that records calls to sentry_sdk.init: before the change it was called with the project's DSN, after it isn't. I'm leaving "Refs #608", since the second part of the issue (still not logged in after clicking Login) may be something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants