fix: prevent reference creation when CREATE permission check fails by Victorthedev · Pull Request #7232 · surrealdb/surrealdb · GitHub
Skip to content

fix: prevent reference creation when CREATE permission check fails#7232

Open
Victorthedev wants to merge 7 commits intosurrealdb:mainfrom
Victorthedev:fix/reference-created-on-permission-denied
Open

fix: prevent reference creation when CREATE permission check fails#7232
Victorthedev wants to merge 7 commits intosurrealdb:mainfrom
Victorthedev:fix/reference-created-on-permission-denied

Conversation

@Victorthedev
Copy link
Copy Markdown

@Victorthedev Victorthedev commented Apr 8, 2026 •
edited
Loading

Fixes #7208

References were being written to the database inside process_table_fields before check_permissions_table ran. This meant that when a WHERE clause permission denied the CREATE, the reference was already persisted as a ghost reference.

Fix: extract reference writing into a new process_table_references method that runs after check_permissions_table in all CREATE, UPDATE, INSERT, UPSERT and RELATE paths.

Added language tests to reproduce the bug and verify the fix:

  • 7208_reference_create_permission_import.surql: sets up schema
    with a table whose CREATE permission uses WHERE false, and a
    REFERENCE field pointing to another table
  • 7208_reference_create_permission.surql: verifies that when a
    record user attempts to CREATE a record and is denied, no ghost
    reference is left behind on the referenced record

Thank you for submitting this pull request. We really appreciate you spending the time to work on SurrealDB. 🚀 🎉

Before proceeding, please add any of the following labels that apply:

  • breaking-change if this PR includes a breaking change. This label is not needed for bug fixes, which change output in line with a user's original expectations.
  • needs-documentation if documentation is needed to explain the change made in the PR.
  • Modifies env vars or commands if any changes are made to environment variables or command flags.

What is the motivation?

Warning

No details provided.

What does this change do?

Warning

No details provided.

What is your testing strategy?

Warning

No details provided.

Security Considerations

Warning

No details provided.

Is this related to any issues?

  • No related issues

Have you read the Contributing Guidelines?

@Victorthedev Victorthedev requested a review from a team as a code owner April 8, 2026 15:33
claude[bot]
claude Bot reviewed Apr 8, 2026
View reviewed changes
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions github-actions Bot added the community PRs from the community. This label is used to ensure all community PRs get reviewed. label Apr 8, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

@itsezc itsezc self-assigned this Apr 9, 2026
@itsezc
Copy link
Copy Markdown
Contributor

itsezc commented Apr 10, 2026 •
edited
Loading

Hi, thanks for the PR and the contribution! would it be possible to do the following:

  • Add a test for UPDATE or UPSERT with denied permissions specifically FOR update WHERE false scenarios
  • Revert any files which aren't related (i.e. insert.rs:97-98)

and we should be good to go!

itsezc
itsezc previously requested changes Apr 10, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@itsezc itsezc left a comment

Choose a reason for hiding this comment

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

Need further tests, ideally for UPDATE and UPSERT with denied permissions

Comment thread surrealdb/core/src/doc/insert.rs
@Victorthedev
Copy link
Copy Markdown
Author

Victorthedev commented Apr 11, 2026

Hi, thanks for the PR and the contribution! would it be possible to do the following:

  • Add a test for UPDATE or UPSERT with denied permissions specifically FOR update WHERE false scenarios
  • Revert any files which aren't related (i.e. insert.rs:97-98)

and we should be good to go!

Hi, good afternoon!

I have reverted the change in insert.rs and added tests for UPDATE and UPSERT

Please let me know if I need to do anything else, thank you!

@itsezc
Copy link
Copy Markdown
Contributor

itsezc commented Apr 13, 2026

Hi, thanks again for the tests and changes. Just a few formatting / linting issues to clean up and run the language tests locally for one engine and look for the failing cases, let me know if you need any assistance for this

@Victorthedev
fix: prevent reference creation when CREATE permission check fails
a3f8cd9 
Fixes surrealdb#7208

References were being written to the database inside
process_table_fields before check_permissions_table ran.
This meant that when a WHERE clause permission denied the
CREATE, the reference was already persisted as a ghost reference.

Fix: extract reference writing into a new process_table_references
method that runs after check_permissions_table in all CREATE,
UPDATE, INSERT, UPSERT and RELATE paths.
@Victorthedev
fix: revert unrelated change in insert.rs RecordExists pattern
50647e5
@Victorthedev
fix: added UPDATE/UPSERT permission tests
c6eac1c
@Victorthedev
fix: added trailing space to end of file
2448710
@Victorthedev Victorthedev force-pushed the fix/reference-created-on-permission-denied branch from 84a0cff to 2448710 Compare April 16, 2026 23:09
@Victorthedev
Copy link
Copy Markdown
Author

Victorthedev commented Apr 18, 2026

Hi, thanks again for the tests and changes. Just a few formatting / linting issues to clean up and run the language tests locally for one engine and look for the failing cases, let me know if you need any assistance for this

Hi, thanks for the feedback!

I ran the full language test suite locally on the mem engine. this branch has 9 failures, all of which are floating point precision differences that exist identically on main as well:

  • language/functions/geo/bearing.surql — floating point precision difference
  • language/functions/geo/area.surql — floating point precision difference
  • language/functions/math/sin.surql — floating point precision difference

None of these are introduced by this PR. Happy to track them in separate issues if helpful.

Regarding formatting/linting, I ran cargo fmt and cargo clippy and only received nightly-only feature warnings (e.g. wrap_comments, imports_granularity) which don't affect stable builds. Could you point me to the specific files or lines you're referring to? I want to make sure I address exactly what you have in mind.

Thanks again for your time reviewing this!

@Victorthedev
Copy link
Copy Markdown
Author

Victorthedev commented Apr 23, 2026

Hi, thanks again for the tests and changes. Just a few formatting / linting issues to clean up and run the language tests locally for one engine and look for the failing cases, let me know if you need any assistance for this

Hi, thanks for the feedback!

I ran the full language test suite locally on the mem engine. this branch has 9 failures, all of which are floating point precision differences that exist identically on main as well:

  • language/functions/geo/bearing.surql — floating point precision difference
  • language/functions/geo/area.surql — floating point precision difference
  • language/functions/math/sin.surql — floating point precision difference

None of these are introduced by this PR. Happy to track them in separate issues if helpful.

Regarding formatting/linting, I ran cargo fmt and cargo clippy and only received nightly-only feature warnings (e.g. wrap_comments, imports_granularity) which don't affect stable builds. Could you point me to the specific files or lines you're referring to? I want to make sure I address exactly what you have in mind.

Thanks again for your time reviewing this!

@itsezc

@itsezc itsezc enabled auto-merge April 23, 2026 06:37
@itsezc
Copy link
Copy Markdown
Contributor

itsezc commented Apr 23, 2026 •
edited
Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@claude claude[bot] claude[bot] left review comments

@itsezc itsezc itsezc left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@itsezc itsezc

Labels

community PRs from the community. This label is used to ensure all community PRs get reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

References are created even if CREATE permission check fails

2 participants

@Victorthedev @itsezc