diff --git a/cmd/assets/skill/reference.md b/cmd/assets/skill/reference.md index 3078279..e82796c 100644 --- a/cmd/assets/skill/reference.md +++ b/cmd/assets/skill/reference.md @@ -73,6 +73,17 @@ Containers for notes; exactly one **default** notebook per account. `--make-default` promotes a notebook (the prior default is demoted; you can't "un-default" directly — promote another). The default notebook can't be deleted. +**The default notebook can never be encrypt-by-default.** Forwarded email, +imports, and notes created with no notebook all land in the default, and none of +those writers can client-side encrypt — so the pair is banned outright. The CLI +refuses it locally, in both directions: `--default-encrypt` on the default +notebook, and `--make-default` on a notebook that already encrypts. To promote an +encrypting notebook, turn encryption off in the same command: + +```bash +harbor notebooks update --make-default --default-encrypt=false +``` + --- ## Notes (aliases: `note`, `n`) diff --git a/cmd/notebooks.go b/cmd/notebooks.go index 5127112..b250d95 100644 --- a/cmd/notebooks.go +++ b/cmd/notebooks.go @@ -104,10 +104,17 @@ var notebooksUpdateCmd = &cobra.Command{ Use --make-default to promote this notebook to the account default (the prior default is demoted automatically). There must always be exactly one default, so -a notebook cannot be un-defaulted directly — promote a different one instead.`, +a notebook cannot be un-defaulted directly — promote a different one instead. + +The default notebook can never also be encrypt-by-default: forwarded email, +imports, and notes created with no notebook all land in the default, and none of +those writers can encrypt. So --default-encrypt is refused on the default +notebook, and --make-default is refused on a notebook that encrypts — unless you +turn it off in the same command, e.g. --make-default --default-encrypt=false.`, Example: ` harbor notebooks update 5b1f... --name "Work — Active" harbor notebooks update 5b1f... --stack Archive --public=false - harbor notebooks update 5b1f... --make-default`, + harbor notebooks update 5b1f... --make-default + harbor notebooks update 5b1f... --make-default --default-encrypt=false`, RunE: func(cmd *cobra.Command, args []string) error { c, _, err := loadClientFromConfig() if err != nil { @@ -124,6 +131,10 @@ a notebook cannot be un-defaulted directly — promote a different one instead.` if len(body) == 0 { return errors.New("nothing to update — pass at least one field flag") } + // Refuse the banned pair before spending a request on a guaranteed 422. + if err := guardDefaultNotebookEncrypt(c, args[0], body); err != nil { + return err + } data, err := c.UpdateNotebook(args[0], body) if err != nil { return mapNotebookError(err) @@ -156,6 +167,11 @@ var notebooksDeleteCmd = &cobra.Command{ } // mapNotebookError gives friendly messages for the notebook-specific codes. +// default_notebook_cannot_encrypt is deliberately absent from the switch below. +// Its server message is user-facing copy shared with the web app, and the CLI's +// default renderer prints an APIError's message verbatim — so paraphrasing it +// here is the one way to get it wrong. Pinned by +// TestDefaultCannotEncryptServerMessageIsNotParaphrased. func mapNotebookError(err error) error { var apiErr *client.APIError if errors.As(err, &apiErr) { @@ -171,6 +187,67 @@ func mapNotebookError(err error) error { return err } +// defaultCannotEncryptMessage is the server's own wording for the banned pair, +// quoted rather than paraphrased. +// +// The rule: a notebook can never be both the account default AND encrypt-by- +// default. The default is where forwarded email, imports, and notes created with +// no notebook land, and none of those writers can client-side encrypt — so a +// note the user believes is sealed would arrive there in the clear. +// +// The CLI refuses locally instead of letting the request 422, so the user gets the +// reason and the fix without spending a write. A request that states both fields +// needs no network at all; the single-flag cases still read the notebook first, +// so offline they surface the read error rather than this refusal. +// The wording matches app.harbor.my internal/notebooks/notebooks.go exactly +// (issue app.harbor.my#1338); web shows the same sentence. +const defaultCannotEncryptMessage = "the default notebook can't encrypt notes by default — forwarded email, imports, and notes with no notebook land there; encrypt a different notebook instead" + +// guardDefaultNotebookEncrypt refuses an update whose RESULTING state would make +// one notebook both the default and encrypt-by-default. +// +// Judged on the resulting state, not on the fields present, because the server +// judges it that way: `--make-default --default-encrypt=false` legally promotes a +// notebook while switching encryption off, and refusing that because the request +// mentions both flags would block the one command that fixes the situation. +// +// It fetches the notebook only when the answer genuinely depends on the current +// state — when the request sets both fields, or sets neither to a banned value, +// no round trip happens. +func guardDefaultNotebookEncrypt(c *client.Client, id string, body map[string]any) error { + wantEncrypt, encryptSet := body["default_encrypt"].(bool) + wantDefault, defaultSet := body["is_default"].(bool) + + // Turning encryption OFF can never produce the pair, whatever else changes. + if encryptSet && !wantEncrypt { + return nil + } + // Both stated in one request: decidable with no fetch. + if encryptSet && wantEncrypt && defaultSet && wantDefault { + return errors.New(defaultCannotEncryptMessage) + } + // Only one side stated; the other comes from the notebook as it stands. + needCurrent := (encryptSet && wantEncrypt) || (defaultSet && wantDefault) + if !needCurrent { + return nil + } + data, err := c.GetNotebook(id, false) + if err != nil { + // A notebook we cannot read is not one we can clear. Let the write go and + // let the server judge it — it enforces the same rule, and failing the + // command here would turn a transient read error into a refusal to write. + return nil + } + nb := parseJSON(client.UnwrapData(data)) + if encryptSet && wantEncrypt && boolean(nb, "is_default") { + return errors.New(defaultCannotEncryptMessage) + } + if defaultSet && wantDefault && boolean(nb, "default_encrypt") { + return errors.New(defaultCannotEncryptMessage + " — switch it off with '--default-encrypt=false' in the same command to promote this notebook") + } + return nil +} + // =========================================================================== // Display // =========================================================================== @@ -239,9 +316,9 @@ func init() { notebooksUpdateCmd.Flags().String("name", "", "New name") notebooksUpdateCmd.Flags().String("stack", "", "New stack") - notebooksUpdateCmd.Flags().Bool("default-encrypt", false, "Encrypt new notes by default") + notebooksUpdateCmd.Flags().Bool("default-encrypt", false, "Encrypt new notes by default (never allowed on the default notebook)") notebooksUpdateCmd.Flags().Bool("public", false, "Make the notebook public") - notebooksUpdateCmd.Flags().Bool("make-default", false, "Promote this notebook to the account default") + notebooksUpdateCmd.Flags().Bool("make-default", false, "Promote this notebook to the account default (refused if it encrypts by default)") notebooksDeleteCmd.Flags().String("notes", "", "What to do with its notes: move_to_default (default) or trash") diff --git a/cmd/notebooks_test.go b/cmd/notebooks_test.go index b59fbc3..91fd6b8 100644 --- a/cmd/notebooks_test.go +++ b/cmd/notebooks_test.go @@ -4,8 +4,14 @@ package cmd import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" "strings" "testing" + + "github.com/HarborMyNotes/harbor-cli/client" ) func TestDisplayNotebooks(t *testing.T) { @@ -46,3 +52,149 @@ func TestMapNotebookError(t *testing.T) { } } } + +// nbGuardServer stands in for the API when the guard needs to read a notebook's +// current state. It records whether the GET happened, so the tests can prove the +// no-fetch cases really do not spend a request. +func nbGuardServer(t *testing.T, isDefault, defaultEncrypt bool, fetched *int) *client.Client { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + *fetched++ + w.Header().Set("Content-Type", "application/json") + fmt.Fprintf(w, `{"id":"nb1","name":"Work","is_default":%t,"default_encrypt":%t}`, isDefault, defaultEncrypt) + })) + t.Cleanup(srv.Close) + return client.NewClient(srv.URL, "at_test") +} + +// TestGuardRefusesBothDirections pins the rule in both directions: turning +// encryption on for the current default, and promoting a notebook that already +// encrypts. Either way the CLI refuses before spending a request on a 422. +func TestGuardRefusesBothDirections(t *testing.T) { + cases := []struct { + name string + isDefault bool + defaultEncrypt bool + body map[string]any + wantRefused bool + }{ + {"encrypt-on for the current default", true, false, + map[string]any{"default_encrypt": true}, true}, + {"promote a notebook that encrypts", false, true, + map[string]any{"is_default": true}, true}, + {"encrypt-on for a non-default notebook", false, false, + map[string]any{"default_encrypt": true}, false}, + {"promote a notebook that does not encrypt", false, false, + map[string]any{"is_default": true}, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fetched := 0 + c := nbGuardServer(t, tc.isDefault, tc.defaultEncrypt, &fetched) + err := guardDefaultNotebookEncrypt(c, "nb1", tc.body) + if tc.wantRefused && err == nil { + t.Fatal("the banned pair was allowed through") + } + if !tc.wantRefused && err != nil { + t.Fatalf("a legal update was refused: %v", err) + } + if err != nil && !strings.Contains(err.Error(), "encrypt a different notebook instead") { + t.Errorf("refusal is not the server's wording: %v", err) + } + }) + } +} + +// TestGuardJudgesTheResultingState is the case the issue calls out explicitly: +// one request carrying BOTH fields legally promotes a notebook while switching +// encryption off. Judging on "does the request mention both flags" would block +// the single command that fixes an encrypting notebook someone wants as default. +func TestGuardJudgesTheResultingState(t *testing.T) { + fetched := 0 + c := nbGuardServer(t, false, true, &fetched) + + // Promote while switching encryption OFF — legal, and decided with no fetch. + if err := guardDefaultNotebookEncrypt(c, "nb1", map[string]any{"is_default": true, "default_encrypt": false}); err != nil { + t.Fatalf("promoting while turning encryption off was refused: %v", err) + } + if fetched != 0 { + t.Errorf("the resulting state was fully stated, but the guard still fetched %d time(s)", fetched) + } + + // Both ON in one request — refused, also with no fetch. + if err := guardDefaultNotebookEncrypt(c, "nb1", map[string]any{"is_default": true, "default_encrypt": true}); err == nil { + t.Fatal("setting both flags true in one request was allowed") + } + if fetched != 0 { + t.Errorf("a request stating both fields needs no round trip, but the guard fetched %d time(s)", fetched) + } +} + +// TestGuardDoesNotFetchWhenIrrelevant proves the guard costs nothing on the +// updates that cannot produce the pair — renames, stack moves, and any request +// that turns encryption off. +func TestGuardDoesNotFetchWhenIrrelevant(t *testing.T) { + for name, body := range map[string]map[string]any{ + "rename only": {"name": "Work — Active"}, + "encryption off": {"default_encrypt": false}, + "off and renamed": {"default_encrypt": false, "name": "x"}, + } { + fetched := 0 + c := nbGuardServer(t, true, true, &fetched) + if err := guardDefaultNotebookEncrypt(c, "nb1", body); err != nil { + t.Errorf("%s was refused: %v", name, err) + } + if fetched != 0 { + t.Errorf("%s should need no round trip, but the guard fetched %d time(s)", name, fetched) + } + } +} + +// TestGuardFailsOpenOnAnUnreadableNotebook proves a transient read error does not +// become a refusal to write. The server enforces the same rule, so letting the +// write through costs a 422 at worst; refusing here would block a legal update +// because an unrelated GET failed. +func TestGuardFailsOpenOnAnUnreadableNotebook(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + if err := guardDefaultNotebookEncrypt(client.NewClient(srv.URL, "at_test"), "nb1", map[string]any{"default_encrypt": true}); err != nil { + t.Fatalf("an unreadable notebook should not block the write: %v", err) + } +} + +// TestDefaultCannotEncryptServerMessageIsNotParaphrased pins that the server's +// 422 reaches the user in the server's own words. +// +// The message is user-facing copy shared with the web app, and the issue asks for +// it verbatim. mapNotebookError paraphrases the other notebook codes, so the risk +// is somebody "helpfully" adding a case for this one; the CLI's default renderer +// already prints an APIError's message, so the correct handling is to leave it +// alone. This fails if a mapping is ever added. +func TestDefaultCannotEncryptServerMessageIsNotParaphrased(t *testing.T) { + const serverMessage = "The default notebook can't encrypt notes by default — forwarded email, imports, and notes with no notebook land there; encrypt a different notebook instead." + apiError := &client.APIError{Code: "default_notebook_cannot_encrypt", Message: serverMessage, Status: 422} + + got := mapNotebookError(apiError) + if got.Error() != apiError.Error() { + t.Fatalf("the server's message was rewritten locally:\n got: %s\nwant: %s", got.Error(), apiError.Error()) + } + var still *client.APIError + if !errors.As(got, &still) { + t.Fatal("the APIError was replaced, so the renderer can no longer print its message and details") + } +} + +// TestLocalRefusalMatchesTheServerWording keeps the refusal the CLI raises on its +// own in step with the sentence the server would have sent. Two wordings for one +// rule is how a user learns to distrust one of them. +func TestLocalRefusalMatchesTheServerWording(t *testing.T) { + const serverMessage = "The default notebook can't encrypt notes by default — forwarded email, imports, and notes with no notebook land there; encrypt a different notebook instead." + // Lower only the FIRST character. Lowercasing the whole sentence would demand + // the CLI mangle a proper noun if the server copy ever gains one. + normalized := strings.TrimSuffix(strings.ToLower(serverMessage[:1])+serverMessage[1:], ".") + if defaultCannotEncryptMessage != normalized { + t.Errorf("the local refusal has drifted from the server's copy:\n local: %s\nserver: %s", defaultCannotEncryptMessage, normalized) + } +} diff --git a/cmd/skill.go b/cmd/skill.go index 34b9144..3b61ddf 100644 --- a/cmd/skill.go +++ b/cmd/skill.go @@ -37,7 +37,7 @@ const ( // skillVersion is the content version of the bundled skill. Bump it whenever // the skill files change; it is surfaced to the user on install and helps // distinguish "already current" from "needs updating". - skillVersion = "1.1.1" + skillVersion = "1.1.2" // codexBlockBegin / codexBlockEnd delimit the managed section the installer // splices into a Codex AGENTS.md (a file the user may share with their own diff --git a/cmd/sync.go b/cmd/sync.go index b45ed43..bb6945a 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -88,6 +88,7 @@ server_record you need to resolve it comes back in the results, so that stays 0. if err != nil { return err } + warnDefaultNotebookEncrypt(changes) data, err := c.SyncPush(map[string]any{"scope_id": scopeID, "device_id": deviceID, "changes": changes}) if err != nil { return mapSyncError(err) @@ -197,6 +198,43 @@ var syncAckCmd = &cobra.Command{ // Helpers // =========================================================================== +// warnDefaultNotebookEncrypt tells the user when a pushed notebook envelope would +// make one notebook both the account default and encrypt-by-default. +// +// It WARNS rather than refuses, because that is what the server does here. Unlike +// PATCH /notebooks/:id — which 422s and writes nothing — sync push coerces: +// a pushed record that ends up default has default_encrypt forced to 0, and the +// corrected record comes back on the next pull as an ordinary server-authoritative +// overwrite. Refusing locally would break the passthrough contract of this command +// (the envelopes are the user's own JSON) and reject a batch the server would have +// accepted. Saying nothing would leave the user to discover on a later pull that a +// flag they set had been quietly turned off. +// +// This is also the whole of the CLI's "sync engine cannot produce the pair" +// obligation: the CLI keeps no local queue and constructs no notebook records of +// its own — `sync push` forwards a JSON file the user wrote — so there is no +// client-side state that could hold the banned pair. Pinned by +// TestNoNotebookRecordsAreConstructedByTheCLI. +func warnDefaultNotebookEncrypt(changes []any) { + for _, ch := range changes { + env, ok := ch.(map[string]any) + if !ok || str(env, "type") != "notebook" { + continue + } + rec := nested(env, "record") + if rec == nil { + continue + } + if boolean(rec, "is_default") && boolean(rec, "default_encrypt") { + fmt.Fprintln(os.Stderr, dim("Note: a pushed notebook is both the default and encrypt-by-default. The server\n"+ + "cannot store that pair — the default is where forwarded email, imports and notes\n"+ + "with no notebook land, and none of those can encrypt — so it will force\n"+ + "default_encrypt off and send the corrected record back on your next pull.")) + return + } + } +} + // runSyncPull pulls one chunk, or (when all is true) pages through every chunk // until caught up, merging them into one synthesized pull-response document. // Extracted from the command so the paging loop is unit-testable. diff --git a/cmd/sync_test.go b/cmd/sync_test.go index ab39d68..63c873f 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -7,9 +7,12 @@ import ( "encoding/json" "errors" "io" + "io/fs" "net/http" "net/http/httptest" "os" + "path/filepath" + "regexp" "strings" "testing" @@ -263,3 +266,111 @@ func TestSyncPushCommandExitsZeroWhenEverythingApplied(t *testing.T) { t.Fatalf("a clean push must exit 0: %v", err) } } + +// TestWarnDefaultNotebookEncrypt proves the push path says something when a +// pushed notebook record carries the banned pair, and stays quiet otherwise. +// +// It warns rather than refuses because sync push COERCES server-side (unlike +// PATCH /notebooks/:id, which 422s): the record lands with default_encrypt +// forced off and comes back corrected on the next pull. Refusing would reject a +// batch the server would have accepted; silence would let a flag the user set +// disappear without explanation. +func TestWarnDefaultNotebookEncrypt(t *testing.T) { + banned := []any{map[string]any{ + "type": "notebook", "id": "nb1", + "record": map[string]any{"id": "nb1", "is_default": true, "default_encrypt": true}, + }} + out := captureStderr(t, func() { warnDefaultNotebookEncrypt(banned) }) + for _, want := range []string{"default", "force", "next pull"} { + if !strings.Contains(out, want) { + t.Errorf("the warning does not mention %q:\n%s", want, out) + } + } + + quiet := []any{ + map[string]any{"type": "notebook", "id": "nb2", + "record": map[string]any{"id": "nb2", "is_default": true, "default_encrypt": false}}, + map[string]any{"type": "notebook", "id": "nb3", + "record": map[string]any{"id": "nb3", "is_default": false, "default_encrypt": true}}, + map[string]any{"type": "note", "id": "n1", + "record": map[string]any{"id": "n1", "is_default": true, "default_encrypt": true}}, + map[string]any{"type": "notebook", "id": "nb4"}, // no record at all + "not an envelope", + } + if out := captureStderr(t, func() { warnDefaultNotebookEncrypt(quiet) }); out != "" { + t.Errorf("warned about a legal push:\n%s", out) + } +} + +// TestWarnDefaultNotebookEncryptSpeaksOnce proves a batch full of offending +// notebooks produces one warning, not one per record — a push is a batch, and a +// wall of identical warnings is how people learn to scroll past them. +func TestWarnDefaultNotebookEncryptSpeaksOnce(t *testing.T) { + var changes []any + for i := 0; i < 5; i++ { + changes = append(changes, map[string]any{ + "type": "notebook", "id": "nb", + "record": map[string]any{"id": "nb", "is_default": true, "default_encrypt": true}, + }) + } + out := captureStderr(t, func() { warnDefaultNotebookEncrypt(changes) }) + if n := strings.Count(out, "cannot store that pair"); n != 1 { + t.Errorf("warned %d times for one batch, want 1:\n%s", n, out) + } +} + +// TestNoNotebookRecordsAreConstructedByTheCLI is the canary over the CLI's "the +// sync engine cannot produce the banned pair" property. +// +// The property holds by construction: the CLI keeps no offline queue, and there +// are exactly two SyncPush call sites — this file's push command, which forwards +// a JSON file the USER wrote, and cmd/crypto.go, which pushes the keystore. So +// there is no client-side state that could hold a default notebook with +// default_encrypt on. +// +// This test is a canary, NOT a proof. It walks the whole module and greps for a +// notebook type tag, which catches the obvious regression — someone building a +// notebook envelope inline — but a determined one slips past: a struct with a +// json tag, a const indirection, or a value assembled at runtime. Treat a +// failure here as certain, and a pass as "nothing obvious", not as a guarantee. +// A real proof would parse with go/ast and follow the values into SyncPush. +func TestNoNotebookRecordsAreConstructedByTheCLI(t *testing.T) { + // A regex, not a fixed string: gofmt aligns struct-literal keys to the longest + // one, so the spacing after "type" changes with the other keys in the map. An + // exact-match check passes on the most natural envelope shape. + banned := regexp.MustCompile(`"type"\s*:\s*"notebook"`) + + // Walk the whole module from its root rather than a hardcoded package list, so + // a new package cannot be invisible to this simply by existing. + scanned := 0 + err := filepath.WalkDir("..", func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + switch d.Name() { + case ".git", "build", "dist", "vendor", "node_modules": + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + src, err := os.ReadFile(path) + if err != nil { + return err + } + scanned++ + if banned.Match(src) { + t.Errorf("%s constructs a notebook sync record — the CLI's 'no local state can hold the banned pair' property no longer holds by construction", path) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + if scanned < 40 { + t.Fatalf("only scanned %d files — the walk is not reaching the source tree", scanned) + } +}