From c4e480a0c47da03c1dd1ee4e984ff4e01132e338 Mon Sep 17 00:00:00 2001 From: Kristian Wind Date: Tue, 18 Aug 2026 13:28:44 +0200 Subject: [PATCH] fix(gmail): stop hiding why it failed, twice over MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from use: "Fetch now" said "something went wrong" and the error log was empty. Both halves were the same bug wearing two hats. `handleGmailSyncNow` answered 502 with the code `internal_error`, which the interface maps to a generic sentence — so Google's own words, which are the whole diagnosis, were replaced by one that says nothing. And `writeError` records nothing; only `s.internal` does. So there was no message to read and nowhere to look it up afterwards. `s.upstream()` is the answer: 502 because this is somebody else's service saying no rather than this server breaking, a code of its own so the interface can quote what was said, and a row in the error log — which is the entire reason that log exists. A Gmail sync failing quietly for a week is otherwise invisible: the background job writes a line the next restart erases. The same connection also left the mailbox unnamed. The address is learned at connect time and that call can fail; a warning went to the log and an empty string to the database, so the page said "an unknown account" and kept saying it. The only way back was to disconnect and reconnect, which nobody would guess. The status endpoint asks again when it finds none, so a wrong answer lasts until the next look rather than until somebody reconnects. Ten Danish strings were hiding in `confirm()` and `app.toast()` calls, including every destructive confirmation in the app — the worst possible set to leave in one language, since it is what somebody reads in the half-second before agreeing to lose something. The i18n guard reads markup and attributes, and a string argument lives in the script block. It reads those now, and found four my own grep missed. Co-Authored-By: Claude Opus 5 --- internal/httpapi/auth_handlers.go | 28 ++++++++ internal/httpapi/errors.go | 7 ++ internal/httpapi/gmail_handlers.go | 41 ++++++++++- internal/httpapi/gmail_test.go | 70 +++++++++++++++++++ internal/httpapi/integrations_handlers.go | 20 +++++- internal/i18n/untranslated_test.go | 50 +++++++++++++ web/src/lib/api.js | 12 ++++ web/src/lib/components/BoardView.svelte | 2 +- web/src/lib/components/TaskDetail.svelte | 2 +- web/src/lib/locales/da.js | 21 +++++- web/src/lib/locales/en.js | 21 +++++- web/src/routes/indstillinger/+page.svelte | 2 +- .../routes/indstillinger/data/+page.svelte | 2 +- .../indstillinger/integrationer/+page.svelte | 10 +-- .../routes/indstillinger/tokens/+page.svelte | 6 +- web/src/routes/projekt/[id]/+page.svelte | 6 +- 16 files changed, 281 insertions(+), 19 deletions(-) create mode 100644 internal/httpapi/gmail_test.go diff --git a/internal/httpapi/auth_handlers.go b/internal/httpapi/auth_handlers.go index 57d6508..7ab3ab0 100644 --- a/internal/httpapi/auth_handlers.go +++ b/internal/httpapi/auth_handlers.go @@ -573,6 +573,34 @@ func inboxName(locale string) string { // 11:49" long after the line explaining it has gone, which tells an operator that // something broke and gives them no way to find out what. The row outlives the // container; Indstillinger → Fejl is where it is read. +// upstream reports a failure that came from somebody else's service. +// +// Separate from `internal` because the cause is different and so is the fix: a 500 +// is this server broken, and this is Google — or whoever — saying no. The status +// says so (502), and the message is passed through rather than replaced, because +// "invalid_grant" or "insufficient authentication scopes" is the whole diagnosis +// and a generic sentence in its place throws it away. +// +// It is recorded like a 500 for exactly the reason the error log exists: a Gmail +// sync that has been failing for a week is invisible otherwise. The background job +// writes a log line the next restart erases, and nobody is watching the one screen +// that would have said so. +func (s *Server) upstream(w http.ResponseWriter, r *http.Request, code, what string, err error) { + s.log.Warn(what, "err", err) + + userID := "" + if u := userFrom(r.Context()); u != nil { + userID = u.ID + } + s.db.RecordError(r.Context(), store.ServerError{ + Method: r.Method, Path: r.URL.Path, Status: http.StatusBadGateway, + What: what, Message: err.Error(), UserID: userID, + RequestID: middleware.GetReqID(r.Context()), + }) + + writeError(w, http.StatusBadGateway, code, err.Error()) +} + func (s *Server) internal(w http.ResponseWriter, r *http.Request, what string, err error) { s.log.Error(what, "err", err) diff --git a/internal/httpapi/errors.go b/internal/httpapi/errors.go index 7295314..1be61b5 100644 --- a/internal/httpapi/errors.go +++ b/internal/httpapi/errors.go @@ -51,6 +51,13 @@ const ( // is not "that already exists" either, and it is the one refusal on the user // page that a person needs explained rather than merely reported. CodeLastAdmin = "last_admin" + + // Gmail said no. Its own words are the diagnosis — "invalid_grant", + // "insufficient authentication scopes" — and they are useless behind a generic + // sentence, which is what `internal_error` gave: a person clicked Fetch now, + // read "something went wrong", and had nowhere to go. The interface wraps the + // message in one that says where it came from. + CodeGmailFailed = "gmail_failed" ) // errLastAdmin is returned to the caller that already wrote the response, so it diff --git a/internal/httpapi/gmail_handlers.go b/internal/httpapi/gmail_handlers.go index 4979029..c1df767 100644 --- a/internal/httpapi/gmail_handlers.go +++ b/internal/httpapi/gmail_handlers.go @@ -150,6 +150,9 @@ func (s *Server) handleGmailCallback(w http.ResponseWriter, r *http.Request) { return } + // Best effort: a mailbox that connects but cannot name itself is still + // connected, and the status endpoint asks again when it finds no address — + // so this failing is a delay rather than a dead end. email, err := gmail.NewClient(token.AccessToken).Profile(r.Context()) if err != nil { s.log.Warn("gmail profile", "err", err) @@ -184,7 +187,7 @@ func (s *Server) handleGmailSyncNow(w http.ResponseWriter, r *http.Request) { created, err := s.SyncGmail(r.Context(), user) if err != nil { - writeError(w, http.StatusBadGateway, CodeInternal, err.Error()) + s.upstream(w, r, CodeGmailFailed, "sync gmail", err) return } writeJSON(w, http.StatusOK, map[string]int{"created": created}) @@ -425,3 +428,39 @@ func (s *Server) handleSetGmailClient(w http.ResponseWriter, r *http.Request) { } w.WriteHeader(http.StatusNoContent) } + +// gmailProfile fetches the mailbox address and stores it, refreshing the access +// token first if it has expired. +// +// Shared by the connect callback and the status endpoint. The callback is the +// natural place to learn the address, and it is also the one moment that cannot be +// retried — so the status endpoint asks again when it finds nothing, and the answer +// corrects itself on the next look. +func (s *Server) gmailProfile(ctx context.Context, user *store.User, settings map[string]any) (string, error) { + str := func(key string) string { + v, _ := settings[key].(string) + return v + } + + access := str("access_token") + expires, _ := settings["expires_at"].(float64) + if access == "" || int64(expires) <= time.Now().Unix() { + token, err := s.gmailConfig(ctx).Refresh(ctx, str("refresh_token")) + if err != nil { + return "", err + } + access = token.AccessToken + settings["access_token"] = access + settings["expires_at"] = token.ExpiresAt.Unix() + } + + email, err := gmail.NewClient(access).Profile(ctx) + if err != nil { + return "", err + } + settings["email"] = email + if err := s.db.SetUserSettings(ctx, user.ID, "gmail", settings); err != nil { + return "", err + } + return email, nil +} diff --git a/internal/httpapi/gmail_test.go b/internal/httpapi/gmail_test.go new file mode 100644 index 0000000..74ed08d --- /dev/null +++ b/internal/httpapi/gmail_test.go @@ -0,0 +1,70 @@ +package httpapi + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/kristianwind/verdande/internal/config" +) + +// A Gmail failure has to survive being reported. +// +// It used to be hidden twice over. The handler answered 502 with the code +// `internal_error`, which the interface maps to "something went wrong" — so +// Google's own words, the only thing that says what to do, were replaced by a +// sentence that says nothing. And `writeError` records nothing, so the error log +// was empty: a person clicked Fetch now, read a generic failure, and there was +// nowhere to look it up afterwards. +func TestAGmailFailureSaysWhatGmailSaidAndIsRecorded(t *testing.T) { + // A Gmail that refuses, the way an expired grant does. + google := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"error":"invalid_grant"}`, http.StatusBadRequest) + })) + defer google.Close() + + ts := newTestServerWith(t, func(cfg *config.Config) { + cfg.GmailClientID = "test-client" + cfg.GmailClientSecret = "test-secret" + }) + ts.bootstrap(t) + + user, err := ts.db.UserByEmail(t.Context(), "kristian@example.dk") + if err != nil { + t.Fatal(err) + } + // Connected, with a refresh token that will be refused. + if err := ts.db.SetUserSettings(t.Context(), user.ID, "gmail", map[string]any{ + "refresh_token": "spent", "trigger": "starred", + }); err != nil { + t.Fatal(err) + } + + resp, body := ts.do(t, "POST", "/api/v1/gmail/sync", nil) + if resp.StatusCode != http.StatusBadGateway { + t.Fatalf("sync: status %d, want 502 — this is Gmail saying no, not this server breaking", + resp.StatusCode) + } + if body["code"] != "gmail_failed" { + t.Errorf("code = %v, want gmail_failed — `internal_error` is mapped to a generic "+ + "sentence, which throws the diagnosis away", body["code"]) + } + if msg, _ := body["error"].(string); msg == "" { + t.Error("the message is empty; Gmail's own words are the whole diagnosis") + } + + // And it is in the error log, which is the point of having one: a sync that has + // been failing for a week is invisible otherwise. + _, logged := ts.do(t, "GET", "/api/v1/errors", nil) + rows, _ := logged["errors"].([]any) + if len(rows) == 0 { + t.Fatal("nothing was recorded; the failure is as invisible as it was before") + } + row := rows[0].(map[string]any) + if row["what"] != "sync gmail" { + t.Errorf("what = %v, want \"sync gmail\"", row["what"]) + } + if row["status"] != float64(http.StatusBadGateway) { + t.Errorf("status = %v, want 502", row["status"]) + } +} diff --git a/internal/httpapi/integrations_handlers.go b/internal/httpapi/integrations_handlers.go index 02e5a5f..008407d 100644 --- a/internal/httpapi/integrations_handlers.go +++ b/internal/httpapi/integrations_handlers.go @@ -379,9 +379,27 @@ func (s *Server) handleGetGmail(w http.ResponseWriter, r *http.Request) { v, _ := values[key].(string) return v } + // The mailbox address, fetched again if it is missing. + // + // It is stored at connect time, and the call that fetches it can fail — a + // slow token, a scope Google had not applied yet. That failure used to be a + // log line and an empty string, so the page said "an unknown account" and went + // on saying it forever: the only way back was to disconnect and reconnect, + // which nobody would guess. Asking again here costs one request on a page + // nobody opens often, and it means the wrong answer lasts until the next look + // rather than until somebody reconnects. + email := str("email") + if email == "" && str("refresh_token") != "" { + if fetched, err := s.gmailProfile(r.Context(), userFrom(r.Context()), values); err == nil { + email = fetched + } else { + s.log.Warn("gmail profile", "err", err) + } + } + writeJSON(w, http.StatusOK, gmailSettings{ Connected: str("refresh_token") != "", - Email: str("email"), + Email: email, Trigger: str("trigger"), Label: str("label"), }) diff --git a/internal/i18n/untranslated_test.go b/internal/i18n/untranslated_test.go index c68d2a4..6622c26 100644 --- a/internal/i18n/untranslated_test.go +++ b/internal/i18n/untranslated_test.go @@ -20,6 +20,56 @@ import ( // away whatever letters it happens to use. A word list rather than a language // detector: this repository ships one static binary, and a list of thirty common // words catches the sentences a person would actually write. +// The same words, inside the code rather than the markup. +// +// `confirm()`, `alert()` and `app.toast()` take a string argument, and a string +// argument lives in the script block where the scanner above never looks. Six of +// them survived the whole i18n sweep that way — every destructive confirmation in +// the app, which is the worst possible set to leave in one language, because it is +// the text somebody reads in the half-second before they agree to lose something. +func TestNoDanishProseInDialogsAndToasts(t *testing.T) { + // The argument of one of the three, quoted or backticked. Deliberately narrow: + // a general scan of every string literal in a Svelte script block would drown + // in class names, keys and query fragments. + call := regexp.MustCompile(`(?:confirm|alert|app\.toast)\(\s*['` + "`" + `]([^'` + "`" + `]{4,})['` + "`" + `]`) + danish := regexp.MustCompile(`(?i)\b(og|er|ikke|det|den|der|til|kan|som|har|med|af|en|et|du|din|dine|vises|ingen|nyere|fundet|ude|valgt|slettet|gemt|opgave|projekt|indstillinger|fjern|opret|vælg|luk|tilføj|ryd|gem|slet|omdøb|bliver|holder|alle)\b`) + + var found []string + root := filepath.Join("..", "..", "web", "src") + err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil || info.IsDir() { + return err + } + if !strings.HasSuffix(path, ".svelte") && !strings.HasSuffix(path, ".js") { + return nil + } + if strings.Contains(path, "locales") { + return nil + } + body, err := os.ReadFile(path) + if err != nil { + return err + } + rel, _ := filepath.Rel(root, path) + for _, m := range call.FindAllStringSubmatch(string(body), -1) { + if danish.MatchString(m[1]) { + found = append(found, rel+": "+m[1]) + } + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", root, err) + } + + if len(found) > 0 { + t.Errorf("%d dialog or toast string(s) are written in Danish inside the code. "+ + "They are what somebody reads before agreeing to lose something, and an "+ + "English interface shows them in Danish:\n %s", + len(found), strings.Join(found, "\n ")) + } +} + func TestNoDanishProseOutsideTheDictionaries(t *testing.T) { // Text nodes of three characters or more, and the three attributes a person // reads. Anything inside {…} is already an expression — usually a t() call — diff --git a/web/src/lib/api.js b/web/src/lib/api.js index 36dfc2b..87280d5 100644 --- a/web/src/lib/api.js +++ b/web/src/lib/api.js @@ -55,6 +55,9 @@ const MESSAGES = { ai_not_configured: 'error.aiNotConfigured', totp_not_enabled: 'error.totpNotEnabled', inbox_protected: 'error.inboxProtected', + // The fallback for when Gmail refused and said nothing usable. When it did say + // something, `humanMessage` quotes it instead — see below. + gmail_failed: 'error.gmailRefused', last_admin: 'error.lastAdmin' }; @@ -67,6 +70,15 @@ const MESSAGES = { */ export function humanMessage(err) { if (!(err instanceof ApiError)) return t(MESSAGES.offline); + + // Gmail's own words are the diagnosis — "invalid_grant", "insufficient + // authentication scopes" — and a generic sentence in their place throws away + // the only thing that says what to do. Wrapped so it reads as a quotation + // rather than as a stray English string in a Danish interface. + if (err.code === 'gmail_failed' && err.message) { + return t('error.gmailSaid', { what: err.message }); + } + const key = MESSAGES[err.code]; if (key) return t(key); return err.message ?? t(MESSAGES.internal_error); diff --git a/web/src/lib/components/BoardView.svelte b/web/src/lib/components/BoardView.svelte index 53c215c..c62e587 100644 --- a/web/src/lib/components/BoardView.svelte +++ b/web/src/lib/components/BoardView.svelte @@ -125,7 +125,7 @@ ); } catch { app.replace(id, previous); - app.toast('Kunne ikke flytte opgaven.'); + app.toast(t('task.moveFailed')); } } diff --git a/web/src/lib/components/TaskDetail.svelte b/web/src/lib/components/TaskDetail.svelte index aaf710f..a41f347 100644 --- a/web/src/lib/components/TaskDetail.svelte +++ b/web/src/lib/components/TaskDetail.svelte @@ -276,7 +276,7 @@ } async function removeComment(comment) { - if (!confirm('Slet kommentaren?')) return; + if (!confirm(t('detail.deleteComment'))) return; const previous = comments; comments = comments.filter((c) => c.id !== comment.id); try { diff --git a/web/src/lib/locales/da.js b/web/src/lib/locales/da.js index e6352ad..0daef78 100644 --- a/web/src/lib/locales/da.js +++ b/web/src/lib/locales/da.js @@ -646,5 +646,24 @@ export const da = { 'update.waiting': 'Venter på at serveren kommer op igen…', 'update.stillDown': 'Serveren er ikke kommet op endnu. Kig i panelet.', 'update.notConfigured': - 'Denne instans kan ikke genstarte sig selv: {missing} er ikke sat. En container kan ikke udskifte sit eget image — panelet gør arbejdet, og det her er kun måden at bede om det.' + 'Denne instans kan ikke genstarte sig selv: {missing} er ikke sat. En container kan ikke udskifte sit eget image — panelet gør arbejdet, og det her er kun måden at bede om det.', + + 'error.gmailSaid': 'Gmail svarede: {what}', + + 'error.gmailRefused': 'Gmail afviste kaldet. Prøv at forbinde postkassen igen.', + + 'int.newAddressQuestion': 'Ny adresse? Mail til den gamle bliver afvist.', + 'int.copied': 'Kopieret.', + 'int.copyFailed': 'Kunne ikke kopiere — markér teksten i stedet.', + + 'detail.deleteComment': 'Slet kommentaren?', + 'int.disconnectQuestion': 'Afbryd forbindelsen til Gmail? Tokens bliver glemt.', + 'int.newLinkQuestion': 'Nyt feed-link? Alle eksisterende abonnementer holder op med at virke.', + 'data.deleteTemplate': 'Slet skabelonen "{name}"?', + 'tokens.revokeQuestion': 'Tilbagekald "{name}"? Alt, der bruger den, holder op med at virke.', + 'account.signOutThisDevice': 'Det er denne enhed. Du bliver logget ud.', + + 'project.deleteQuestion': 'Slet "{name}" med alle dets opgaver?', + 'project.deleteRecoverable': + 'Det havner i papirkurven og kan hentes tilbage under Indstillinger → Data og skabeloner i {days} dage.' }; diff --git a/web/src/lib/locales/en.js b/web/src/lib/locales/en.js index 21276b9..7dd3e94 100644 --- a/web/src/lib/locales/en.js +++ b/web/src/lib/locales/en.js @@ -641,5 +641,24 @@ export const en = { 'update.waiting': 'Waiting for the server to come back…', 'update.stillDown': 'The server has not come back yet. Have a look in the panel.', 'update.notConfigured': - 'This instance cannot restart itself: {missing} are not set. A container cannot replace its own image — the panel does the work, and this is only the way to ask.' + 'This instance cannot restart itself: {missing} are not set. A container cannot replace its own image — the panel does the work, and this is only the way to ask.', + + 'error.gmailSaid': 'Gmail said: {what}', + + 'error.gmailRefused': 'Gmail refused the call. Try connecting the mailbox again.', + + 'int.newAddressQuestion': 'A new address? Mail to the old one is refused.', + 'int.copied': 'Copied.', + 'int.copyFailed': 'Could not copy — select the text instead.', + + 'detail.deleteComment': 'Delete the comment?', + 'int.disconnectQuestion': 'Disconnect Gmail? The tokens are forgotten.', + 'int.newLinkQuestion': 'A new feed link? Every existing subscription stops working.', + 'data.deleteTemplate': 'Delete the template "{name}"?', + 'tokens.revokeQuestion': 'Revoke "{name}"? Everything using it stops working.', + 'account.signOutThisDevice': 'That is this device. You will be signed out.', + + 'project.deleteQuestion': 'Delete "{name}" with all its tasks?', + 'project.deleteRecoverable': + 'It goes to the trash and can be restored under Settings → Data and templates for {days} days.' }; diff --git a/web/src/routes/indstillinger/+page.svelte b/web/src/routes/indstillinger/+page.svelte index e8a3f7b..2b7c917 100644 --- a/web/src/routes/indstillinger/+page.svelte +++ b/web/src/routes/indstillinger/+page.svelte @@ -215,7 +215,7 @@ async function endSession(session) { if (session.current) { - if (!confirm('Det er denne enhed. Du bliver logget ud.')) return; + if (!confirm(t('account.signOutThisDevice'))) return; await api.endSession(session.id); // A full reload rather than clearing the store by hand: the session is // gone, so every open socket and every cached answer in this tab is now diff --git a/web/src/routes/indstillinger/data/+page.svelte b/web/src/routes/indstillinger/data/+page.svelte index b3b0034..03adf8b 100644 --- a/web/src/routes/indstillinger/data/+page.svelte +++ b/web/src/routes/indstillinger/data/+page.svelte @@ -138,7 +138,7 @@ } async function deleteTemplate(template) { - if (!confirm(`Slet skabelonen "${template.name}"?`)) return; + if (!confirm(t('data.deleteTemplate', { name: template.name }))) return; const previous = templates; templates = templates.filter((t) => t.id !== template.id); diff --git a/web/src/routes/indstillinger/integrationer/+page.svelte b/web/src/routes/indstillinger/integrationer/+page.svelte index 40ee888..6f02e9c 100644 --- a/web/src/routes/indstillinger/integrationer/+page.svelte +++ b/web/src/routes/indstillinger/integrationer/+page.svelte @@ -125,7 +125,7 @@ } async function disconnectGmail() { - if (!confirm('Afbryd forbindelsen til Gmail? Tokens bliver glemt.')) return; + if (!confirm(t('int.disconnectQuestion'))) return; try { await api.disconnectGmail(); gmail = await api.getGmail(); @@ -153,7 +153,7 @@ }); async function rotateFeed() { - if (!confirm('Nyt feed-link? Alle eksisterende abonnementer holder op med at virke.')) return; + if (!confirm(t('int.newLinkQuestion'))) return; try { feedURL = (await api.rotateFeed()).url; } catch (e) { @@ -162,7 +162,7 @@ } async function rotateMail() { - if (!confirm('Ny adresse? Mail til den gamle bliver afvist.')) return; + if (!confirm(t('int.newAddressQuestion'))) return; try { const r = await api.rotateMailAddress(); mailAddress = r.address; @@ -175,9 +175,9 @@ async function copy(text) { try { await navigator.clipboard.writeText(text); - app.toast('Kopieret.'); + app.toast(t('int.copied')); } catch { - app.toast('Kunne ikke kopiere — markér teksten i stedet.'); + app.toast(t('int.copyFailed')); } } diff --git a/web/src/routes/indstillinger/tokens/+page.svelte b/web/src/routes/indstillinger/tokens/+page.svelte index 99d09c7..1479194 100644 --- a/web/src/routes/indstillinger/tokens/+page.svelte +++ b/web/src/routes/indstillinger/tokens/+page.svelte @@ -60,7 +60,7 @@ } catch { // Insecure context, or permission refused. The token is on screen and // selectable, so this is a convenience that failed, not the feature. - app.toast('Kunne ikke kopiere — markér teksten i stedet.'); + app.toast(t('int.copyFailed')); } } @@ -69,12 +69,12 @@ await navigator.clipboard.writeText(connectorURL); connectorCopied = true; } catch { - app.toast('Kunne ikke kopiere — markér teksten i stedet.'); + app.toast(t('int.copyFailed')); } } async function revoke(token) { - if (!confirm(`Tilbagekald "${token.name}"? Alt, der bruger den, holder op med at virke.`)) + if (!confirm(t('tokens.revokeQuestion', { name: token.name }))) return; const previous = tokens; diff --git a/web/src/routes/projekt/[id]/+page.svelte b/web/src/routes/projekt/[id]/+page.svelte index 676e3cc..c838ae8 100644 --- a/web/src/routes/projekt/[id]/+page.svelte +++ b/web/src/routes/projekt/[id]/+page.svelte @@ -188,9 +188,9 @@ async function remove() { const days = 30; const confirmed = confirm( - `Slet "${project.name}" med alle dets opgaver?\n\n` + - `Det havner i papirkurven og kan hentes tilbage under ` + - `Indstillinger → Data og skabeloner i ${days} dage.` + t('project.deleteQuestion', { name: projectName(project) }) + + '\n\n' + + t('project.deleteRecoverable', { days }) ); if (!confirmed) return;