Skip to content

feat(skills): add last9-api skill with a deterministic token-handling CLI - #21

Merged
prathamesh-sonpatki merged 3 commits into
masterfrom
feat/last9-api-skill
Oct 1, 2026
Merged

prathamesh-sonpatki merged 3 commits into
masterfrom
feat/last9-api-skill

Conversation

@prathamesh-sonpatki

Copy link
Copy Markdown
Member

What

New last9-api skill for calling the Last9 REST API from scripts, CI, or an agent without the MCP server. It ships a stdlib-only CLI so the auth flow is deterministic instead of re-derived by the model each time.

python3 skills/last9-api/scripts/last9.py login --region ap-south-1   # once; hidden prompt, validated by exchange
python3 skills/last9-api/scripts/last9.py api POST logs/api/v2/query_range/json -q start=... -q end=... -d @q.json

Commands: login, token, status, logout, api.

  • The refresh token is self-describing: aud gives the host and organization_slug gives the org, so the pasted token is the only config.
  • api sets X-LAST9-API-TOKEN: Bearer <token> (the two mistakes the docs warn about), caches access tokens, retries once on expiry, and adds the required region on logs/ and cat/ paths.
  • It refuses full URLs on a foreign host and does not follow redirects, so the token cannot leak.
  • Credentials live in ~/.last9 (dir 0700, files 0600, atomic writes). LAST9_REFRESH_TOKEN is honored for CI and never written to disk. login saves nothing if validation fails.
  • References: auth, logs, traces, change events, Alertmanager migration, taken from the public docs and corrected where live behavior differed.

Review first: guardrail change

scripts/check-skill-pack.sh limited skill payloads to SKILL.md + references/*.md. This PR widens it by exactly one type, a flat skills/<name>/scripts/<file>.py, through a single shared PAYLOAD_RE (previously duplicated in two places). Symlinks, nesting, other extensions, and entrypoint-less skills are still rejected. check-skill-pack-selftest.sh gains eight cases (one happy path, seven must-fail). I confirmed the happy path fails against the old regex and that each must-fail case trips for the intended reason.

This means every npx skills add / opencode tarball install now carries a script that handles refresh tokens, so changes under any skills/*/scripts/ deserve security-level review.

Verified

  • 27 unit tests pass (no network; urlopen mocked), including 401 retry without looping, token non-leakage, file modes, and region handling.
  • check-skill-pack.sh, its selftest, and check-skill-hygiene.sh pass.
  • Live against an own-org token: exchange, cache reuse, 0700/0600 modes, login/status/logout, foreign-host refusal, and the logs and traces recipes. No writes were made to prod (change_events was not called).

Findings that differ from the public docs

  • region is required on logs endpoints (the docs mark it optional). A wrong region returns 500 ERR_S3_CONFIG_MISSING or a 502 maintenance page.
  • Access tokens last 72h (the docs say 24h). The CLI reads expires_at.
  • Refresh tokens are not rotated on exchange.
  • The Alertmanager migration path and its content type are unverified. The reference flags this; I did not call that endpoint.

Not in this PR

  • Browser-based login (no paste): tracked in FDE-366.
  • Skill-loading smoke check across hosts (npx skills add into a scratch project, then a canary prompt): not run yet.
  • Correcting the public docs for the two discrepancies above.

🤖 Generated with Claude Code

… CLI

Adds skills/last9-api: a stdlib-only Python helper (scripts/last9.py) plus
references for calling the Last9 REST API without the MCP server.

CLI: login / token / status / logout / api. The refresh token is
self-describing (aud -> host, organization_slug -> org), so one pasted
secret is the only config. It sets the X-LAST9-API-TOKEN Bearer header,
caches access tokens, retries once on expiry, refuses foreign hosts and
redirects, and auto-adds the required `region` on logs/traces paths.
Credentials live in ~/.last9 (dir 0700, files 0600, atomic writes);
LAST9_REFRESH_TOKEN is honored for CI and never written to disk.

References cover auth, logs, traces, change events and Alertmanager
migration. Behavior was verified live against an own-org token: refresh
tokens are not rotated, access tokens last 72h (public docs say 24h), and
region is required on logs endpoints (docs say optional).

