fix: bind related-table allowRead to a proper resource instance - #1532
Merged
Conversation
The related-table read check inside the built-in allowRead invoked `relatedTable.prototype.allowRead.call(null, user, property, context)` — binding `this` to null. It happens to be harmless today because the built-in allowRead is closure-based and never reads `this`, but null is an improper binding and would break if the base hook ever referenced instance state. Construct a related-type resource instance and invoke allowRead on it instead, so `this` is a valid resource of the related type. This is behavior-preserving: the same base table-level RBAC check runs (relatedTable is the base table class), and it deliberately does not consult a related-table allowRead *override* — default RBAC has no per-record granularity and overrides are grant hooks evaluated on their own read paths. Verified: tsc clean; operations RBAC integration suite 30/30 (attribute/table read permissions unaffected). Refs #1487 (allowRead consolidation), #1422. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the permission checking logic in resources/Table.ts by instantiating a properly-bound instance of the related table (relatedResource) to invoke allowRead, rather than calling it with a null context via .call(null, ...). This ensures that this correctly references a valid resource of the related type. There are no review comments, and I have no additional feedback to provide.
Contributor
|
Reviewed; no blockers found. |
cb1kenobi
approved these changes
Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The related-table read check inside the built-in
allowReadinvokedrelatedTable.prototype.allowRead.call(null, user, property, context)— bindingthistonull. It's harmless today because the built-inallowReadis closure-based and never readsthis, butnullis an improper binding that would break if the base hook ever referenced instance state.This constructs a related-type resource instance and invokes
allowReadon it, sothisis a valid resource of the related type.Scope (deliberately narrow)
relatedTableis the base table class, so the resolved method is identical), only with a correctthis.allowReadoverride. Per theallowReaddesign, it is a grant hook over RBAC, default RBAC has no per-record granularity, and overrides are evaluated on their own read paths. Propagating overrides to related-table reads would be a behavior change and is intentionally out of scope here.This is the safe "signature/binding correction" slice of the #1422 / #1487
allowReadreview — the fail-closed hardening is in #1489.Testing
tsc --noEmitclean.integrationTests/server/operation-user-rbac.test.ts— 30/30 (table/attribute read-permission paths unaffected).No new test: the change is behavior-preserving, so existing relationship + RBAC coverage applies. Happy to add a dedicated relationship-with-attribute-permission regression if preferred.
Refs #1487, #1422.
🤖 Generated with Claude Code