Repository navigation
test: cover curl commands, sensitive variables and logout - #235
Merged
pjcdawkins merged 3 commits intoOct 15, 2026
Merged
pjcdawkins merged 3 commits into
pjcdawkins merged 3 commits into
Conversation
Add integration tests for security-relevant behavior: - api:curl refuses a path with a host (an absolute or scheme-relative URL) without sending a request, censors the access token in verbose output, and passes -X, --json and -H to curl. - api:curl, project:curl, environment:curl and organization:curl request paths under their own base path. - Sensitive variables at project and environment level: the value is sent on create and update but not displayed by variable:get or variable:list, and a variable can be updated to be sensitive. - auth:logout of the current session revokes its tokens and deletes its files, warns when an API token is set via config, and --other keeps the current session when there are no others. - auth:api-token-login is refused when an API token is set via config. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Note
Reviewed — No blocking findings · 🔵 1 minor point
🔍 Full review · 4 files reviewed
🔵 Minor point
integration-tests/api_curl_test.go:169—assert.Empty(t, otherRecorder.requests())can never fail.CurlCli::runbuilds the URL asrtrim($baseUrl, '/') . '/' . ltrim($path, '/'), so if theparse_url($path, PHP_URL_HOST)check were removed, both test paths (http://127.0.0.1:N/stealand//127.0.0.1:N/steal) would still go to the API server, as<api>/http://127.0.0.1:N/stealand<api>/127.0.0.1:N/steal. curl would never contactotherServer. Onlyassert.Empty(t, apiRecorder.requests())and the exit-code/stderr checks would catch a regression. So the comment on line 149, which says the check keeps the token from being sent to another host, misstates what this test protects.
Verification
- The strings the tests expect ('Invalid path: ', 'The value of --json contains invalid JSON.', "Bearer [token]", 'An API token is already set via config', 'Warning: an API token is set via config') exist in both the Go commands and legacy PHP sources.
CurlCli::runreturns 1 before fetching a token for any path with a host, and the invalid-JSON exception is thrown before curl runs, so the request-count assertions after those cases hold.- The mock auth server always issues
access-token-1unless unique tokens are enabled, so theBearer access-token-1and censoring assertions match its behavior. - Go auth
auth:api-token-loginchecksHasConfiguredTokenbefore the interactive prompt, so 'Please enter an API token' is never printed when a token is configured. - The mock API redacts sensitive variable values, and
var:listwrites an empty value for them in machine-readable formats, which matches the expectedenv:SECRET,<level>,CSV row.
This change is tests only. It adds Go integration tests that run in the integration-test job of .github/workflows/ci.yml, once with legacy auth and once with TEST_CLI_GO_AUTH=1. I did not run them, because they need a built CLI binary.
Review details
- Commit: 1be1a2a
- Model: claude-opus-5-5
Review 1 of 10 for this pull request · View the full run
The path is appended to the API URL, so curl would never contact another server even without the host check. Assert only that no request is sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…itive-variables-and
pjcdawkins
enabled auto-merge (squash)
October 15, 2026 10:09
pjcdawkins
deleted the
cli-209-integration-tests-curl-commands-sensitive-variables-and
branch
October 15, 2026 10:13
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.
Adds integration tests for security-relevant behavior:
api:curlrefuses a path with a host (absolute or//host/...) with exit code 1, and sends no request to either server. The access token is shown as[token]in-vand-vvoutput.-X,--json(including invalid JSON) and-Hreach the request.api:curl,project:curl,environment:curlandorganization:curleach request under their own base path.variable:get(without-P) or by CSVvariable:listoutput. A variable can also be updated to be sensitive.auth:logoutof a single session revokes its access and refresh tokens and deletes its session and certificate files. It warns when an API token is set via config.--otherkeeps the current session when no other sessions exist.auth:api-token-loginis refused when an API token is set via config.The tests pass with both legacy auth and Go auth (
TEST_CLI_GO_AUTH=1).🤖 Generated with Claude Code