From 95a78f7002641b66067c1d498821a7b4acc13e11 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 20:13:57 -0400 Subject: [PATCH 1/6] Test URLs with segments past the item they name The router's structural fallback matches a path prefix, so these resolve to the parent item today: an occurrence edit page loses its date, and a document in a folder resolves to the folder. --- internal/urlarg/urlarg_trailing_test.go | 120 ++++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 internal/urlarg/urlarg_trailing_test.go diff --git a/internal/urlarg/urlarg_trailing_test.go b/internal/urlarg/urlarg_trailing_test.go new file mode 100644 index 000000000..ceea3d354 --- /dev/null +++ b/internal/urlarg/urlarg_trailing_test.go @@ -0,0 +1,120 @@ +package urlarg + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// A URL with segments past an item names the item only through a path bc3 +// serves for it. Anything else is refused, never resolved to the parent. +func TestParseTrailingSegments(t *testing.T) { + const base = "https://app.basecamp.com/99999/buckets/123" + tests := []struct { + name string + input string + want *Parsed + }{ + { + name: "occurrence edit page keeps the occurrence date", + input: base + "/schedule_entries/456/occurrences/20261022/edit", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "schedule_entries", RecordingID: "456", OccurrenceDate: "20261022"}, + }, + { + name: "document in a folder names the document", + input: base + "/vaults/111/documents/456", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "documents", RecordingID: "456"}, + }, + { + name: "document edit page in a folder names the document", + input: base + "/vaults/111/documents/456/edit", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "documents", RecordingID: "456"}, + }, + { + name: "upload in a folder names the upload", + input: base + "/vaults/111/uploads/456", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "Google document in a folder names the Google document", + input: base + "/vaults/111/google_documents/456", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "google_documents", RecordingID: "456"}, + }, + { + name: "cloud file in a folder names the cloud file", + input: base + "/vaults/111/cloud_files/456", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "cloud_files", RecordingID: "456"}, + }, + { + name: "folder in a folder names the inner folder", + input: base + "/vaults/111/vaults/222", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "vaults", RecordingID: "222"}, + }, + { + name: "to-do in a to-do list names the to-do", + input: base + "/todolists/111/todos/456", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "todos", RecordingID: "456"}, + }, + { + name: "question in a questionnaire names the question", + input: base + "/questionnaires/111/questions/456", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "questions", RecordingID: "456"}, + }, + { + name: "chat line edit page names the line, not the chat", + input: base + "/chats/789/lines/111/edit", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "lines", RecordingID: "111", CampfireID: "789"}, + }, + { + name: "to-do edit page names the to-do", + input: base + "/todos/789/edit", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "todos", RecordingID: "789"}, + }, + { + name: "edit page with a trailing slash, query and comment fragment", + input: base + "/todos/789/edit/?x=1#__recording_5", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "todos", RecordingID: "789", CommentID: "5"}, + }, + { + name: "card edit page names the card", + input: base + "/card_tables/cards/789/edit", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "cards", RecordingID: "789"}, + }, + { + name: "upload download link names the upload", + input: base + "/uploads/456/download/Q3%20report.pdf", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "person edit page names the person", + input: "https://app.basecamp.com/99999/people/5/edit", + want: &Parsed{AccountID: "99999", Type: "people", RecordingID: "5"}, + }, + {name: "unknown page under a to-do", input: base + "/todos/789/unknown"}, + {name: "occurrence status path", input: base + "/schedule_entries/456/occurrences/20261022/status/trashed"}, + {name: "new-document page in a folder", input: base + "/vaults/111/documents/new"}, + {name: "folder child the parser does not model", input: base + "/vaults/111/children/222"}, + {name: "answer under a question", input: base + "/questions/111/answers/2"}, + {name: "boost under a message", input: base + "/messages/789/boosts/5"}, + {name: "step under a card", input: base + "/card_tables/cards/789/steps/5"}, + {name: "upload version download", input: base + "/uploads/456/versions/7/download/a.pdf"}, + {name: "bucket without a resource", input: base}, + {name: "unknown page under a project", input: "https://app.basecamp.com/99999/projects/123/unknown"}, + {name: "unknown account-level page", input: "https://app.basecamp.com/99999/whatever/5/deep"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, Parse(tt.input)) + }) + } +} + +// A refused URL is still a URL, so callers report it as one rather than +// treating it as a name or an ID. +func TestRefusedURLIsStillURL(t *testing.T) { + url := "https://app.basecamp.com/99999/buckets/123/vaults/111/children/222" + assert.True(t, IsURL(url)) + assert.Nil(t, Parse(url)) + assert.Equal(t, url, ExtractID(url)) +} From ae837d610840e26b19b29b7f92e71f8a31304ab7 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 20:13:57 -0400 Subject: [PATCH 2/6] Refuse URLs with segments past the item they name The SDK router's structural fallback matches a path prefix, so a URL with more segments resolved to the item they sit under. Parse now rewrites the longer paths bc3 serves for an item (nested children, edit pages, upload downloads) to the item's own path, and returns nil for a structural match that leaves segments unaccounted for. --- internal/urlarg/urlarg.go | 55 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/internal/urlarg/urlarg.go b/internal/urlarg/urlarg.go index 366cad4ca..cf2158ec1 100644 --- a/internal/urlarg/urlarg.go +++ b/internal/urlarg/urlarg.go @@ -22,6 +22,23 @@ var pingAtRe = regexp.MustCompile(`/circles/(\d+)@(\d+)`) // Basecamp links to for the same Ping, and drops the my/sidebar prefix. var pingSidebarRe = regexp.MustCompile(`/my/sidebar(/circles/\d+)`) +// itemPaths are the longer paths bc3 serves for an item, each rewritten to the +// item's own path as ${1}${2}: a child nested under its parent, which bc3 also +// serves at top level, an upload's download link, and an edit page. Nested +// children come first so that their edit pages reduce to the child. +var itemPaths = []*regexp.Regexp{ + regexp.MustCompile(`/vaults/\d+(/(?:vaults|documents|google_documents|cloud_files|uploads)/\d+)([/?#]|$)`), + regexp.MustCompile(`/todolists/\d+(/todos/\d+)([/?#]|$)`), + regexp.MustCompile(`/questionnaires/\d+(/questions/\d+)([/?#]|$)`), + regexp.MustCompile(`(/uploads/\d+)/download/[^?#]+([?#]|$)`), + regexp.MustCompile(`(/\d+)/edit/?([?#]|$)`), +} + +// structuralPath is the whole path of each shape the router's structural +// fallback recognizes. The fallback matches only a prefix of the path, so a URL +// with more segments would otherwise name the item those segments sit under. +var structuralPath = regexp.MustCompile(`^/\d+(?:/projects/\d+|/buckets/\d+/(?:card_tables/[^/]+/\d+|[^/]+(?:/\d+)?))?$`) + // Parsed represents components extracted from a Basecamp URL. type Parsed struct { AccountID string @@ -45,8 +62,9 @@ func IsURL(input string) bool { return router.Match(normalized) != nil } -// normalize rewrites the web @-forms into routable URLs. Chat lines become the -// slash form; a Ping line loses its @line, which the second result returns. +// normalize rewrites the web @-forms and itemPaths into routable URLs. Chat +// lines become the slash form; a Ping line loses its @line, which the second +// result returns. func normalize(input string) (normalized, pingLineID string) { normalized = chatAtRe.ReplaceAllString(input, "${1}/lines/${2}") normalized = pingSidebarRe.ReplaceAllString(normalized, "${1}") @@ -54,11 +72,36 @@ func normalize(input string) (normalized, pingLineID string) { pingLineID = m[2] normalized = pingAtRe.ReplaceAllString(normalized, "/circles/${1}") } + for _, re := range itemPaths { + normalized = re.ReplaceAllString(normalized, "${1}${2}") + } return normalized, pingLineID } +// urlPath returns the path the router matches: the URL without its scheme, +// host, query, fragment, trailing slash, or .json suffix. +func urlPath(rawURL string) string { + path, _, _ := strings.Cut(rawURL, "#") + path, _, _ = strings.Cut(path, "?") + if _, afterScheme, ok := strings.Cut(path, "://"); ok { + path = afterScheme + } else { + path = strings.TrimPrefix(path, "//") + } + if !strings.HasPrefix(path, "/") { + slash := strings.Index(path, "/") + if slash == -1 { + return "" + } + path = path[slash:] + } + return strings.TrimSuffix(strings.TrimRight(path, "/"), ".json") +} + // Parse extracts IDs from a Basecamp URL. -// Returns nil if the input is not a valid Basecamp URL. +// Returns nil if the input is not a valid Basecamp URL, or if its path goes +// past the item it names in a way Parse does not model, so that callers refuse +// it rather than act on the parent item. // // Supported URL patterns: // - https://3.basecamp.com/{account}/buckets/{bucket}/{type}/{id} @@ -69,6 +112,9 @@ func normalize(input string) (normalized, pingLineID string) { // - https://3.basecamp.com/{account}/circles/{ping}@{lineId} // - https://3.basecamp.com/{account}/my/sidebar/circles/{ping}, with or without @{lineId} // - https://3.basecamp.com/{account}/buckets/{bucket}/schedule_entries/{id}/occurrences/{date} +// - https://3.basecamp.com/{account}/buckets/{bucket}/vaults/{folder}/documents/{id}, and the +// other children bc3 nests under folders, to-do lists, and questionnaires +// - any of these followed by /edit, and an upload followed by /download/{filename} // - https://3.basecamp.com/{account}/projects/{project} func Parse(input string) *Parsed { // Normalize the Basecamp web @-forms (chats/789@111, circles/456@111) @@ -79,6 +125,9 @@ func Parse(input string) *Parsed { if m == nil { return nil } + if m.Source == basecamp.MatchedStructural && !structuralPath.MatchString(urlPath(normalized)) { + return nil + } if pingLineID != "" { pingID := m.Params["pingId"] From 9bc74ddf7b884693ac7c76caf790c8c6327f7be7 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 20:13:57 -0400 Subject: [PATCH 3/6] Cover nested and over-long URLs in url parse e2e --- e2e/url.bats | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/e2e/url.bats b/e2e/url.bats index 250253e94..2e0f5d400 100644 --- a/e2e/url.bats +++ b/e2e/url.bats @@ -133,6 +133,19 @@ load test_helper assert_output_contains "ID required" } +@test "basecamp url parse names a document in a folder, not the folder" { + run basecamp url parse "https://3.basecamp.com/123/buckets/456/vaults/111/documents/789" --json + assert_success + assert_json_value ".data.type" "documents" + assert_json_value ".data.recording_id" "789" +} + +@test "basecamp url parse refuses a path past the item it names" { + run basecamp url parse "https://3.basecamp.com/123/buckets/456/todos/789/unknown" + assert_failure + assert_output_contains "Could not parse URL" +} + @test "basecamp url parse fails for non-Basecamp URL" { run basecamp url parse "https://github.com/test/repo" assert_failure From eddc55d80d47ffa34a4688747a6a462b9617ba96 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 20:32:06 -0400 Subject: [PATCH 4/6] Keep versioned upload links resolving to the upload bc3 links an upload's download and preview to a version, even the latest, as uploads/{id}/versions/{n}/download/{file} and uploads/{id}/versions/{n}/representations/{key}. Main resolved these to the upload; the trailing-segment check refused them, so pasting a download link into `files show` failed. Rewrite them, and the new-version page, to the upload. uploads/{id}/versions/{n} on its own is not a page bc3 serves and stays refused. --- internal/urlarg/urlarg.go | 12 ++++++++---- internal/urlarg/urlarg_trailing_test.go | 22 +++++++++++++++++++++- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/internal/urlarg/urlarg.go b/internal/urlarg/urlarg.go index cf2158ec1..18cd1df1a 100644 --- a/internal/urlarg/urlarg.go +++ b/internal/urlarg/urlarg.go @@ -24,13 +24,16 @@ var pingSidebarRe = regexp.MustCompile(`/my/sidebar(/circles/\d+)`) // itemPaths are the longer paths bc3 serves for an item, each rewritten to the // item's own path as ${1}${2}: a child nested under its parent, which bc3 also -// serves at top level, an upload's download link, and an edit page. Nested -// children come first so that their edit pages reduce to the child. +// serves at top level, an upload's download, preview and new-version links, and +// an edit page. Nested children come first so that their edit pages reduce to +// the child. bc3 links a download or preview to a version even when it is the +// latest, so a version's links name the upload. var itemPaths = []*regexp.Regexp{ regexp.MustCompile(`/vaults/\d+(/(?:vaults|documents|google_documents|cloud_files|uploads)/\d+)([/?#]|$)`), regexp.MustCompile(`/todolists/\d+(/todos/\d+)([/?#]|$)`), regexp.MustCompile(`/questionnaires/\d+(/questions/\d+)([/?#]|$)`), - regexp.MustCompile(`(/uploads/\d+)/download/[^?#]+([?#]|$)`), + regexp.MustCompile(`(/uploads/\d+)(?:/versions/\d+)?/download/[^?#]+([?#]|$)`), + regexp.MustCompile(`(/uploads/\d+)/versions/(?:\d+/representations/[^/?#]+|new)/?([?#]|$)`), regexp.MustCompile(`(/\d+)/edit/?([?#]|$)`), } @@ -114,7 +117,8 @@ func urlPath(rawURL string) string { // - https://3.basecamp.com/{account}/buckets/{bucket}/schedule_entries/{id}/occurrences/{date} // - https://3.basecamp.com/{account}/buckets/{bucket}/vaults/{folder}/documents/{id}, and the // other children bc3 nests under folders, to-do lists, and questionnaires -// - any of these followed by /edit, and an upload followed by /download/{filename} +// - any of these followed by /edit, and an upload followed by /download/{filename}, +// /versions/new, or a version's /download/{filename} or /representations/{key} // - https://3.basecamp.com/{account}/projects/{project} func Parse(input string) *Parsed { // Normalize the Basecamp web @-forms (chats/789@111, circles/456@111) diff --git a/internal/urlarg/urlarg_trailing_test.go b/internal/urlarg/urlarg_trailing_test.go index ceea3d354..2fc32a8a7 100644 --- a/internal/urlarg/urlarg_trailing_test.go +++ b/internal/urlarg/urlarg_trailing_test.go @@ -85,6 +85,26 @@ func TestParseTrailingSegments(t *testing.T) { input: base + "/uploads/456/download/Q3%20report.pdf", want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, }, + { + name: "upload version download link names the upload", + input: base + "/uploads/456/versions/7/download/a.pdf", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "upload version download link with a query names the upload", + input: base + "/uploads/456/versions/7/download/Q3%20report.pdf?disposition=attachment", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "upload version preview names the upload", + input: base + "/uploads/456/versions/7/representations/eyJfcmFpbHMiOnt9fQ==--a1b2", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "new-version page names the upload", + input: base + "/uploads/456/versions/new", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, { name: "person edit page names the person", input: "https://app.basecamp.com/99999/people/5/edit", @@ -97,7 +117,7 @@ func TestParseTrailingSegments(t *testing.T) { {name: "answer under a question", input: base + "/questions/111/answers/2"}, {name: "boost under a message", input: base + "/messages/789/boosts/5"}, {name: "step under a card", input: base + "/card_tables/cards/789/steps/5"}, - {name: "upload version download", input: base + "/uploads/456/versions/7/download/a.pdf"}, + {name: "upload version page bc3 does not serve", input: base + "/uploads/456/versions/7"}, {name: "bucket without a resource", input: base}, {name: "unknown page under a project", input: "https://app.basecamp.com/99999/projects/123/unknown"}, {name: "unknown account-level page", input: "https://app.basecamp.com/99999/whatever/5/deep"}, From c5da2a5baba5ab9b7e2dffbfe3ad0ad1e87ba508 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 20:50:42 -0400 Subject: [PATCH 5/6] Refuse a URL only when its extra path names another item Refusing every structural match not in a list of known bc3 forms broke real links that main resolved, such as todos/{id}/dates/edit and comments/{id}/compositions/edit. Segments after an item's ID now name that item unless one of them is another ID: Parse routes the URL again without them, and refuses it only if an ID is still left over. That replaces the /edit and versions/new rewrites. A .json suffix is dropped before the nested-child rewrites, so vaults/{id}/documents/{id}.json names the document. API routes that end in a person's ID (questions/{id}/answers/by/{person}, people/{person}/out_of_office, the reports by person) or an embed's sgid are refused, since that ID is not a recording. --- e2e/url.bats | 4 +- internal/urlarg/urlarg.go | 68 ++++++++++++++-------- internal/urlarg/urlarg_trailing_test.go | 75 +++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 30 deletions(-) diff --git a/e2e/url.bats b/e2e/url.bats index 2e0f5d400..488933d59 100644 --- a/e2e/url.bats +++ b/e2e/url.bats @@ -140,8 +140,8 @@ load test_helper assert_json_value ".data.recording_id" "789" } -@test "basecamp url parse refuses a path past the item it names" { - run basecamp url parse "https://3.basecamp.com/123/buckets/456/todos/789/unknown" +@test "basecamp url parse refuses a path to another item under the one it names" { + run basecamp url parse "https://3.basecamp.com/123/buckets/456/messages/789/boosts/5" assert_failure assert_output_contains "Could not parse URL" } diff --git a/internal/urlarg/urlarg.go b/internal/urlarg/urlarg.go index 18cd1df1a..9acfd3f04 100644 --- a/internal/urlarg/urlarg.go +++ b/internal/urlarg/urlarg.go @@ -22,26 +22,31 @@ var pingAtRe = regexp.MustCompile(`/circles/(\d+)@(\d+)`) // Basecamp links to for the same Ping, and drops the my/sidebar prefix. var pingSidebarRe = regexp.MustCompile(`/my/sidebar(/circles/\d+)`) -// itemPaths are the longer paths bc3 serves for an item, each rewritten to the -// item's own path as ${1}${2}: a child nested under its parent, which bc3 also -// serves at top level, an upload's download, preview and new-version links, and -// an edit page. Nested children come first so that their edit pages reduce to -// the child. bc3 links a download or preview to a version even when it is the -// latest, so a version's links name the upload. +// jsonSuffixRe matches the .json an API URL's path ends in. +var jsonSuffixRe = regexp.MustCompile(`^([^?#]*)\.json([?#]|$)`) + +// itemPaths are paths whose segments past an item name another item, each +// rewritten to that item's own path as ${1}${2}: a child nested under its +// parent, which bc3 also serves at top level, and an upload's download and +// preview links. bc3 links those to a version even when it is the latest, so +// they name the upload; a download's filename, which may be all digits, goes +// with them. var itemPaths = []*regexp.Regexp{ regexp.MustCompile(`/vaults/\d+(/(?:vaults|documents|google_documents|cloud_files|uploads)/\d+)([/?#]|$)`), regexp.MustCompile(`/todolists/\d+(/todos/\d+)([/?#]|$)`), regexp.MustCompile(`/questionnaires/\d+(/questions/\d+)([/?#]|$)`), regexp.MustCompile(`(/uploads/\d+)(?:/versions/\d+)?/download/[^?#]+([?#]|$)`), - regexp.MustCompile(`(/uploads/\d+)/versions/(?:\d+/representations/[^/?#]+|new)/?([?#]|$)`), - regexp.MustCompile(`(/\d+)/edit/?([?#]|$)`), + regexp.MustCompile(`(/uploads/\d+)/versions/\d+/representations/[^/?#]+/?([?#]|$)`), } // structuralPath is the whole path of each shape the router's structural -// fallback recognizes. The fallback matches only a prefix of the path, so a URL -// with more segments would otherwise name the item those segments sit under. +// fallback recognizes. The fallback matches only a prefix of the path. var structuralPath = regexp.MustCompile(`^/\d+(?:/projects/\d+|/buckets/\d+/(?:card_tables/[^/]+/\d+|[^/]+(?:/\d+)?))?$`) +// trailingWords matches the segments after a path's last ID when none of them +// is an ID: a page of that item, such as edit or dates/edit. +var trailingWords = regexp.MustCompile(`(/\d+)(?:/[^/?#]*[^/?#0-9][^/?#]*)+/?([?#]|$)`) + // Parsed represents components extracted from a Basecamp URL. type Parsed struct { AccountID string @@ -65,9 +70,9 @@ func IsURL(input string) bool { return router.Match(normalized) != nil } -// normalize rewrites the web @-forms and itemPaths into routable URLs. Chat -// lines become the slash form; a Ping line loses its @line, which the second -// result returns. +// normalize rewrites the web @-forms and itemPaths into routable URLs, and +// drops a .json suffix. Chat lines become the slash form; a Ping line loses its +// @line, which the second result returns. func normalize(input string) (normalized, pingLineID string) { normalized = chatAtRe.ReplaceAllString(input, "${1}/lines/${2}") normalized = pingSidebarRe.ReplaceAllString(normalized, "${1}") @@ -75,6 +80,7 @@ func normalize(input string) (normalized, pingLineID string) { pingLineID = m[2] normalized = pingAtRe.ReplaceAllString(normalized, "/circles/${1}") } + normalized = jsonSuffixRe.ReplaceAllString(normalized, "${1}${2}") for _, re := range itemPaths { normalized = re.ReplaceAllString(normalized, "${1}${2}") } @@ -82,7 +88,7 @@ func normalize(input string) (normalized, pingLineID string) { } // urlPath returns the path the router matches: the URL without its scheme, -// host, query, fragment, trailing slash, or .json suffix. +// host, query, fragment, or trailing slash. func urlPath(rawURL string) string { path, _, _ := strings.Cut(rawURL, "#") path, _, _ = strings.Cut(path, "?") @@ -98,13 +104,28 @@ func urlPath(rawURL string) string { } path = path[slash:] } - return strings.TrimSuffix(strings.TrimRight(path, "/"), ".json") + return strings.TrimRight(path, "/") +} + +// match routes a normalized URL, refusing a structural match that leaves an ID +// unconsumed. Words after the item's ID name the item: the URL is routed again +// without them, so a page of an API item, such as a chat line's edit page, +// still names that item. +func match(normalized string) *basecamp.Match { + for _, u := range []string{normalized, trailingWords.ReplaceAllString(normalized, "${1}${2}")} { + m := router.Match(u) + if m == nil || m.Source == basecamp.MatchedAPI || structuralPath.MatchString(urlPath(u)) { + return m + } + } + return nil } // Parse extracts IDs from a Basecamp URL. -// Returns nil if the input is not a valid Basecamp URL, or if its path goes -// past the item it names in a way Parse does not model, so that callers refuse -// it rather than act on the parent item. +// Returns nil if the input is not a valid Basecamp URL, if its path goes past +// the item it names to another one Parse does not model, or if the ID it ends +// in is a person's or an embed's, so that callers refuse it rather than act on +// the wrong recording. // // Supported URL patterns: // - https://3.basecamp.com/{account}/buckets/{bucket}/{type}/{id} @@ -117,19 +138,22 @@ func urlPath(rawURL string) string { // - https://3.basecamp.com/{account}/buckets/{bucket}/schedule_entries/{id}/occurrences/{date} // - https://3.basecamp.com/{account}/buckets/{bucket}/vaults/{folder}/documents/{id}, and the // other children bc3 nests under folders, to-do lists, and questionnaires -// - any of these followed by /edit, and an upload followed by /download/{filename}, -// /versions/new, or a version's /download/{filename} or /representations/{key} +// - any of these followed by segments with no ID, such as /edit, and an upload +// followed by /download/{filename} or a version's /download/{filename} or +// /representations/{key} // - https://3.basecamp.com/{account}/projects/{project} func Parse(input string) *Parsed { // Normalize the Basecamp web @-forms (chats/789@111, circles/456@111) // into routable URLs before routing. normalized, pingLineID := normalize(input) - m := router.Match(normalized) + m := match(normalized) if m == nil { return nil } - if m.Source == basecamp.MatchedStructural && !structuralPath.MatchString(urlPath(normalized)) { + // questions/{id}/answers/by/{person}, people/{person}/out_of_office and the + // reports by person end in a person's ID, and embeds/{sgid} in an sgid. + if m.Params["sgid"] != "" || (m.Params["personId"] != "" && m.PathType != "people") { return nil } diff --git a/internal/urlarg/urlarg_trailing_test.go b/internal/urlarg/urlarg_trailing_test.go index 2fc32a8a7..572e844ce 100644 --- a/internal/urlarg/urlarg_trailing_test.go +++ b/internal/urlarg/urlarg_trailing_test.go @@ -6,8 +6,9 @@ import ( "github.com/stretchr/testify/assert" ) -// A URL with segments past an item names the item only through a path bc3 -// serves for it. Anything else is refused, never resolved to the parent. +// Segments past an item's ID name that item unless one of them is another ID. +// Then the URL names that other item only through a path Parse knows, and is +// otherwise refused, never resolved to the parent. func TestParseTrailingSegments(t *testing.T) { const base = "https://app.basecamp.com/99999/buckets/123" tests := []struct { @@ -110,16 +111,78 @@ func TestParseTrailingSegments(t *testing.T) { input: "https://app.basecamp.com/99999/people/5/edit", want: &Parsed{AccountID: "99999", Type: "people", RecordingID: "5"}, }, - {name: "unknown page under a to-do", input: base + "/todos/789/unknown"}, - {name: "occurrence status path", input: base + "/schedule_entries/456/occurrences/20261022/status/trashed"}, - {name: "new-document page in a folder", input: base + "/vaults/111/documents/new"}, + { + name: "to-do dates edit page names the to-do", + input: base + "/todos/789/dates/edit", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "todos", RecordingID: "789"}, + }, + { + name: "comment editor page names the comment", + input: base + "/comments/789/compositions/edit", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "comments", RecordingID: "789"}, + }, + { + name: "unknown page under a to-do names the to-do", + input: base + "/todos/789/unknown", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "todos", RecordingID: "789"}, + }, + { + name: "occurrence status path keeps the occurrence date", + input: base + "/schedule_entries/456/occurrences/20261022/status/trashed", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "schedule_entries", RecordingID: "456", OccurrenceDate: "20261022"}, + }, + { + name: "new-document page in a folder names the folder", + input: base + "/vaults/111/documents/new", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "vaults", RecordingID: "111"}, + }, + { + name: "unknown page under a project names the project", + input: "https://app.basecamp.com/99999/projects/123/unknown", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "project"}, + }, + { + name: "JSON document in a folder names the document", + input: base + "/vaults/111/documents/222.json", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "documents", RecordingID: "222"}, + }, + { + name: "JSON document in a folder with a comment fragment names the comment", + input: base + "/vaults/111/documents/222.json#__recording_789", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "documents", RecordingID: "222", CommentID: "789"}, + }, + { + name: "download link with digits in the filename names the upload", + input: base + "/uploads/456/download/report2024.pdf", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "download link with an all-digit filename names the upload", + input: base + "/uploads/456/download/12345", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "download link with an all-digit filename and extension names the upload", + input: base + "/uploads/456/download/12345.pdf", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "version download link with an all-digit filename names the upload", + input: base + "/uploads/456/versions/7/download/12345", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + {name: "upload version as JSON", input: base + "/uploads/456/versions/7.json"}, + {name: "answers by a person end in the person's ID", input: base + "/questions/111/answers/by/789"}, + {name: "a person's out-of-office ends in the person's ID", input: "https://app.basecamp.com/99999/people/5/out_of_office"}, + {name: "to-dos assigned to a person end in the person's ID", input: "https://app.basecamp.com/99999/reports/todos/assigned/5"}, + {name: "a person's progress ends in the person's ID", input: "https://app.basecamp.com/99999/reports/users/progress/5"}, + {name: "an embed ends in its sgid", input: "https://app.basecamp.com/99999/embeds/BAh7abc"}, {name: "folder child the parser does not model", input: base + "/vaults/111/children/222"}, {name: "answer under a question", input: base + "/questions/111/answers/2"}, {name: "boost under a message", input: base + "/messages/789/boosts/5"}, {name: "step under a card", input: base + "/card_tables/cards/789/steps/5"}, {name: "upload version page bc3 does not serve", input: base + "/uploads/456/versions/7"}, {name: "bucket without a resource", input: base}, - {name: "unknown page under a project", input: "https://app.basecamp.com/99999/projects/123/unknown"}, {name: "unknown account-level page", input: "https://app.basecamp.com/99999/whatever/5/deep"}, } From 960bb086a7f79b05e66e35fab25c75119894c90f Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Thu, 22 Oct 2026 21:07:12 -0400 Subject: [PATCH 6/6] Treat an ID with a suffix as an ID when trimming URL paths vaults/{id}/documents/222.json/ resolved to the folder. The trailing slash hid the .json from the suffix drop and the child rewrite, and the trailing-segment trim read 222.json as a word and dropped it. The same gap let uploads/{id}/versions/7.json/ resolve to the upload, and refused card_tables/lists/222.json/, which main resolves. Parse now collapses runs of slashes in the path and drops a trailing one, as Rails does when routing, before any other rewrite. And a segment after an ID counts as a word only if it starts with a letter, so one that starts with digits, a percent escape or a sign, such as 222.json, 222.html or %32%32%32, is never trimmed: the URL is refused rather than resolved to the parent. Download filenames are unaffected, because the download rewrite consumes them first. --- internal/urlarg/urlarg.go | 40 +++++++++++++++++++------ internal/urlarg/urlarg_trailing_test.go | 40 +++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/internal/urlarg/urlarg.go b/internal/urlarg/urlarg.go index 9acfd3f04..4380eb8d2 100644 --- a/internal/urlarg/urlarg.go +++ b/internal/urlarg/urlarg.go @@ -22,6 +22,9 @@ var pingAtRe = regexp.MustCompile(`/circles/(\d+)@(\d+)`) // Basecamp links to for the same Ping, and drops the my/sidebar prefix. var pingSidebarRe = regexp.MustCompile(`/my/sidebar(/circles/\d+)`) +// slashRunRe matches a run of slashes, which Rails routes as one. +var slashRunRe = regexp.MustCompile(`/{2,}`) + // jsonSuffixRe matches the .json an API URL's path ends in. var jsonSuffixRe = regexp.MustCompile(`^([^?#]*)\.json([?#]|$)`) @@ -36,16 +39,17 @@ var itemPaths = []*regexp.Regexp{ regexp.MustCompile(`/todolists/\d+(/todos/\d+)([/?#]|$)`), regexp.MustCompile(`/questionnaires/\d+(/questions/\d+)([/?#]|$)`), regexp.MustCompile(`(/uploads/\d+)(?:/versions/\d+)?/download/[^?#]+([?#]|$)`), - regexp.MustCompile(`(/uploads/\d+)/versions/\d+/representations/[^/?#]+/?([?#]|$)`), + regexp.MustCompile(`(/uploads/\d+)/versions/\d+/representations/[^/?#]+([?#]|$)`), } // structuralPath is the whole path of each shape the router's structural // fallback recognizes. The fallback matches only a prefix of the path. var structuralPath = regexp.MustCompile(`^/\d+(?:/projects/\d+|/buckets/\d+/(?:card_tables/[^/]+/\d+|[^/]+(?:/\d+)?))?$`) -// trailingWords matches the segments after a path's last ID when none of them -// is an ID: a page of that item, such as edit or dates/edit. -var trailingWords = regexp.MustCompile(`(/\d+)(?:/[^/?#]*[^/?#0-9][^/?#]*)+/?([?#]|$)`) +// trailingWords matches the segments after a path's last ID when each starts +// with a letter: a page of that item, such as edit or dates/edit. A segment +// that starts otherwise, such as 222.json or %32%32, may hold an ID. +var trailingWords = regexp.MustCompile(`(/\d+)(?:/[A-Za-z][^/?#]*)+([?#]|$)`) // Parsed represents components extracted from a Basecamp URL. type Parsed struct { @@ -71,10 +75,10 @@ func IsURL(input string) bool { } // normalize rewrites the web @-forms and itemPaths into routable URLs, and -// drops a .json suffix. Chat lines become the slash form; a Ping line loses its -// @line, which the second result returns. +// drops a .json suffix and the slashes Rails ignores. Chat lines become the +// slash form; a Ping line loses its @line, which the second result returns. func normalize(input string) (normalized, pingLineID string) { - normalized = chatAtRe.ReplaceAllString(input, "${1}/lines/${2}") + normalized = chatAtRe.ReplaceAllString(squeezeSlashes(input), "${1}/lines/${2}") normalized = pingSidebarRe.ReplaceAllString(normalized, "${1}") if m := pingAtRe.FindStringSubmatch(normalized); m != nil { pingLineID = m[2] @@ -87,8 +91,26 @@ func normalize(input string) (normalized, pingLineID string) { return normalized, pingLineID } +// squeezeSlashes collapses each run of slashes in a URL's path to one and drops +// a trailing slash, as Rails does when routing, so that a suffix or ID before +// them still ends the path. +func squeezeSlashes(rawURL string) string { + end := strings.IndexAny(rawURL, "?#") + if end == -1 { + end = len(rawURL) + } + start := 0 + if i := strings.Index(rawURL[:end], "://"); i != -1 { + start = i + len("://") + } else if strings.HasPrefix(rawURL, "//") { + start = len("//") + } + path := strings.TrimRight(slashRunRe.ReplaceAllString(rawURL[start:end], "/"), "/") + return rawURL[:start] + path + rawURL[end:] +} + // urlPath returns the path the router matches: the URL without its scheme, -// host, query, fragment, or trailing slash. +// host, query, or fragment. func urlPath(rawURL string) string { path, _, _ := strings.Cut(rawURL, "#") path, _, _ = strings.Cut(path, "?") @@ -104,7 +126,7 @@ func urlPath(rawURL string) string { } path = path[slash:] } - return strings.TrimRight(path, "/") + return path } // match routes a normalized URL, refusing a structural match that leaves an ID diff --git a/internal/urlarg/urlarg_trailing_test.go b/internal/urlarg/urlarg_trailing_test.go index 572e844ce..a2d9eb69c 100644 --- a/internal/urlarg/urlarg_trailing_test.go +++ b/internal/urlarg/urlarg_trailing_test.go @@ -151,6 +151,41 @@ func TestParseTrailingSegments(t *testing.T) { input: base + "/vaults/111/documents/222.json#__recording_789", want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "documents", RecordingID: "222", CommentID: "789"}, }, + { + name: "JSON document in a folder with a trailing slash, query and comment fragment names the comment", + input: base + "/vaults/111/documents/222.json/?x=1#__recording_789", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "documents", RecordingID: "222", CommentID: "789"}, + }, + { + name: "JSON card column with a trailing slash, query and comment fragment names the column", + input: base + "/card_tables/lists/222.json/?x=1#__recording_789", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "columns", RecordingID: "222", CommentID: "789"}, + }, + { + name: "document in a folder after a doubled slash names the document", + input: base + "/vaults/111//documents/222?x=1#__recording_789", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "documents", RecordingID: "222", CommentID: "789"}, + }, + { + name: "to-do edit page after a doubled slash names the to-do", + input: base + "/todos/789//edit?x=1#__recording_5", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "todos", RecordingID: "789", CommentID: "5"}, + }, + { + name: "to-do ID after a doubled slash names the to-do", + input: base + "/todos//789", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "todos", RecordingID: "789"}, + }, + { + name: "download link with an all-digit filename and a trailing slash names the upload", + input: base + "/uploads/456/download/12345/?x=1#y", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, + { + name: "version download link with a digit-leading filename and a trailing slash names the upload", + input: base + "/uploads/456/versions/7/download/222.json/?disposition=attachment", + want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, + }, { name: "download link with digits in the filename names the upload", input: base + "/uploads/456/download/report2024.pdf", @@ -172,6 +207,11 @@ func TestParseTrailingSegments(t *testing.T) { want: &Parsed{AccountID: "99999", ProjectID: "123", Type: "uploads", RecordingID: "456"}, }, {name: "upload version as JSON", input: base + "/uploads/456/versions/7.json"}, + {name: "upload version as JSON with a trailing slash, query and fragment", input: base + "/uploads/456/versions/7.json/?x=1#__recording_5"}, + {name: "document in a folder with another suffix", input: base + "/vaults/111/documents/222.html?x=1#y"}, + {name: "document in a folder as uppercase JSON", input: base + "/vaults/111/documents/222.JSON"}, + {name: "document in a folder with a percent-encoded ID", input: base + "/vaults/111/documents/%32%32%32"}, + {name: "document in a folder with a signed ID", input: base + "/vaults/111/documents/+222"}, {name: "answers by a person end in the person's ID", input: base + "/questions/111/answers/by/789"}, {name: "a person's out-of-office ends in the person's ID", input: "https://app.basecamp.com/99999/people/5/out_of_office"}, {name: "to-dos assigned to a person end in the person's ID", input: "https://app.basecamp.com/99999/reports/todos/assigned/5"},