Refuse a token id a JSON number can no longer carry - #107
portdeveloper merged 1 commit into
Conversation
|
Re-checked the diff against the scope before you spend time on it, and one line of it was thinner than the issue asks. "Cover raw JSON input above the safe-integer boundary and a native-tool numeric argument": I had the JSON path and the exported helpers, but nothing asserting the action boundary when a real JS number arrives directly, which is the shape a native tool call takes. Added that test and force-pushed; nothing else changed. Red test redone properly as well. My first run of it was wrong: the fix was already committed, so stashing removed only the test and the suite passed for the wrong reason. Checking the sources out at the previous commit while keeping the tests gives the honest number: six of the nine fail, four at the action boundary and two at the helpers. Three of those six are the missing refusal itself; the other three fail because the Everything else stands: 524 pass locally, the three failures in |
1764353 to
90d43fc
Compare
The precision is gone before any line of this repo runs. JSON.parse turns 9007199254740993 into 9007199254740992, so parseAction never sees the id the model wrote, String() then makes it an ordinary decimal, and requireTokenId from portdeveloper#105 accepts it because it is a legal uint256. The transfer is complete, signable, and for a different token. Nothing downstream can recover the digit, so this refuses rather than repairs, at the two places a number can enter. At the action boundary the check sits before the String() coercion, because after it the value is indistinguishable from an id somebody meant. In requireTokenId only the number branch changes: strings and bigints keep the whole uint256 range, so 9007199254740993 remains a usable id when it is written as a string, which is what the refusal tells the operator to do. runAction gained a transferNft seam beside the sendToken one it already had. Without it the ordering is unobservable: a test can see a refusal but not whether the wallet was reached first, and ordering is the entire fix here. Tests: the model output from the issue and the same number arriving as a native tool argument, both asserting the refusal and that the stubbed wallet recorded no call; the safe-integer boundary accepted as a number and the next value refused; the same id accepted as a string; the exported helpers refusing the number; an exact large string decoded back out of the calldata unchanged; and uint256 max still legal. Six of the nine fail with the sources checked out at the previous commit.
90d43fc to
7568045
Compare
2 participants
Closes #106.
What changed
The precision is gone before any line of this repo runs.
JSON.parseturns9007199254740993into9007199254740992, soparseActionnever sees the id the model wrote,String()then makes it an ordinary decimal, andrequireTokenIdfrom #105 accepts it because it is a legaluint256. The transfer that comes out is complete, signable, and for a different token.Nothing downstream can recover the digit, so this refuses rather than repairs, at the two places a number can enter:
runAction,transfer_nft): the check sits before theString(a.tokenId)coercion. After it the value is indistinguishable from an id somebody meant, so the ordering is the fix rather than a detail of it.requireTokenId: only thenumberbranch changes. Strings and bigints keep the wholeuint256range, so9007199254740993stays a usable id when written as a string, which is exactly what the refusal tells the operator to do.0is still an id and hex still works.runActiongained atransferNftseam beside thesendTokenone it already had. Without it the ordering is unobservable: a test can see a refusal but not whether the wallet was reached first. Production callers pass neither, so the executed path is unchanged.Verification
runActionas a real JS number rather than throughparseAction: same boundary, same refusal, wallet still untouched.9007199254740991accepted as a number, the next value refused as a number and accepted as a string.transferNftbefore it needs a session, which is reachable only because Refuse an unreadable NFT token id instead of throwing or encoding zero #105 moved the id check ahead of the session check.uint256max still legal, so the guard did not narrow the range.transferNftseam does not exist yet, and I would rather say so than count them as evidence.npm test: 524 pass. Three tests intest/native-tools-cli-exit.test.mjsfail in my environment, identically with and without this diff, and CI is green on master, so they are my local Node 20 against your Node 22 rather than anything from this change.Scope
Nothing outside NFT token-id precision: no ABI change, no transaction-flow change, and no touching other numeric fields on other actions even where the same shape exists.