fix: [DO NOT MERGE] sign the metadata the same way it is sent - #9848
Open
LautaroPetaccio wants to merge 1 commit into
Open
fix: [DO NOT MERGE] sign the metadata the same way it is sent#9848LautaroPetaccio wants to merge 1 commit into
LautaroPetaccio wants to merge 1 commit into
Conversation
`NewFromRaw` folded the whole payload before signing:
$"{method}:{path}:{unixTimestamp}:{raw}".ToLowerInvariant()
while the request delivered `x-identity-metadata` unfolded. The two disagreed
about the metadata's casing, and the signature only ever covered the folded
form -- so a key or value could be re-cased between signing and delivery and
still verify. Services read the delivered header, which meant they were
authorizing on bytes the signature never covered.
Now only the method and path are lowercased and the metadata is interpolated
verbatim, matching createPayload in @dcl/crypto-middleware 6.x. What is signed
here is what every Decentraland verifier reconstructs.
This is the sole payload builder behind roughly twenty signed call sites, so it
is also what lets the canonicalMetadataKeys fallbacks come out of the services.
They exist only because clients sign the older format.
DEPLOY ORDERING: ships only after every service this client calls verifies the
6.x payload. A verifier still on the old format folds both sides, so it accepts
this signature while the metadata is all-lowercase and rejects it the moment any
uppercase appears -- and scene metadata is camelCase throughout (`realmName`,
`sceneId`, `isGuest`, `realm.serverName`). camera-reel-service is the known
blocker: it verifies through `dcl-crypto-middleware-rs`, whose repository is
archived, so its fix cannot currently be released.
Four tests, in the existing WebRequests test assembly. Their expectations are
not guesses: this file was compiled and run standalone, and it produces
`post:/api/quote:<ts>:{"realmName":"main"}` and `get:/status:<ts>:{}` -- method
and path lowercased, metadata untouched, and the empty-metadata default still
the literal `{}` that `NewFromUrl` depends on.
decentraland-bot
added
the
ext-contribution
Contributor
🚦 CI Status
Build skipped — no changes detected under Warnings not reduced: 12613 => 12618 — remove at least 6 warnings to merge. No warnings in files changed by this PR — showing general ones you can remove to unblock (50 of 12618)
|
Contributor
|
Slack notification sent to #explorer-ext-contributions for external review. |
LautaroPetaccio
changed the title
decentraland-bot
left a comment
decentraland-bot
left a comment
Contributor
There was a problem hiding this comment.
STEP 2 — Root-cause check: PASS ✅
The PR fixes the root cause. The old .ToLowerInvariant() on the entire payload string folded the metadata's casing in the signature, but the x-identity-metadata header delivered the metadata verbatim via WebRequestHeadersInfo.WithSign(). This made the signature cover a lowercased representation while services verified against the original-cased header bytes — a genuine signature coverage gap. The fix correctly applies lowercasing only to method and path, leaving metadata verbatim so signed bytes match delivered bytes.
STEP 3 — Design & integration: PASS ✅
No new types, systems, lifecycle management, or persistent state introduced. The change modifies the existing NewFromRaw factory method on the WebRequestSignInfo readonly struct — the sole payload builder behind ~20 signed call sites. This is the correct and minimal location for the fix.
Owner search: Not applicable — no new long-lived unit is introduced. The struct is stateless.
Teardown trace: No subscriptions, event hookups, connections, or resources added.
Consumer note: Two call sites in PulseMultiplayerBus.Handshake and RPCSocialServices bypass WebRequestSignInfo and construct payloads manually with hardcoded {} metadata. These are unaffected by this change (lowercase of {} is {}), but are a pre-existing maintenance concern outside the scope of this PR.
STEP 4 — Member audit: PASS ✅
No new public properties or accessors added. The existing StringToSign property is unchanged.
STEP 5 — Line-level review
One P2 finding — see inline comment.
STEP 6 — Complexity: COMPLEX
Touches auth/web3 signing code paths — the signature payload builder used by every signed web request in the client.
STEP 7 — QA: YES
Modifies runtime code that affects how every signed request's payload is constructed. Changes authentication behavior that is deploy-ordering sensitive.
STEP 8 — Non-blocking warnings: None
Main scene not modified.
Security review: No issues found
The change is a security improvement — it closes a gap where metadata casing wasn't covered by the signature. No new vulnerabilities introduced. No hardcoded secrets, no injection risks, no input validation regressions. The string.IsNullOrEmpty fallback to "{}" in NewFromRaw matches the same fallback in WithSign(), so signed bytes always equal delivered bytes.
Deploy ordering (services must upgrade to @dcl/crypto-middleware 6.x before this ships) is correctly flagged in the PR description as a prerequisite.
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies the auth/web3 signing payload builder (WebRequestSignInfo.NewFromRaw) used by every signed request in the client.
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by unknown (<@unknown>) via Slack
Comment on lines
+25
to
+39
| /// <summary> | ||
| /// Builds the ADR-44 payload the auth chain is signed over. | ||
| /// <para> | ||
| /// The method and path are lowercased; the timestamp and metadata are interpolated verbatim. | ||
| /// That last part matters: folding the whole string, as this did before, left the metadata's | ||
| /// casing outside the signature while the request still delivered `x-identity-metadata` | ||
| /// unfolded. A key or value could therefore be re-cased between signing and delivery and still | ||
| /// verify, and services read that header — so they were authorizing on bytes the signature | ||
| /// never covered. | ||
| /// </para> | ||
| /// <para> | ||
| /// Matches createPayload in @dcl/crypto-middleware 6.x, so what is signed here is exactly what | ||
| /// every Decentraland verifier reconstructs. | ||
| /// </para> | ||
| /// </summary> |
Contributor
There was a problem hiding this comment.
[P2] XML doc narrates external behavior (CLAUDE.md §11 anti-patterns).
The extended comment describes what services and verifiers do ("services read that header", "every Decentraland verifier reconstructs") rather than what this code itself does or guarantees. The spec reference (@dcl/crypto-middleware 6.x) is valuable; the external behavior narration should be trimmed.
Suggested change
| /// <summary> | |
| /// Builds the ADR-44 payload the auth chain is signed over. | |
| /// <para> | |
| /// The method and path are lowercased; the timestamp and metadata are interpolated verbatim. | |
| /// That last part matters: folding the whole string, as this did before, left the metadata's | |
| /// casing outside the signature while the request still delivered `x-identity-metadata` | |
| /// unfolded. A key or value could therefore be re-cased between signing and delivery and still | |
| /// verify, and services read that header — so they were authorizing on bytes the signature | |
| /// never covered. | |
| /// </para> | |
| /// <para> | |
| /// Matches createPayload in @dcl/crypto-middleware 6.x, so what is signed here is exactly what | |
| /// every Decentraland verifier reconstructs. | |
| /// </para> | |
| /// </summary> | |
| /// <summary> | |
| /// Builds the ADR-44 payload the auth chain is signed over. | |
| /// <para> | |
| /// The method and path are lowercased; the timestamp and metadata are interpolated verbatim | |
| /// so the signed bytes match the delivered <c>x-identity-metadata</c> header byte-for-byte. | |
| /// </para> | |
| /// <para> | |
| /// Matches <c>createPayload</c> in <c>@dcl/crypto-middleware</c> 6.x. | |
| /// </para> | |
| /// </summary> |
Contributor
|
PR #9848, run #32776904160 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
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.
Signs the payload format
@dcl/crypto-middleware6.x verifies.What changed
WebRequestSignInfo.NewFromRawfolded the whole payload before signing:while the request delivered
x-identity-metadataunfolded. The signature only ever covered the folded form, so a key or value could be re-cased between signing and delivery and still verify — and services read the delivered header, so they were authorizing on bytes the signature never covered.Now only the method and path are lowercased and the metadata is interpolated verbatim. This is the sole payload builder behind roughly twenty signed call sites, so it is the whole client-side change.
Testing
Four tests in the existing
DCL.WebRequests.Testsassembly: method and path normalized, metadata verbatim, two metadata strings differing only in case now producing different payloads, and the empty-metadata default still the literal{}thatNewFromUrldepends on.Their expectations are not guesses. Unity cannot be built here, so I compiled and ran
WebRequestSignInfo.csstandalone against a stub for its one non-Unity dependency. It produces:Method and path lowercased,
realmNameuntouched, empty metadata still{}. The assertions are pinned to that observed output rather than to reasoning about it. Running them under the Unity test runner is still CI's job.Do not merge before every service this client calls verifies the 6.x payload.
A verifier still on the old format folds both sides. It therefore accepts this signature while the metadata is all-lowercase, and rejects it the moment any uppercase appears — and this client's metadata is camelCase throughout (
realmName,sceneId,isGuest,realm.serverName). So the breakage is not gradual: every signed request carrying real scene metadata starts failing at once.