Pack check: allow exactly one new payload type, a flat
skills/<name>/scripts/<file>.py, via a single shared PAYLOAD_RE. Symlinks,
nesting and other extensions stay rejected, with selftest must-fail cases
for each. Also ignore __pycache__.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
In a read-only home directory or sandbox (for example Codex's default),
write_private raised and the CLI crashed even though the exchange had
succeeded. Warn on stderr and carry on; the token is re-exchanged on the
next call. The credentials write in login stays fatal.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@nishantmodak nishantmodak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completed the full 13-file review at aeaafc949526251007e29aef740cdda3c6be0123. One blocking credential-transport issue: the CLI accepts plain HTTP for the token host and sends the bearer token without TLS. Details and required fix are inline.

Validation: all 28 native Python tests, the skill distribution check, and packaging selftests passed. Added HTTPS, foreign-authority and redirect controls passed; the plaintext-token assertion failed twice (initial suite and minimized rerun). GitHub structural reference, secret scan and plugin parity checks succeeded. Python 3.12.3 / Node 22.21.0; no real tokens, live endpoints, supported-host skill smoke, or migration call exercised. The CLI is new at this head; the base has no equivalent runtime, so comparison is against the HTTPS positive control.

Reproduction: save below as skills/last9-api/scripts/test_review_transport.py alongside the existing test_last9.py fixtures. Run:

cd skills/last9-api/scripts
python3 -m unittest -v test_review_transport

Setup uses a temporary profile/cache and SYNTHETIC_ACCESS token, mocking only the network boundary. Expected: reject HTTP before any bearer-bearing request. Actual: urlopen receives the HTTP URL with the bearer header; the assertion lists that URL. No local workspace artifacts are required.

import io
import time
import unittest
import urllib.request
from unittest import mock
from test_last9 import Base, FakeResp, last9

class TestReviewTransport(Base):
    def setUp(self):
        super().setUp()
        self.login()
        self.ctx = last9.Ctx('default')
        last9.write_private(self.ctx.cache_file, {'access_token': 'SYNTHETIC_ACCESS', 'expires_at': time.time() + 7200})
        saved = urllib.request._opener
        self.addCleanup(urllib.request.install_opener, saved)

    def test_http_must_not_send_bearer(self):
        with mock.patch('urllib.request.urlopen', return_value=FakeResp(b'{}')) as network:
            self.run_cli(['api', 'GET', 'http://app.last9.io/api/v4/organizations/acme/change_events'])
        leaked = [c.args[0].full_url for c in network.call_args_list
                  if c.args[0].full_url.startswith('http:')
                  and c.args[0].get_header('X-last9-api-token') == 'Bearer SYNTHETIC_ACCESS']
        self.assertEqual([], leaked, 'Bearer credentials must never be sent over cleartext HTTP')

    def test_https_positive_control(self):
        with mock.patch('urllib.request.urlopen', return_value=FakeResp(b'{}')) as network:
            code, _, _ = self.run_cli(['api', 'GET', 'https://app.last9.io/api/v4/organizations/acme/change_events'])
        self.assertEqual(code, 0)
        network.assert_called_once()
        self.assertEqual(network.call_args.args[0].get_header('X-last9-api-token'), 'Bearer SYNTHETIC_ACCESS')

    def test_foreign_authorities_rejected(self):
        paths = ['https://foreign.example/x', 'https://app.last9.io.foreign.example/x',
                 'https://app.last9.io@foreign.example/x', 'http://foreign.example/x']
        for path in paths:
            with self.subTest(path=path), mock.patch('urllib.request.urlopen') as network:
                code, _, _ = self.run_cli(['api', 'GET', path])
                self.assertEqual(code, 1)
                network.assert_not_called()

    def test_redirect_refused(self):
        req=urllib.request.Request('https://app.last9.io/x',headers={'X-LAST9-API-TOKEN':'Bearer SYNTHETIC_ACCESS'})
        for code in [301,302,303,307,308]:
            with self.subTest(code=code):
                self.assertIsNone(last9.NoRedirect().redirect_request(req, io.BytesIO(), code, 'redirect', {}, 'https://foreign.example/x'))

if __name__ == '__main__':
    unittest.main()



def resolve_url(ctx, path, queries):
if path.startswith(("http://", "https://")):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Reject plaintext URLs before sending the bearer token

api GET http://app.last9.io/api/v4/organizations/acme/change_events passes this hostname-only check, and send() attaches X-LAST9-API-TOKEN: Bearer ... to the resulting unencrypted HTTP request. A copied or generated URL using HTTP therefore exposes the access token to the network path before any server-side HTTPS redirect can help, allowing reuse with its read/write/delete scopes. Require HTTPS before obtaining/sending credentials; keep the foreign-host and redirect checks. The consolidated review includes a real-CLI regression that fails twice on this head and an HTTPS positive control that passes. No real credentials or external network calls were used.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 1cd0b4c. resolve_url now requires the https scheme before any credential is attached, so http:// URLs are refused locally and urlopen is never called. The foreign-host check and the no-redirect handler are unchanged. Your four cases are in the PR as scripts/test_transport.py; the http case failed before the change (exit 0, token sent) and passes now. SKILL.md and its error table now state the https-only rule. 32 tests pass in total.

resolve_url compared only the hostname for full URLs, so
http://app.last9.io/... passed the check and the access token was sent
unencrypted. Require the https scheme before any credential is attached;
the foreign-host and no-redirect controls are unchanged.

Adds test_transport.py (reported in review): http must not send the token,
https positive control, foreign-authority rejection, redirect refusal.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@nishantmodak nishantmodak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plaintext-token blocker is fixed at 1cd0b4c08e3c52bb87a1e1e1eb6a943941512412. I reran the exact regression attached to my earlier review: HTTP is rejected before any network call, and HTTPS, foreign-host and redirect controls pass. No new actionable blockers found across the complete current 14-file scope (CLI/auth/cache, references, packaging checks and consumers), building on the prior complete inspection and rechecking the update.

Validation: 32 native Python tests plus the four original regression cases passed; the distribution gate and packaging selftests passed. Four additional boundary probes passed (seeded URL-authority cases, refresh failure, expiry margin and environment-token cache separation), and all four original regression cases passed again. Current GitHub reference, parity and secret-scan checks succeeded. Python 3.12.3 / Node 22.21.0; network mocked with synthetic credentials. Python 3.8, live endpoints, skill-loading smoke and the explicitly unverified migration recipe were not exercised. The earlier regression failure is prior-head evidence, not a new execution on that head.

@prathamesh-sonpatki
prathamesh-sonpatki merged commit ec0d201 into master Oct 1, 2026
4 checks passed
@prathamesh-sonpatki
prathamesh-sonpatki deleted the feat/last9-api-skill branch October 1, 2026 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants