Skip to content

rts: Deny op data that has no path rather than reading one that is not there - #4224

Open
marksvc wants to merge 1 commit into
task/sec1-7from
task/sec1-9
Open

marksvc wants to merge 1 commit into
task/sec1-7from
task/sec1-9

Conversation

@marksvc

@marksvc marksvc commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

The editable properties check reads a path from each component of a client's op
without establishing that there is one. The deny rule that refuses op data that
is not a list of components cannot protect it, because sharedb-access runs every
allow rule before any deny rule. So a client submitting a rich-text delta against
a doc with editable properties made the check read a path that is not there, and
the submit failed with a TypeError out of our code in place of "Permission
denied". Nothing was written either way.

The template matcher now accepts a path that may be missing, and matches no
template to one. Such op data therefore changes no editable property, and the
check denies it.


Open in Devin Review


Stack created with GitHub Stacks CLI • Give Feedback 💬


This change is Reviewable

@marksvc
marksvc added this pull request to stack #4226 October 22, 2026 21:54
@marksvc
marksvc deployed to screenshot_diff October 22, 2026 22:07 — with GitHub Actions Active
@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.20%. Comparing base (dacc865) to head (4f2e9e1).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@             Coverage Diff              @@
##           task/sec1-7    #4224   +/-   ##
============================================
  Coverage        81.20%   81.20%           
============================================
  Files              670      670           
  Lines            42920    42922    +2     
  Branches          7116     7088   -28     
============================================
+ Hits             34853    34855    +2     
- Misses            6902     6919   +17     
+ Partials          1165     1148   -17     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

…t there

The editable properties check reads a path from each component of a client's op
without establishing that there is one. The deny rule that refuses op data that
is not a list of components cannot protect it, because sharedb-access runs every
allow rule before any deny rule. So a client submitting a rich-text delta against
a doc with editable properties made the check read a path that is not there, and
the submit failed with a TypeError out of our code in place of "Permission
denied". Nothing was written either way.

The template matcher now accepts a path that may be missing, and matches no
template to one. Such op data therefore changes no editable property, and the
check denies it.
@marksvc
marksvc removed this pull request from stack #4226 October 23, 2026 16:11
@marksvc
marksvc changed the base branch from task/sec1-8 to task/sec1-7 October 23, 2026 16:11
@marksvc
marksvc added this pull request to stack #4228 October 23, 2026 16:11
@marksvc marksvc added testing not required ai-review-author-sign-off PR author has looked at AI review of change and determined no findings warrant further changes labels Oct 23, 2026
@marksvc
marksvc marked this pull request as ready for review October 23, 2026 16:15
@marksvc
marksvc deployed to screenshot_diff October 23, 2026 16:22 — with GitHub Actions Active
@marksvc
marksvc deployed to screenshot_diff October 23, 2026 16:34 — with GitHub Actions Active

This branch was successfully deployed

1 active deployment
screenshot_diff — 4f2e9e1f Deployed Oct 23, 2026 by marksvc via Compare Screenshots #1192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review-author-sign-off PR author has looked at AI review of change and determined no findings warrant further changes testing not required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant