refactor(security-practices): port security article code samples to Tolk#2134
refactor(security-practices): port security article code samples to Tolk#2134aigerimu wants to merge 6 commits into
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughConverts security best-practice code samples in documentation from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
aigerimu
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Thanks for the detailed security documentation updates in Per-comment submission: 9 posted, 2 failed. Unposted inline comments (raw text):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
contract-dev/techniques/security.mdx (1)
544-552:⚠️ Potential issue | 🟠 MajorThis section is not fully ported to Tolk yet.
The vulnerable snippet still uses a
funcfence (Line 546) while the paired secure snippet istolk. For this PR’s objective (“port code samples to Tolk”), this section should be fully converted for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contract-dev/techniques/security.mdx` around lines 544 - 552, The vulnerable snippet uses a `func` code fence while the paired secure snippet is `tolk`, so update the vulnerable example to use a `tolk` fence and ensure both examples are fully ported to Tolk; locate the snippet containing `infoDict.delete(index);` and replace the surrounding fence language from `func` to `tolk`, then verify the secure implementation block is also a complete Tolk block (matching fence type and content) so the section is fully converted and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contract-dev/techniques/security.mdx`:
- Around line 340-342: The Vote struct's votes field is declared as a signed int
(Vote.votes : int32) but Storage.votes is unsigned (Storage.votes : uint32),
causing a signed/unsigned mismatch at the addition site (storage.votes +=
msg.votes); change Vote.votes to uint32 to match Storage.votes and ensure the
addition uses the same unsigned type throughout (update the Vote struct
declaration and any references to Vote.votes accordingly).
- Around line 95-103: The Tolk external-message handlers (function
onExternalMessage and similar examples) call the wrong API: replace the call to
acceptMessage() with the documented acceptExternalMessage() to correctly accept
incoming external messages; update each usage in the Tolk code blocks that
currently call acceptMessage() so they call acceptExternalMessage() instead
(e.g., inside onExternalMessage and the two other external-message examples).
- Around line 270-277: The example uses msg.send(128) which is
SendRemainingBalance and will drain the contract; update the example to use a
non-draining send mode (e.g., msg.send(0) for a regular send or msg.send(3) to
forward fees) or explicitly document why draining is intended; locate the
createMessage call and the msg.send(128) invocation (and note the presence of
seqno replay protection) and replace the 128 mode or add a clear comment
explaining the deliberate use of SendRemainingBalance.
- Around line 64-71: The secure-example snippets use the outdated Tolk signature
onInternalMessage(msgValue: int, inMsgFull: cell, inMsgBody: slice) and the bare
sender variable; update both occurrences (the snippets handling empty message
and the later similar block) to the modern entrypoint signature fun
onInternalMessage(in: InMessage) and replace any use of sender with
in.senderAddress; adjust any references to message parts to use fields on the
InMessage (e.g., in.body or in.raw) so sendRawMessage is called with the correct
in-derived values and types (keep the same destruction logic but sourced from
in).
---
Duplicate comments:
In `@contract-dev/techniques/security.mdx`:
- Around line 544-552: The vulnerable snippet uses a `func` code fence while the
paired secure snippet is `tolk`, so update the vulnerable example to use a
`tolk` fence and ensure both examples are fully ported to Tolk; locate the
snippet containing `infoDict.delete(index);` and replace the surrounding fence
language from `func` to `tolk`, then verify the secure implementation block is
also a complete Tolk block (matching fence type and content) so the section is
fully converted and consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca165394-0a02-4b9f-90ad-50f083258940
📒 Files selected for processing (1)
contract-dev/techniques/security.mdx
| TakeMoney => { | ||
| assert (in.senderAddress == storage.oneContractAddress) | ||
| throw ErrCode.InvalidMoneyProvider; | ||
| // ...further processing... | ||
| } |
There was a problem hiding this comment.
[HIGH] Commented TODO-style ellipsis in Tolk example
The Tolk example for AnotherContract includes a placeholder comment // ...further processing... inside a runnable snippet. The documentation style guide requires using language-appropriate comments for omissions and explicitly forbids literal ellipses used as pseudo-code placeholders in examples, especially when they could be mistaken for syntax. Keeping this placeholder as-is violates the style rules for documentation snippets and may confuse readers about whether the code is complete or executable. The surrounding example is otherwise realistic and should either show minimal real logic or use a neutral, non-elliptic comment to indicate omitted details (see https://github.com/ton-org/docs/blob/main/contribute/style-guide-extended.mdx?plain=1#L640-L652 for the underlying rule).
| TakeMoney => { | |
| assert (in.senderAddress == storage.oneContractAddress) | |
| throw ErrCode.InvalidMoneyProvider; | |
| // ...further processing... | |
| } | |
| TakeMoney => { | |
| assert (in.senderAddress == storage.oneContractAddress) | |
| throw ErrCode.InvalidMoneyProvider; | |
| // Add application-specific processing here. | |
| } |
Please leave a reaction 👍/👎 to this suggestion to improve future reviews for everyone!
|
Thanks for the updates in Per-comment submission: 1 posted, 1 failed. Unposted inline comments (raw text):
|
| var fromVotes = getVotingPower(votes, from); | ||
| var toVotes = getVotingPower(votes, to); | ||
|
|
||
| assert (fromVotes >= amount) throw 998; // Validate sufficient balance |
There was a problem hiding this comment.
This secure version still accepts a negative amount. For example, amount = -10 passes fromVotes >= amount, then increases fromVotes and decreases toVotes, which is exactly the signed-value class of bug this section warns about. Please either make amount an unsigned type or add an explicit amount >= 0 check before the balance check.
| ;; ... | ||
| ```tolk | ||
| fun onExternalMessage(inMsg: slice) { | ||
| acceptMessage(); |
There was a problem hiding this comment.
acceptMessage() is not the Tolk API used by the standard library docs for accepting external messages; the documented function is acceptExternalMessage()
this appears again in the secure example below at line 142, so both examples should use acceptExternalMessage() to stay copy-pasteable and consistent with the earlier replay-protection snippet
| assert (authorizedAdmin(sender)) throw ErrCode.Unauthorized; | ||
| assert (validateCode(newCode)) throw ErrCode.InvalidCode; | ||
|
|
||
| contract.setCode(newCode); |
There was a problem hiding this comment.
Tolk exposes contract.setCodePostponed(newCodeCell), not contract.setCode(newCode). as written, the final secure implementation points readers to a non-existent API; please use contract.setCodePostponed(newCode) here, matching the Tolk standard-library docs and the upgrade guide
https://docs.ton.org/tolk/features/standard-library#contract-setcodepostponed-newcodecell
3 participants
closes #1186
Summary by CodeRabbit