fix: return NotAuthorized for bookmark auth failures - #735
Open
TheFaith-Code wants to merge 7 commits into
Open
Conversation
|
@TheFaith-Code Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
update_campaign and update_campaign_description publish the metadata updated event with a (old_title, old_description, new_title, new_description) 4-tuple, but two tests still unpacked it as a 2-tuple, causing UnexpectedSize. Align the assertions with the emitted shape.
require_auth failures are escalated to a panic by the SDK. A panic escaping a contract call hits the generated extern "C" entrypoint (nounwind), so the host's catch_unwind can never intercept it and the process aborts. Catch it inside the contract body below that boundary, gated on the testutils feature so wasm/production builds keep the standard panic behavior and still compile without std.
davidmaronio
requested changes
Aug 3, 2026
davidmaronio
left a comment
davidmaronio
left a comment
Contributor
There was a problem hiding this comment.
thanks for digging into this, the writeup shows real investigation into the panic path and the regression test for removing someone else's bookmark is a scenario we genuinely didn't cover. a few things need to change before this can land though:
- src/bookmarks.rs:20-35: the catch_unwind path only exists under cfg(feature = "testutils"), so production wasm still traps with a host auth error and never returns Error::NotAuthorized. that means issue #672 isn't actually fixed on-chain, and the test now asserts behavior the deployed contract doesn't have. please drop require_auth_or_not_authorized entirely and keep plain user.require_auth() in the contract.
- src/tests/test_bookmarks.rs:123-125: you can write this regression test with no contract change. without mock_all_auths, client.try_remove_saved_campaign(&contributor2, &campaign_id) returns an invoke/host auth error you can assert on directly. that keeps test and production semantics identical.
- src/bookmarks.rs:25-28: even if we kept the wrapper, catch_unwind maps every panic to NotAuthorized, so any future unrelated panic in that closure becomes a fake auth error in tests.
- src/tests/test_bookmarks.rs:94-116: prefer setup_env + create_campaign over hand-building a Campaign struct with set_campaign, so the test doesn't silently drift when the struct gains fields.
happy to re-review once the contract-side wrapper is removed and the test asserts the host auth failure instead.
…h error in tests - Drop wrapper from , keeping standard in contract methods. - Update in to use and instead of manual struct construction. - Assert host authorization failure directly with and to match production contract semantics.
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
Fixes the bookmark authorization regression for saved-campaign removal by ensuring an auth mismatch returns
NotAuthorizedinstead of bubbling up as a host panic.What changed
require_auth()failures to the contract'sError::NotAuthorized.Why
Previously, bookmark removal could fail with a host auth panic when the supplied address did not have the required authorization. This made the behavior inconsistent with the contract’s intended error model and made the regression hard to test.
Verification
cargo fmt --checkcargo test --features testutilsCloses #672