Skip to content

fix(base): reject wrapped JSON payloads in +record-upsert #305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zfaustk wants to merge 3 commits into
base: main
Choose a base branch
from
+35 −10
Open

fix(base): reject wrapped JSON payloads in +record-upsert #305

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3692370
fix(base): reject top-level fields wrapper in +record-upsert
zfaustk Apr 7, 2026
123746c
test(base): cover direct record object validation
zfaustk Apr 7, 2026
ef6baf2
test(base): reject wrapped upsert payloads in execute flow
zfaustk Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions shortcuts/base/base_execute_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ func TestBaseRecordExecuteUpsertUpdate(t *testing.T) {
"data": map[string]interface{}{"record_id": "rec_x", "fields": map[string]interface{}{"Name": "Alice"}},
},
})
if err := runShortcut(t, BaseRecordUpsert, []string{"+record-upsert", "--base-token", "app_x", "--table-id", "tbl_x", "--record-id", "rec_x", "--json", `{"fields":{"Name":"Alice"}}`}, factory, stdout); err != nil {
if err := runShortcut(t, BaseRecordUpsert, []string{"+record-upsert", "--base-token", "app_x", "--table-id", "tbl_x", "--record-id", "rec_x", "--json", `{"Name":"Alice"}`}, factory, stdout); err != nil {
t.Fatalf("err=%v", err)
}
if got := stdout.String(); !strings.Contains(got, `"updated": true`) || !strings.Contains(got, `"rec_x"`) {
Expand Down Expand Up @@ -544,14 +544,25 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) {
"data": map[string]interface{}{"record_id": "rec_new", "fields": map[string]interface{}{"Name": "Alice"}},
},
})
if err := runShortcut(t, BaseRecordUpsert, []string{"+record-upsert", "--base-token", "app_x", "--table-id", "tbl_x", "--json", `{"fields":{"Name":"Alice"}}`}, factory, stdout); err != nil {
if err := runShortcut(t, BaseRecordUpsert, []string{"+record-upsert", "--base-token", "app_x", "--table-id", "tbl_x", "--json", `{"Name":"Alice"}`}, factory, stdout); err != nil {
t.Fatalf("err=%v", err)
}
if got := stdout.String(); !strings.Contains(got, `"created": true`) || !strings.Contains(got, `"rec_new"`) {
t.Fatalf("stdout=%s", got)
}
})

t.Run("reject top-level fields wrapper", func(t *testing.T) {
factory, stdout, _ := newExecuteFactory(t)
err := runShortcut(t, BaseRecordUpsert, []string{"+record-upsert", "--base-token", "app_x", "--table-id", "tbl_x", "--json", `{"fields":{"Name":"Alice"}}`}, factory, stdout)
if err == nil || !strings.Contains(err.Error(), "direct record object") {
t.Fatalf("err=%v", err)
}
if got := stdout.String(); got != "" {
t.Fatalf("stdout=%s", got)
}
})

