Repository navigation
fix(api): escape IDs in request paths, never follow redirects, hide secrets unless --show-secrets - #173
Merged
Conversation
AccountURL and LookupURL joined IDs into the path raw, so a # cut the rest off as a fragment and a ? started a query: conferences hangup 'team#2' ended conference team, and conferences stop-record '#' sent DELETE /Conference/, which ends every conference. A newline in an ID also forged extra lines in the --dry-run output. Each part is now path-escaped, so the dry-run line shows the escaped URL. A part that is blank, . or .. still names another resource (calls hangup '' built /Call//), so Do and DoMultipart refuse it with BAD_INPUT, exit 1, before anything is sent or printed.
conferences stop-record sent its DELETE without asking, unlike hangup, and before IDs were escaped a name of '#' turned it into DELETE /Conference/, which ends every conference. It now takes --yes, or --dry-run to preview, as hangup does.
No client set CheckRedirect, so Go followed redirects: a 307 re-sent the Authorization header to another port of the same host, and the request body to any host. The client from api.New now stops at a 3xx, and the event-stream client is a copy of it, so it does too, where it used to keep only the transport. Do, DoMultipart, the event stream and plivo api report the 3xx as an UPSTREAM_ERROR with its status, exit 3; returned as an answer, it read as a success with an empty result.
PLIVO_BUDDY_URL and [buddy].url were used unchecked, so ask and support sent the account's credentials, and login took its new token back, over plain http to any host. The URL must now be https, or http to this machine (127.0.0.1, ::1, localhost) for a local server. Anything else is refused with BAD_INPUT, exit 1, naming where it was set, before any request.
Secrets replaces the value of every field named *password or *auth_token in a JSON response with [REDACTED] and leaves the rest byte for byte, key order included, where Body re-marshals and sorts. Its names are narrower than Body's: responses name IDs after credentials (credential_uuid), and agent flows arrive with secrets masked by the server, which restores its own mask on a write, so a masked, empty or non-string value stays as it is. Nothing calls it yet.
-o json printed the API response whole, so subaccounts list and get printed every auth_token and new_auth_token, and endpoints get the password hash, into terminals, logs and agent transcripts. The bytes kept for -o json now go through redact.Secrets, in SetRaw, which also covers a merged --all list, and plivo api does the same with its response. The new global --show-secrets prints them. subaccounts create still prints the new subaccount's token, as that is what it is run for. Table views never showed these fields. The --schema note and plivo api help say so; help goldens and docs/COMMANDS.md are regenerated for the new flag.
The participant commands appended the participant ID to the call's URL by hand, so it missed the escaping AccountURL now does: participant kick room '#' --yes sent DELETE .../Participant/, the whole list. mpcResourceURL takes the parts below the call and passes them to AccountURL, so the ID is escaped, and a blank, . or .. one refused.
QA needs to point a release binary at a local capture server. When PLIVO_API_URL is set, it replaces the API base URL, for example http://127.0.0.1:18555/v1/cli/api. The requests carry the account's credentials, so only an http or https URL whose host is 127.0.0.1, ::1 or localhost (any port) is taken. Anything else is refused with BAD_INPUT, exit 1, before any request, rather than ignored. Unset, the CLI behaves as before. The assistant URL keeps its own rule.
agents get -o json feeds agents update --file, and the agents API masks a flow's secrets itself and restores its own mask on a write. A value the CLI hid would go back as [REDACTED] and be stored. Agent responses now keep the bytes as sent, on the agents commands and on plivo api for /AgentFlow paths, so the round trip writes back exactly what it read.
Member and participant IDs take a comma list, such as 1,2 for two members, and path escaping sent it as 1%2C2. Every character but the comma is still escaped.
The session client in the internal build followed redirects too, taking its session token along. It now returns a 3xx as the answer, which every caller already reports as an error, as it does any status outside 2xx.
The pre-check read that confirms the call or message exists swallows its errors, so a lookup hiccup does not block a diagnosis. It swallowed the refusal of a blank, . or .. id with them, and voice calls diagnose and messaging sms, mms and whatsapp diagnose then sent the assistant a turn about nothing. Client.Refused reports the refused path part, and the pre-check returns it.
The call id is glued to name_ or uuid_ before the path is built, so a blank one reached the API as the call named "name_": multiparty end '' --yes sent DELETE .../MultiPartyCall/name_/. A blank id now goes to AccountURL as it is, which refuses it before any request.
sip trunks create, the URI, credential and IP access control list creates, and the update and delete results print a body re-marshalled by mustJSON, which skipped the scrub every other -o json body gets: a password in a create response came out as sent. mustJSON now returns api.Visible of its bytes, so --show-secrets still shows it.
The path is printed as given by --dry-run, --explain and --log-level debug, so a newline in it added a line of its own, such as a forged "[dry-run] GET ..." one. A path with a control character is now refused as BAD_INPUT before anything is printed, quoted in the message.
agents list, agents runs list and agents nodes list come back as sent, as agents get does; a row each now fails if one is scrubbed. execCmd names --show-secrets among the flags it resets before a run, as it does the other global flags (resetAllFlags already reset it after one).
…hint A redirect the CLI refused to follow said only that nothing was sent, and named the assistant URL settings. Its hint now quotes the Location, says to add the trailing slash when the request path has none (plivo api GET /Message gets a 301 for it), and names PLIVO_API_URL too. Do, DoMultipart and plivo api build it from the response; the event stream and the internal build, which keep no headers, give the rest.
PLIVO_API_URL took only the exact hosts 127.0.0.1, ::1 and localhost, and the assistant URL any loopback address but only a lower-case localhost. Both now use onThisMachine: localhost in any letter case, 127.0.0.0/8 or ::1. PLIVO_API_URL is also refused with a user, a query or a fragment, since the request path is appended to it, and neither error echoes the value back, as it may hold a password.
Secrets replaced only a non-empty string, and left one containing REDACTED as it was, to keep the server's own mask on agent flows. A password sent as a number, an array or an object came out as sent, and so did one that merely contained the word. Agent flows no longer pass through Secrets, so every value of a *password or *auth_token field is now replaced whole, except null and "", which hide nothing.
The session client's event stream built a fresh http.Client, which followed redirects, and used none of the session client's settings. It now copies the session client with the timeout cleared, as the public event stream does, and reports a 3xx as an error like a 4xx.
auth token revoke glued the id to the path as typed, so a / or ? in it sent the DELETE to another path, and .. to the parent. The id is now escaped, and . and .. are refused as BAD_INPUT.
The help said auth tokens and passwords read [REDACTED] as on every other command, but /AgentFlow paths come back as sent, as agents get prints them, since the server masks their secrets and restores its mask on a write. Goldens and the command reference regenerated.
# Conflicts: # cmd/mpc.go # cmd/mpc_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Request-safety fixes from the latest QA round.
.or..ID is refused before anything is sent. A#or?in an ID used to reach a different resource:voice conferences stop-record '#'sentDELETE /Conference/(hang up all conferences) andaccount subaccounts update '#'posted to the collection. Commas stay literal for member lists.conferences stop-recordnow needs--yes.PLIVO_BUDDY_URL,[buddy].url) must be https, or http on this machine. NewPLIVO_API_URLpoints the CLI at a server on this machine only, for local request capture.[REDACTED]in every output format and inplivo api, unless--show-secretsis given.subaccounts createstill shows the new token; agent flows are left as sent so they round-trip.plivo apirefuses control characters in the path, so a newline can't forge a[dry-run]line.Testing: each fix has a test that fails without it; redirect, loopback and scrub cases run against local servers. One read-only live check:
plivo api GET /Message(no trailing slash) is a 301, which the old client followed into a 404, so nothing that worked changes.Size
Added lines by type, blank lines excluded. Comments counts comment lines in non-test code.
Removed: 87 lines. Tests include 24 comment lines.
Tests include 230 lines of regenerated help snapshots (the new global
--show-secretsline in each); hand-written tests are 234 lines.