Skip to content

fix(providers): clearer toast on 403 when removing a provider key#2949

Open
Sanjays2402 wants to merge 2 commits into
nesquena:masterfrom
Sanjays2402:fix/2572-providers-remove-403-message
Open

fix(providers): clearer toast on 403 when removing a provider key#2949
Sanjays2402 wants to merge 2 commits into
nesquena:masterfrom
Sanjays2402:fix/2572-providers-remove-403-message

Conversation

@Sanjays2402
Copy link
Copy Markdown
Contributor

Picks up one of the small fix candidates from #2572: when the Remove click in Settings → Providers hits a 403 (CSRF cookie/header drift, classic stale-tab signature), show a recovery-shaped toast instead of the raw 'Cross-origin request rejected' string. The raw string reads like a reverse-proxy deployment bug and leaves the user nowhere to go.

This is the JS-only sub-fix the issue calls out as not needing to be combined with the deeper investigation; it doesn't change the server response, doesn't paper over the origin-mismatch deployment case (any non-403 error still falls through to the existing 'Error: …' toast carrying the server message), and doesn't touch the CSRF code itself.

Verified locally by patching _check_csrf to return False, clicking Remove on a provider card, and confirming the toast now reads 'Session expired. Reload the page and try again.' Reverted the patch and confirmed the success path still removes the key and refreshes the dropdown caches.

Refs #2572

The Remove button under Settings -> Providers calls
POST /api/providers/delete, which runs through _check_csrf. When the
CSRF cookie/header pair has drifted (typically a tab opened before the
most recent login or cookie rotation), the server returns 403 with the
string 'Cross-origin request rejected'. That string reads like a
reverse-proxy deployment problem and gives the user no next step (nesquena#2572).

Surface a recovery-shaped toast on 403 from this endpoint:
'Session expired. Reload the page and try again.' The underlying
server response is unchanged so logs/diagnostics still see the original
string; only the user-facing toast is replaced for this code path.

Verified locally by patching _check_csrf to return False, clicking
Remove on a provider card, and confirming the toast now reads the new
message instead of the raw cross-origin string.

Refs nesquena#2572
@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Summary

Thanks @Sanjays2402 — picking the small JS-only slice off #2572 is exactly the right scope, and the catch shape (status check + fallback to the existing 'Error: '+e.message toast) is idiomatic to how api() exposes status. I read the diff plus api/routes.py:1308-1368 (the CSRF rejection path) and static/workspace.js:30-50 (where api() lifts the JSON error onto err.message + err.status).

There is one concern: the server already distinguishes three CSRF rejection reasons, and a uniform "Session expired" toast masks the more useful one.

Code reference

api/routes.py:1308-1314:

def _csrf_rejection_error(handler) -> str:
    reason = getattr(handler, _CSRF_FAILURE_ATTR, "")
    if reason == "origin_mismatch":
        return "Cross-origin mismatch - check reverse proxy headers"
    if reason == "token_mismatch":
        return "Session expired - reload the page"
    return "Cross-origin request rejected"

That string is what handle_post (and friends) put into the JSON body at api/routes.py:4898-4900:

if not _csrf_exempt_path(parsed.path) and not _check_csrf(handler):
    return j(handler, {"error": _csrf_rejection_error(handler)}, status=403)

And static/workspace.js:38-45 lifts j.error onto err.message before throwing:

let message=text;
try{const j=JSON.parse(text);message=j.error||j.message||text;}catch(e){}
const err=new Error(message);
err.status=res.status;

So in the token_mismatch and origin_mismatch cases the browser already receives a tailored string. The PR's branch at static/panels.js:6878-6884 replaces all three with the same 'Session expired. Reload the page and try again.' message, which:

  • For token_mismatch: equivalent to the server's "Session expired - reload the page". Fine.
  • For origin_mismatch: regression. The server message "Cross-origin mismatch - check reverse proxy headers" is the actionable one for the deployment-shape failure bug(providers): 'Cross-origin request rejected' toast when clicking Remove on a provider key #2572 explicitly called out as "any non-403 error" would fall through. But it's a 403, so the new branch eats it.
  • For the catch-all "Cross-origin request rejected": improvement (this is the only path the local test reproduced, since _check_csrf returning False directly with no reason set hits the fallback).

Diagnosis / Recommendation

Use the server's own message rather than overwriting it. The whole point of the three-way distinction landing in _csrf_rejection_error was so the client could surface it verbatim:

}catch(e){
  // /api/providers/delete returns a CSRF-shaped 403 with the actual reason
  // already in the error message (see api/routes.py:_csrf_rejection_error:
  // "Session expired", "Cross-origin mismatch — check reverse proxy headers",
  // or the fallback). Pass it through so deployment-shape failures keep
  // their actionable hint (#2572).
  if(e&&e.status===403){
    showToast(e.message||'Session expired. Reload the page and try again.',6000,'error');
  }else{
    showToast('Error: '+e.message);
  }
  if(els.saveBtn){els.saveBtn.disabled=false;els.saveBtn.textContent=t('providers_save');}
}

That keeps the recovery-shaped toast for the legitimately-stale-tab case (token_mismatch) while preserving the reverse-proxy hint for the origin case. The fallback "Cross-origin request rejected" string would still be visible too, but that's the case the issue says reads like a deployment bug — and it actually is one (or the reverse-proxy X-Forwarded-Host is missing, see the _check_csrf host check at api/routes.py:1340-1358). The deeper #2572 investigation can decide whether to rewrite that fallback server-side.

Optional polish: since showToast already auto-classifies by message keywords ('fail|error|denied|invalid|...' regex at static/ui.js:3795), the explicit 'error' third argument is redundant for messages containing "expired" or "mismatch" — but harmless and clearer to read, so I'd leave it.

Test plan

After the fix, three cases (no server changes needed):

  1. Stale token: Open Settings → Providers, then in DevTools delete the hermes_csrf cookie. Click Remove. Expect: "Session expired - reload the page" toast.
  2. Origin mismatch: Override Origin header via a DevTools fetch hook (or patch _check_csrf to force-return origin_mismatch). Click Remove. Expect: "Cross-origin mismatch - check reverse proxy headers" toast.
  3. Catch-all: Force _check_csrf to return False without setting a reason. Click Remove. Expect: "Cross-origin request rejected" toast.

The non-403 path (server 500, network error) should still hit 'Error: '+e.message — quick sanity check by removing a provider while the server is down.

CHANGELOG entry can stay the same; the user-visible improvement is real, just don't claim a single substituted string. Maybe reword to "Removing a provider key now surfaces the server's specific CSRF rejection reason (session expired, origin mismatch, etc.) instead of a generic raw string..."

@Sanjays2402
Copy link
Copy Markdown
Contributor Author

Right call, fixed in 0211e1e. Now passes e.message through verbatim so all three _csrf_rejection_error reasons (token mismatch, origin mismatch, fallback) reach the toast unchanged, with the generic "Session expired" string kept only as the fallback when e.message is empty. CHANGELOG entry rephrased to match.

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