reportClientError sent no Authorization header, so every source='frontend' row
was anonymous no matter who was signed in -- which made ErrorLog.user_id,
to_dict()['username'] and the "User #N" row in pages/Errors.jsx dead end to
end, and the route's own docstring ("a valid JWT attaches user_id") false.
Attaching the header by hand does not violate the deliberate design here: the
comment forbids this.request() -- its 401 handling, refresh loop and throwing
-- not a header. The raw fetch and the swallow-everything contract are intact,
and Content-Type already made this a non-simple CORS request, so no new
preflight is added.
The review's stated rationale was wrong and the corrected version is in the
comment: verify_jwt_in_request(optional=True) tolerates a MISSING token only.
In Flask-JWT-Extended 4.6.0 it catches NoAuthorizationError alone, so an
expired token raises ExpiredSignatureError and garbage raises DecodeError. A
stale token is harmless here only because the route wraps that call in
try/except and falls back to an anonymous report -- tightening that except
would turn every expired session's crash report into a 500.
Three backend tests pin the contract the header relies on: valid token
attributes the row, no token stays anonymous, junk token stays anonymous
rather than 500ing. The frontend side that sends it is unproven -- the panel
has no test runner for pages.
Known ceiling: dedup bumps count on an existing unresolved row without
backfilling user_id, so attribution is first-writer-wins per fingerprint.
Reported by Copilot on the error-tracker PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 500 handler shares the app-context-scoped db.session with the view that
just failed, which breaks two ways.
The one that matters: when the crash IS a database error, that session is
already in a failed transaction, so record_error's very first query raises
PendingRollbackError -- swallowed by its own never-raise contract. Database-
caused 500s were therefore the one class of crash the error tracker could never
show, and they are the class most worth seeing. 72 API views commit with no
try/except anywhere in the function, so this is not a narrow path.
The second: record_error's commit() flushes whatever ORM state the crashed view
left pending, persisting a half-applied write on the way out.
Rolling back first fixes both. Flask-SQLAlchemy's teardown rolls this session
back moments later anyway, so a clean request is unaffected.
Reviewed as an alternative to a partial unique index on the dedup fingerprint,
which was rejected: record_error swallows everything and returns None, so an
IntegrityError would DROP the error report rather than dedupe it; set_resolved
commits unguarded, so un-resolving an old row sharing a fingerprint would 500 a
legitimate admin action; and CREATE UNIQUE INDEX would abort the migration on
exactly the installs that already accumulated duplicates. The race is real but
its blast radius is duplicate rows in an admin list -- nothing reads
fingerprint for uniqueness, and the lookup is .first(), not .one().
Both new tests fail with the rollback removed (mutation-checked by line
number). The first draft of the DB-error test did NOT: a hand-built
IntegrityError never poisons the session, so it has to be a real failed flush.
Race reported as High by Copilot on the error-tracker PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Juan Denis