Skip to content

Clarify guarantees for Hash#1870

Open
mootz12 wants to merge 1 commit into
mainfrom
hash-docs-improvement
Open

Clarify guarantees for Hash#1870
mootz12 wants to merge 1 commit into
mainfrom
hash-docs-improvement

Conversation

@mootz12
Copy link
Copy Markdown
Contributor

@mootz12 mootz12 commented May 12, 2026

What

Update the Hash docs to clarify what guarantees the type provides.

Hash<N> values returned by SDK crypto functions are produced by secure cryptographic hash functions, and host-managed __check_auth receives a host-constructed authentication payload hash. However, Hash<N> is represented at contract boundaries as BytesN<N>, so accepting it as a general contract argument does not prove that caller-supplied bytes came from a hash function.

Why

The previous docs overstated the construction/provenance guarantees of Hash<N>.

Known limitations

None

@claude
Copy link
Copy Markdown

claude Bot commented May 12, 2026

Claude finished @mootz12's task in 1m 25s —— View job


Review — Clarify guarantees for Hash

  • Read changed files
  • Inspect Hash<N> type definition and surrounding context
  • Check _migrating.rs doc updates
  • Verify wording accuracy of new guarantees
  • Post review feedback

Summary

Doc-only change. The previous wording was materially misleading: it claimed Hash<N> "can only be constructed in contexts where the value has been generated by a secure cryptographic function", but Hash<N> has a direct TryFromValForContractFn impl (soroban-sdk/src/crypto.rs:109-115) and no TryFromVal<Env, Val> impl. That means a contract author can write fn foo(env: Env, h: Hash<32>) and the host will happily hand them attacker-controlled bytes typed as Hash<32>. The new wording correctly describes the actual guarantee surface:

  1. SDK-produced: Crypto::sha256 / Crypto::keccak256 return values.
  2. Host-produced: the signature_payload parameter of CustomAccountInterface::__check_auth.
  3. Anywhere else (general entrypoint args, storage) → no provenance guarantee.

I verified all three claims against the code (crypto.rs:132-143, auth.rs:97-108, try_from_val_for_contract_fn.rs:1-58). The new docs are accurate, and the steering toward BytesN<N> for externally-supplied digests is the right call. Approve from a correctness standpoint.

Notes (all minor / non-blocking)

soroban-sdk/src/crypto.rs:28-29 — small grammar nit

so accepting it as a general contract entrypoint argument does not prove that the
caller supplied bytes were produced by a secure cryptographic hash function.

caller supplied reads as a clause; it should be a hyphenated compound adjective: caller-supplied bytes.

soroban-sdk/src/crypto.rs:32 — pre-existing malformed markdown, drive-by candidate

/// **__Note:_** A Hash should not be used with storage, …

The asterisk/underscore pairing is unbalanced (**__…_**), which renders inconsistently across markdown engines. Not introduced by this PR, but it sits one line below the lines you're already touching, so this is a low-cost drive-by fix to **Note:** (or **_Note:_**). Feel free to leave it for a follow-up.

soroban-sdk/src/crypto.rs:40-47 — internal from_bytes doc is now slightly out of step

/// This is intended for test-only, since `Hash` type is only meant to be
/// constructed via secure manners.
#[cfg(test)]
pub(crate) fn from_bytes(bytes: BytesN<N>) -> Self {

This still uses the old "only meant to be constructed via secure manners" framing that the public doc just walked back. It's pub(crate) + cfg(test), so user-invisible — but if you want internal consistency, "constructed via secure means or via contract entrypoint args" would now be the accurate statement.

Optional — consider a stronger recommendation

The current wording says accepting Hash<N> "does not prove" provenance and tells users to use BytesN<N> "unless the calling context provides an explicit provenance guarantee". For most contract authors, the only such context they actually control is __check_auth. It might be worth being more directive — e.g. "Do not use Hash<N> as a contract entrypoint parameter type; prefer BytesN<N> and compute the hash inside the contract." This is a judgment call and the current wording is defensible; flagging in case you want the stricter version.

soroban-sdk/src/_migrating.rs:267-272 — Reads well. "ABI-compatible" is the right replacement for the old "interchangeable" (which suggested symmetry that doesn't exist — you can't construct a Hash<32> from a BytesN<32> outside test or contract-entrypoint contexts).

Other dimensions checked

  • Security: This PR strengthens the security posture by correcting docs that could lead contract authors to trust attacker-controlled bytes. No code paths change.
  • Performance / bugs: N/A — doc-only.
  • Breaking changes: None — Hash<N> API surface is unchanged.
    · Branch: hash-docs-improvement

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Soroban SDK documentation to accurately describe what the crypto::Hash<N> type does (and does not) guarantee about byte provenance, especially across contract ABI boundaries.

Changes:

  • Clarifies that Hash<N> returned from Crypto::sha256 / Crypto::keccak256 has host/SDK provenance guarantees.
  • Clarifies that Hash<N> used as a general contract argument is ABI-represented as BytesN<N>, and therefore does not prove caller-supplied bytes came from a hash function.
  • Updates the migration guide text for CustomAccountInterface::__check_auth to describe the host-constructed signature_payload guarantee more precisely.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
soroban-sdk/src/crypto.rs Refines Hash<N> rustdoc to distinguish guaranteed provenance from hash-returning APIs vs. untrusted ABI inputs.
soroban-sdk/src/_migrating.rs Updates v20→v21 migration notes to clarify Hash<32> vs BytesN<32> ABI-compatibility and provenance in host-managed __check_auth.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants