Refuse URLs with segments past the item they name
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.htmlor%32%32%32is never trimmed away. Examples aretodos/789/edit,todos/789/dates/edit,comments/789/compositions/editandvaults/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.mainalready 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}andquestionnaires/{id}/questions/{id}. Runs of slashes in the path become one and a trailing slash is dropped, as Rails does when routing. Then a.jsonsuffix is dropped, sovaults/111/documents/222.json/#__recording_789names comment 789 on document 222. - an upload's
download/{filename}link and a version'sdownload/{filename}andrepresentations/{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 asdownload/12345is not read as an ID.
- a child that bc3 serves both nested and at top level:
- Any other path with another ID returns nil. Examples are
questions/1/answers/2,messages/789/boosts/5,card_tables/cards/789/steps/5anduploads/456/versions/7. Callers already refuse a nil result.IsURLstill 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}andembeds/{sgid}. Onmain,recordings trashwith 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 andshowhandles it.
URL (after /99999/buckets/123) | main | This PR |
|---|---|---|
/schedule_entries/456/occurrences/20261022/edit | entry 456, no date | entry 456, occurrence 20261022 |
/vaults/111/documents/456 | folder 111 | document 456 |
/chats/789/lines/111/edit | chat 789 | line 111 |
/todos/789/dates/edit | to-do 789 | to-do 789 |
/vaults/111/children/222 | folder 111 | nil |
/questions/111/answers/by/789 | recording 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>sendsPUT /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 parsedoes not print the occurrence date. That was already true for plain occurrence URLs.- A refused URL reaches commands that use
ExtractIDas 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}andcategories/{id}, parse as before. A command that ignores the type can still act on the recording that shares the number, as onmain. - An account-level page with no ID after the account, such as
my/profile/edit, still parses as the account, as onmain.
Testing
internal/urlarg/urlarg_trailing_test.gohas 56 table cases that compare the wholeParsedresult. Againstmain'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
.jsondrop, 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 parsenames a document in a folder and refusesmessages/789/boosts/5.- I ran 141 URLs through
Parseonmainand 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/cipasses
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}, andquestionnaires/{id}/questions/{id}. A.jsonsuffix 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}, anduploads/{id}/versions/new. A bareuploads/{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
IsURLstill 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.