Refuse URLs with segments past the item they name

Merged
RO
2d ago
+359-4

Fixes #944.

What

urlarg.Parse no longer resolves a URL to an item its path runs past. Every command that takes <id|url> goes through it, so the fix applies to all of them at once.

The rule is about what the extra segments name:

  • Segments after an item's ID that hold no other ID name that item. A segment holds no ID only when it starts with a letter, so 222.json, 222.html or %32%32%32 is never trimmed away. Examples are todos/789/edit, todos/789/dates/edit, comments/789/compositions/edit and vaults/111/documents/new. Parse routes the URL again without those segments, so a page of an API item still names it. A chat line's edit page names the line, and an occurrence's edit page keeps its date. main already resolved most of these, because the prefix match landed on the same item.
  • Segments that hold another ID name another item. Parse resolves that item through the paths it knows, which it rewrites to the item's own path before routing:
    • a child that bc3 serves both nested and at top level: vaults/{id}/{vaults,documents,google_documents,cloud_files,uploads}/{id}, todolists/{id}/todos/{id} and questionnaires/{id}/questions/{id}. Runs of slashes in the path become one and a trailing slash is dropped, as Rails does when routing. Then a .json suffix is dropped, so vaults/111/documents/222.json/#__recording_789 names comment 789 on document 222.
    • an upload's download/{filename} link and a version's download/{filename} and representations/{key} links. bc3 links downloads and previews to a version even when it is the latest. The filename goes with the link, so an all-digit filename such as download/12345 is not read as an ID.
  • Any other path with another ID returns nil. Examples are questions/1/answers/2, messages/789/boosts/5, card_tables/cards/789/steps/5 and uploads/456/versions/7. Callers already refuse a nil result. IsURL still returns true for these URLs, so callers report a URL error and do not treat the URL as a name.
  • API routes that end in an ID that is not a recording return nil. These are questions/{id}/answers/by/{person}, people/{person}/out_of_office, reports/todos/assigned/{person}, reports/users/progress/{person} and embeds/{sgid}. On main, recordings trash with an answers-by-person URL acts on the recording that shares the person's number. people/{person} still parses as a person, because its type says so and show handles it.
URL (after /99999/buckets/123)mainThis PR
/schedule_entries/456/occurrences/20261022/editentry 456, no dateentry 456, occurrence 20261022
/vaults/111/documents/456folder 111document 456
/chats/789/lines/111/editchat 789line 111
/todos/789/dates/editto-do 789to-do 789
/vaults/111/children/222folder 111nil
/questions/111/answers/by/789recording 789 (a person)nil

Why

The SDK router tries its API routes first, and those match the whole path. When none matches, it falls back to structural patterns that match only the start of the path and drop the rest. The fault is in the SDK, filed as basecamp/basecamp-sdk#1036. This PR adds the check at the CLI's single entry point, so the CLI does not wait for an SDK release.

An earlier version of this PR refused every structural match not in a list of known bc3 forms. Review kept finding real bc3 links that main resolved and that list refused, such as dates/edit and compositions/edit. The list could not be finished. Whether the extra segments hold an ID can be checked on any URL.

On main, against an unreachable server:

  • basecamp recordings trash <chat line edit URL> sends PUT /recordings/789/status/trashed.json, which trashes the whole chat.
  • basecamp spotlight <occurrence edit URL> spotlights the whole recurring series.

On this branch, the first command trashes line 111 and the second is refused with the existing occurrence message.

Not included

  • basecamp url parse does not print the occurrence date. That was already true for plain occurrence URLs.
  • A refused URL reaches commands that use ExtractID as the raw string. The command then fails with its existing "Invalid ID" error, not a message that names the URL.
  • API routes whose ID is not a recording but whose type names it, such as webhooks/{id}, templates/{id} and categories/{id}, parse as before. A command that ignores the type can still act on the recording that shares the number, as on main.
  • An account-level page with no ID after the account, such as my/profile/edit, still parses as the account, as on main.

Testing

  • internal/urlarg/urlarg_trailing_test.go has 56 table cases that compare the whole Parsed result. Against main's parser, 35 fail. The 21 that pass are forms where the prefix match gives the right item, and they check that the fix does not refuse them. Against eddc55d8, 17 fail. Against c5da2a5b, 10 fail: a trailing slash after .json, a doubled slash, and a nested ID with a suffix, a percent escape or a sign.
  • Removing any one part of the change fails at least 3 cases: the download rewrite, the .json drop, the person and sgid check, the second routing without the trailing words, the slash collapse (6) or the letter-start rule for a trimmed segment (4).
  • e2e/url.bats: url parse names a document in a folder and refuses messages/789/boosts/5.
  • I ran 141 URLs through Parse on main and on this branch. 54 results changed. In 30 the result went from the parent, an empty result or nil to the item itself, the child, line, person or Ping, or the occurrence date came back. In 24 a URL that named a different item now returns nil. None went from the right item to nil, and none resolves to an item other than the one the URL names. The other 87 did not change.
  • bin/ci passes

Summary by cubic

Fixes #944 so urlarg.Parse no longer resolves a URL to the parent item when its path runs past the item. Every command that takes <id|url> goes through Parse, so the chat line edit URL now trashes the line, not the whole chat, and an occurrence edit URL is refused rather than spotlighting the whole series.

Behavior

  • Words after an item's ID name that item, so the URL is routed again without them: todos/789/edit, todos/789/dates/edit, comments/789/compositions/edit, and unknown pages all still name the item. An occurrence's edit page keeps its date. A segment counts as a word only if it starts with a letter: one that starts with digits, a percent escape, or a sign (222.json, 222.html, %32%32%32) is treated as an ID and refused rather than resolved to the parent.
  • Siblings bc3 serves also at top level are rewritten to the item's own path: vaults/{id}/{vaults,documents,google_documents,cloud_files,uploads}/{id}, todolists/{id}/todos/{id}, and questionnaires/{id}/questions/{id}. A .json suffix is dropped before rewriting.
  • Upload download and preview links, which bc3 points at a version even when it is the latest, resolve to the upload: uploads/{id}/download/{filename}, uploads/{id}/versions/{n}/download/{filename}, uploads/{id}/versions/{n}/representations/{key}, and uploads/{id}/versions/new. A bare uploads/{id}/versions/{n} is not a page bc3 serves and stays refused.
  • Any remaining extra ID, plus paths ending in a person's ID or an embed's sgid, returns nil. Since IsURL still returns true, callers report a URL error instead of treating the URL as a name.
  • Runs of slashes in the path are collapsed and a trailing slash dropped, as Rails routes them, so a suffix or ID before them still ends the path.

The root cause is the SDK router's structural fallback matching only a path prefix (basecamp/basecamp-sdk#1036); this PR adds the check at the CLI's single entry point so it does not wait for an SDK release.