t.Run("delete", func(t *testing.T) {
factory, stdout, reg := newExecuteFactory(t)
reg.Register(&httpmock.Stub{
Expand Down Expand Up @@ -587,7 +598,7 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) {
URL: "/open-apis/base/v3/bases/app_x/tables/tbl_x/fields/fld_att",
Body: map[string]interface{}{
"code": 0,
"data": map[string]interface{}{"id": "fld_att", "name": "附件", "type": "attachment"},
"data": map[string]interface{}{"id": "fld_att", "name": "附件", "type": "attachment"},
},
})
reg.Register(&httpmock.Stub{
Expand All @@ -598,7 +609,7 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) {
"data": map[string]interface{}{
"record_id": "rec_x",
"fields": map[string]interface{}{
"附件": []interface{}{
"附件": []interface{}{
map[string]interface{}{
"file_token": "existing_tok",
"name": "existing.pdf",
Expand Down Expand Up @@ -629,7 +640,7 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) {
"data": map[string]interface{}{
"record_id": "rec_x",
"fields": map[string]interface{}{
"附件": []interface{}{
"附件": []interface{}{
map[string]interface{}{
"file_token": "existing_tok",
"name": "existing.pdf",
Expand Down Expand Up @@ -671,7 +682,7 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) {
}

updateBody := string(updateStub.CapturedBody)
if !strings.Contains(updateBody, `"附件"`) ||
if !strings.Contains(updateBody, `"附件"`) ||
Comment on lines 601 to +685
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 7, 2026

Choose a reason for hiding this comment

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

P1 Encoding corruption in unrelated test strings

The PR replaces valid UTF-8 Chinese characters with Latin-1 mojibake across multiple unrelated stubs and assertions: "附件" (attachment) → "é件" at lines 601, 612, 643, and 685; "状态" (status) → "ç¶æ" at line 718; "已完成" → "å·²å®æ" and keyword "已" → "å·²" at lines 913–919. This appears to be an editor encoding bug where the UTF-8 source was re-saved as Latin-1. The tests may still pass because the corruption is symmetric between stubs and assertions, but the data no longer represents realistic API responses and will mislead future maintainers. These lines were not part of the intended change — please revert to the original Chinese characters.

!strings.Contains(updateBody, `"file_token":"existing_tok"`) ||
!strings.Contains(updateBody, `"name":"existing.pdf"`) ||
!strings.Contains(updateBody, `"size":2048`) ||
Expand Down Expand Up @@ -704,7 +715,7 @@ func TestBaseRecordExecuteReadCreateDelete(t *testing.T) {
URL: "/open-apis/base/v3/bases/app_x/tables/tbl_x/fields/fld_status",
Body: map[string]interface{}{
"code": 0,
"data": map[string]interface{}{"id": "fld_status", "name": "状态", "type": "text"},
"data": map[string]interface{}{"id": "fld_status", "name": "状态", "type": "text"},
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Same UTF-8 encoding corruption — 状态 became ç¶æ.

The field name 状态 (meaning "status") shows the same mojibake corruption pattern as the attachment field name.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/base/base_execute_test.go` at line 718, The test data contains a
mojibake string for the field name where the map entry with id "fld_status" uses
corrupted bytes; replace the corrupted value ("��") with the correct UTF-8
Chinese string "状态" in the map literal, and ensure the source file is
saved/committed as UTF-8 to prevent recurrence; scan for other occurrences of
"ç�¶æ" in the test file to correct any additional corrupted field names.

},
})

Expand Down Expand Up @@ -899,13 +910,13 @@ func TestBaseFieldExecuteSearchOptions(t *testing.T) {
URL: "/open-apis/base/v3/bases/app_x/tables/tbl_x/fields/fld_amount/options",
Body: map[string]interface{}{
"code": 0,
"data": map[string]interface{}{"options": []interface{}{map[string]interface{}{"id": "opt_1", "name": "已完成"}}, "total": 1},
"data": map[string]interface{}{"options": []interface{}{map[string]interface{}{"id": "opt_1", "name": "已完成"}}, "total": 1},
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Same UTF-8 encoding corruption in search options test.

The keyword and option names have similar corruption:

  • 已 → å·²
  • 已完成 → å·²å®æ

These should be restored to the original Chinese characters unless this is intentional test data for encoding handling.

Also applies to: 916-916, 919-919

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/base/base_execute_test.go` at line 913, The test contains UTF-8
corrupted Chinese characters in the options/keyword map literal (the map literal
with "options": []interface{}{map[string]interface{}{"id": "opt_1", "name":
"..."} } ) — replace the corrupted sequences like "å·²" and "å·²å®�æ��" with the
correct Chinese characters "已" and "已完成" respectively in that map (and the other
identical occurrences of the same option/keyword entries in the same test file)
so the option name and keyword strings use the original Chinese text.

},
})
if err := runShortcut(t, BaseFieldSearchOptions, []string{"+field-search-options", "--base-token", "app_x", "--table-id", "tbl_x", "--field-id", "fld_amount", "--keyword", "已", "--limit", "10"}, factory, stdout); err != nil {
if err := runShortcut(t, BaseFieldSearchOptions, []string{"+field-search-options", "--base-token", "app_x", "--table-id", "tbl_x", "--field-id", "fld_amount", "--keyword", "å·²", "--limit", "10"}, factory, stdout); err != nil {
t.Fatalf("err=%v", err)
}
if got := stdout.String(); !strings.Contains(got, `"options"`) || !strings.Contains(got, `"已完成"`) {
if got := stdout.String(); !strings.Contains(got, `"options"`) || !strings.Contains(got, `"已完成"`) {
t.Fatalf("stdout=%s", got)
}
}
Expand Down
3 changes: 3 additions & 0 deletions shortcuts/base/base_shortcuts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ func TestBaseRecordValidate(t *testing.T) {
if err := BaseRecordUpsert.Validate(ctx, newBaseTestRuntime(map[string]string{"base-token": "b", "table-id": "tbl_1", "json": `{"Name":"A"}`}, nil, nil)); err != nil {
t.Fatalf("upsert validate err=%v", err)
}
if err := BaseRecordUpsert.Validate(ctx, newBaseTestRuntime(map[string]string{"base-token": "b", "table-id": "tbl_1", "json": `{"fields":{"Name":"A"}}`}, nil, nil)); err == nil || !strings.Contains(err.Error(), "direct record object") {
t.Fatalf("err=%v", err)
}
if err := BaseRecordUpsert.Validate(ctx, newBaseTestRuntime(map[string]string{"base-token": "b", "table-id": "tbl_1", "json": "{"}, nil, nil)); err != nil {
t.Fatalf("invalid record json should bypass CLI validate, err=%v", err)
}
Expand Down
11 changes: 11 additions & 0 deletions shortcuts/base/record_ops.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,17 @@ func dryRunRecordHistoryList(_ context.Context, runtime *common.RuntimeContext)
}

func validateRecordJSON(runtime *common.RuntimeContext) error {
body, err := parseJSONObject(runtime.Str("json"), "json")
if err != nil {
// Keep invalid JSON handling on the execution path unchanged; only
// intercept the common top-level shape mistake here.
return nil
}
if len(body) == 1 {
if fields, ok := body["fields"].(map[string]interface{}); ok && len(fields) > 0 {
Copy link
Copy Markdown

@greptile-apps greptile-apps bot Apr 7, 2026

Choose a reason for hiding this comment

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

P2 Empty {"fields":{}} wrapper silently bypasses the check

The len(fields) > 0 guard means a payload of {"fields": {}} is not rejected, even though it is still the wrong shape. Consider dropping the length check so the empty-wrapper case is also caught early:

Suggested change
if fields, ok := body["fields"].(map[string]interface{}); ok && len(fields) > 0 {
if _, ok := body["fields"].(map[string]interface{}); ok {

return common.FlagErrorf("--json for +record-upsert must be a direct record object, not a top-level \"fields\" wrapper; use '{\"Name\":\"Alice\"}' instead of '{\"fields\":{\"Name\":\"Alice\"}}'. If your real field name is literally \"fields\", use the field ID as the key.")
}
}
return nil
}

Expand Down
Loading