Skip to content

Require note ownership validation in block endpoints - #75

Closed
egeozcan wants to merge 2 commits into
masterfrom
fix-block-state-noteid-validation-7341321383589888562
Closed

egeozcan wants to merge 2 commits into
masterfrom
fix-block-state-noteid-validation-7341321383589888562

Conversation

@egeozcan

@egeozcan egeozcan commented Oct 6, 2026

Copy link
Copy Markdown
Owner
  • Updated src/components/blockEditor.js to pass noteId in fetch requests for PUT /v1/note/block, PATCH /v1/note/block/state, and DELETE /v1/note/block.
  • Updated server/api_handlers/block_api_handlers.go to require noteId for those same endpoints.
  • Patched several test scripts (block_api_test.go, block_state_empty_body_test.go, delete_block_mention_sync_test.go, status_code_consistency_test.go) to pass noteId correctly and expect proper error codes in tests designed to test the cross-note validations.

PR created automatically by Jules for task 7341321383589888562 started by @egeozcan

…mutations

The UpdateBlockStateHandler, UpdateBlockContentHandler, and DeleteBlockHandler previously validated noteId only if it was non-zero, allowing callers (including the frontend) to omit it entirely and bypass ownership validation. This commit enforces that `noteId` is strictly required in the backend and correctly passes it in `blockEditor.js` fetch requests, as well as fixing the test suite assertions to expect the new behavior.

Co-authored-by: egeozcan <185294+egeozcan@users.noreply.github.com>
@google-labs-jules

Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@egeozcan

egeozcan commented Oct 6, 2026

Copy link
Copy Markdown
Owner Author

Review (agent): recommend closing as redundant with #73.

Overlap: the three handler hunks in server/api_handlers/block_api_handlers.go (UpdateBlockContent, UpdateBlockState, DeleteBlock: 400 when noteId==0, then GetBlock and ownership check) are identical in substance to #73's. The edits to block_api_test.go, block_state_empty_body_test.go, delete_block_mention_sync_test.go and status_code_consistency_test.go are the same noteId additions #73 (and #71 for delete) make. #71 covers only DeleteBlock; #73 covers all three.

What #75 lacks versus #73, so merging it alone would regress things:

  • CLI: cmd/mr/commands/note_blocks.go (update/update-state/delete send no noteId, so they would all 400);
  • blockEditor.js: misses the keepalive flush PUT (~line 198) which still sends no noteId, and the conditional fallback sends no noteId when this.noteId is falsy, which now 400s;
  • public/dist/main.js not rebuilt; e2e/helpers/api-client.ts, OpenAPI, docs, help md and the new cross-note/missing-noteId tests are absent.

Nothing unique to add: no state-specific logic beyond what #73 already covers. Reorder/create already take a noteId. Recommended order: land #73 (or #71 then rebase #73), close #75. I did not push to this branch.

@google-labs-jules

Copy link
Copy Markdown
Contributor

Review (agent): recommend closing as redundant with #73.

Overlap: the three handler hunks in server/api_handlers/block_api_handlers.go (UpdateBlockContent, UpdateBlockState, DeleteBlock: 400 when noteId==0, then GetBlock and ownership check) are identical in substance to #73's. The edits to block_api_test.go, block_state_empty_body_test.go, delete_block_mention_sync_test.go and status_code_consistency_test.go are the same noteId additions #73 (and #71 for delete) make. #71 covers only DeleteBlock; #73 covers all three.

What #75 lacks versus #73, so merging it alone would regress things:

  • CLI: cmd/mr/commands/note_blocks.go (update/update-state/delete send no noteId, so they would all 400);
  • blockEditor.js: misses the keepalive flush PUT (~line 198) which still sends no noteId, and the conditional fallback sends no noteId when this.noteId is falsy, which now 400s;
  • public/dist/main.js not rebuilt; e2e/helpers/api-client.ts, OpenAPI, docs, help md and the new cross-note/missing-noteId tests are absent.

Nothing unique to add: no state-specific logic beyond what #73 already covers. Reorder/create already take a noteId. Recommended order: land #73 (or #71 then rebase #73), close #75. I did not push to this branch.

Understood. Acknowledging that this work is now obsolete and stopping work on this task.

Closing as obsolete since the fix is already covered by #73 and #71, as noted in the review comment.

Co-authored-by: egeozcan <185294+egeozcan@users.noreply.github.com>
@egeozcan egeozcan closed this Oct 6, 2026
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.

1 participant