From 38b9e5a06ef3ec41540b6091e034cfd775cc7326 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Fri, 9 Oct 2026 14:15:23 -0400 Subject: [PATCH] Tighten Google Chat webhook validation and settings response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restrict the decoded key and token to a URL-safe ASCII alphabet so the 16-character minimum and the four-character masked tail measure the same thing; a token of a few multi-byte characters could otherwise be shown in full (MQ-165-01). Validate the raw query as exactly key=…&token=…, refusing empty segments, a second "?", reordered, extra or repeated parameters (MQ-165-02). Drop configured and updated_at from the settings response so it is the four-field contract: space_label, url_masked, last_test_at, last_test_status; url_masked is null exactly when nothing is stored (MQ-165-03). Task: 919e9422-3a41-4f1f-8967-77da17f5f9f3 Co-Authored-By: Claude Opus 5.5 --- docs/ARCHITECTURE.md | 11 +++++--- internal/chat/chat.go | 12 +++------ internal/chat/chat_test.go | 4 +-- internal/chat/webhook.go | 51 +++++++++++++++++++++-------------- internal/chat/webhook_test.go | 9 ++++++- internal/server/chat_test.go | 6 ++--- 6 files changed, 55 insertions(+), 38 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 7e1ccff..54ebbbd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -693,13 +693,16 @@ Runs are recorded in `schedule_runs` and executed through `jobs`. `{"url", "space_label"}`, both required. The URL must be exactly `https://chat.googleapis.com/v1/spaces/{space}/messages?key=…&token=…`: that scheme and host spelled so, no port or user info, the space id - letters, digits, `-` or `_`, and only the query parameters `key` and - `token`, each once and at least 16 characters. The space label is 1–100 + letters, digits, `-` or `_`, and the raw query exactly + `key=…&token=…` (in that order, no empty segment, no second `?`, no + other parameter), each decoding to at least 16 characters of letters, + digits, `-`, `_`, `.`, `~` or `=`. The space label is 1–100 characters. A refused body is 400 `invalid_request` (not a JSON object of the two fields) or `invalid_webhook`. Saving replaces any stored webhook and clears its test outcome. - - `GET /api/v1/settings/chat-webhook` answers `{configured, space_label, - url_masked, last_test_at, last_test_status, updated_at}`; `url_masked` + - `GET /api/v1/settings/chat-webhook` answers `{space_label, url_masked, + last_test_at, last_test_status}`, so `url_masked` is `null` exactly when + no webhook is stored; `url_masked` shows the space id and only the last four characters of the decoded token (`…/spaces/AAQA…/messages?key=…&token=…ZxQ=`). Absent values are `null`. diff --git a/internal/chat/chat.go b/internal/chat/chat.go index 4cd09ef..ca617da 100644 --- a/internal/chat/chat.go +++ b/internal/chat/chat.go @@ -59,9 +59,8 @@ var ( ) // Settings is the stored webhook as the API shows it: never the URL, only -// its masked form. +// its masked form. URLMasked is nil when no webhook is stored. type Settings struct { - Configured bool `json:"configured"` SpaceLabel *string `json:"space_label"` // URLMasked is the webhook URL without its key and with only the last // few characters of its token. @@ -70,7 +69,6 @@ type Settings struct { // test message is sent to this webhook. LastTestAt *string `json:"last_test_at"` LastTestStatus *string `json:"last_test_status"` - UpdatedAt *string `json:"updated_at"` } // TestResult is the outcome of a test message. @@ -143,8 +141,8 @@ func NewService(st *store.Store, box *secretbox.Box, publicURL *url.URL, transpo }, nil } -// Get returns the stored webhook's settings; Configured is false and every -// other field nil before one is saved. +// Get returns the stored webhook's settings; every field is nil before one +// is saved. func (s *Service) Get(ctx context.Context) (Settings, error) { row, err := db.New(s.st.Reader).GetChatSettings(ctx) if errors.Is(err, sql.ErrNoRows) { @@ -440,14 +438,12 @@ func (s *Service) timestamp() string { } func settingsFrom(row db.ChatSetting, hook Webhook) Settings { - masked, updated := hook.Masked(), row.UpdatedAt + masked := hook.Masked() return Settings{ - Configured: true, SpaceLabel: nullString(row.SpaceLabel), URLMasked: &masked, LastTestAt: nullString(row.LastTestAt), LastTestStatus: nullString(row.LastTestStatus), - UpdatedAt: &updated, } } diff --git a/internal/chat/chat_test.go b/internal/chat/chat_test.go index ce75072..6af9988 100644 --- a/internal/chat/chat_test.go +++ b/internal/chat/chat_test.go @@ -172,7 +172,7 @@ func TestPutGetStoresSealedWebhook(t *testing.T) { svc, st := newTestService(t, newFakeChat(t)) got, err := svc.Get(ctx) - if err != nil || got.Configured || got.SpaceLabel != nil || got.URLMasked != nil || got.LastTestAt != nil { + if err != nil || got.SpaceLabel != nil || got.URLMasked != nil || got.LastTestAt != nil { t.Fatalf("Get unconfigured = %+v, %v", got, err) } @@ -180,7 +180,7 @@ func TestPutGetStoresSealedWebhook(t *testing.T) { if err != nil { t.Fatal(err) } - if !put.Configured || *put.SpaceLabel != "Family dinner" || *put.URLMasked != "https://chat.googleapis.com/v1/spaces/"+testSpace+"/messages?key=…&token=…ZxQ=" || put.LastTestAt != nil || put.LastTestStatus != nil { + if *put.SpaceLabel != "Family dinner" || *put.URLMasked != "https://chat.googleapis.com/v1/spaces/"+testSpace+"/messages?key=…&token=…ZxQ=" || put.LastTestAt != nil || put.LastTestStatus != nil { t.Errorf("Put = %+v", put) } got, err = svc.Get(ctx) diff --git a/internal/chat/webhook.go b/internal/chat/webhook.go index f42ae91..e7e08b4 100644 --- a/internal/chat/webhook.go +++ b/internal/chat/webhook.go @@ -28,6 +28,10 @@ const ( // space id. var webhookPath = regexp.MustCompile(`^/v1/spaces/([A-Za-z0-9_-]{1,128})/messages$`) +// credentialChars is the alphabet of a decoded key or token: URL-safe +// ASCII, so its length in bytes is its length in characters. +var credentialChars = regexp.MustCompile(`^[A-Za-z0-9_.~=-]*$`) + // Webhook is a validated Google Chat incoming webhook URL. Its String is // the full URL, a secret: it is only ever sealed for storage or sent to // Google Chat, never logged, shown or put in an error. @@ -39,8 +43,9 @@ type Webhook struct { // ParseWebhook validates raw as // https://chat.googleapis.com/v1/spaces/{space}/messages?key=…&token=…, -// with exactly the query parameters key and token, each once and at least -// MinCredentialLength characters. Its error never quotes raw. +// whose raw query is exactly key=…&token=…, in that order, each decoding to +// at least MinCredentialLength characters of credentialChars. Its error +// never quotes raw. func ParseWebhook(raw string) (Webhook, error) { switch { case raw == "": @@ -65,28 +70,34 @@ func ParseWebhook(raw string) (Webhook, error) { if m == nil { return Webhook{}, errors.New("path must be /v1/spaces/{space}/messages") } - q, err := url.ParseQuery(u.RawQuery) - if err != nil { - return Webhook{}, errors.New("query is not valid") + // The raw query is exactly key=…&token=…: no empty segment, no second + // "?", no other or repeated parameter. + if strings.Contains(u.RawQuery, "?") { + return Webhook{}, errors.New("query must not contain ?") } - for name, values := range q { - if name != "key" && name != "token" { - return Webhook{}, errors.New("query must hold only key and token") - } - if len(values) != 1 { - return Webhook{}, fmt.Errorf("query must hold %s once", name) - } + segments := strings.Split(u.RawQuery, "&") + if len(segments) != 2 { + return Webhook{}, errors.New("query must be exactly key=…&token=…") } - for _, name := range []string{"key", "token"} { - v := q.Get(name) - if v == "" { - return Webhook{}, fmt.Errorf("query must hold a %s", name) + creds := make([]string, 2) + for i, name := range []string{"key", "token"} { + escaped, ok := strings.CutPrefix(segments[i], name+"=") + if !ok { + return Webhook{}, errors.New("query must be exactly key=…&token=…") + } + v, err := url.QueryUnescape(escaped) + if err != nil { + return Webhook{}, errors.New("query is not valid") + } + if !credentialChars.MatchString(v) { + return Webhook{}, fmt.Errorf("%s must be letters, digits, -, _, ., ~ or =", name) } if len(v) < MinCredentialLength { return Webhook{}, fmt.Errorf("%s must be at least %d characters", name, MinCredentialLength) } + creds[i] = v } - return Webhook{raw: raw, space: m[1], token: q.Get("token")}, nil + return Webhook{raw: raw, space: m[1], token: creds[1]}, nil } // String returns the full webhook URL, a secret. @@ -98,7 +109,7 @@ func (w Webhook) String() string { // characters of its token, such as // "https://chat.googleapis.com/v1/spaces/AAQA1234/messages?key=…&token=…x3Dk". func (w Webhook) Masked() string { - // MinCredentialLength bytes are at least maskedTail runes. - token := []rune(w.token) - return webhookPrefix + w.space + "/messages?key=…&token=…" + string(token[len(token)-maskedTail:]) + // The token is ASCII of at least MinCredentialLength bytes, so the + // tail is maskedTail of its many characters. + return webhookPrefix + w.space + "/messages?key=…&token=…" + w.token[len(w.token)-maskedTail:] } diff --git a/internal/chat/webhook_test.go b/internal/chat/webhook_test.go index c5c4e96..6acff3e 100644 --- a/internal/chat/webhook_test.go +++ b/internal/chat/webhook_test.go @@ -16,7 +16,6 @@ func TestParseWebhookAccepts(t *testing.T) { t.Parallel() for _, raw := range []string{ testHook, - "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?token=" + testToken + "&key=" + testKey, "https://chat.googleapis.com/v1/spaces/AAAA_x-y/messages?key=" + testKey + "&token=" + testToken, } { hook, err := ParseWebhook(raw) @@ -55,6 +54,14 @@ func TestParseWebhookRejects(t *testing.T) { "no token": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?key=" + testKey, "empty token": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?key=" + testKey + "&token=", "short key": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?key=abc&token=" + testToken, + "token first": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?token=" + testToken + "&key=" + testKey, + "empty segment": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?key=" + testKey + "&&token=" + testToken, + "leading ampersand": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?&" + q[1:], + "trailing ampersand": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages" + q + "&", + "second question": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages" + q + "?x", + "multi-byte token": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?key=" + testKey + "&token=" + strings.Repeat("%F0%9F%98%80", 4), + "multi-byte key": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?key=" + strings.Repeat("%F0%9F%98%80", 4) + "&token=" + testToken, + "plus in token": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages?key=" + testKey + "&token=" + testToken + "+", "repeated key": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages" + q + "&key=" + testKey, "extra parameter": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages" + q + "&threadKey=x", "bad escape in query": "https://chat.googleapis.com/v1/spaces/" + testSpace + "/messages" + q + "%zz", diff --git a/internal/server/chat_test.go b/internal/server/chat_test.go index 14ca2c6..1218c6c 100644 --- a/internal/server/chat_test.go +++ b/internal/server/chat_test.go @@ -131,7 +131,7 @@ func TestChatWebhookAPI(t *testing.T) { } rec, body := call(http.MethodGet, chatWebhookPath, "") - if rec.Code != http.StatusOK || body["configured"] != false || body["url_masked"] != nil || body["space_label"] != nil { + if rec.Code != http.StatusOK || body["url_masked"] != nil || body["space_label"] != nil || body["last_test_at"] != nil || body["last_test_status"] != nil || len(body) != 4 { t.Errorf("GET unconfigured = %d %v", rec.Code, body) } rec, body = call(http.MethodPost, chatWebhookTestPath, "") @@ -160,8 +160,8 @@ func TestChatWebhookAPI(t *testing.T) { rec, body = call(http.MethodPut, chatWebhookPath, `{"url":"`+testChatHook+`","space_label":"Family dinner"}`) wantMasked := "https://chat.googleapis.com/v1/spaces/AAQA1b2C3d4/messages?key=…&token=…ZxQ=" - if rec.Code != http.StatusOK || body["configured"] != true || body["space_label"] != "Family dinner" || body["url_masked"] != wantMasked || - body["last_test_at"] != nil || body["last_test_status"] != nil { + if rec.Code != http.StatusOK || body["space_label"] != "Family dinner" || body["url_masked"] != wantMasked || + body["last_test_at"] != nil || body["last_test_status"] != nil || len(body) != 4 { t.Errorf("PUT = %d %v", rec.Code, body) } if _, ok := body["url"]; ok {