Initial ARC Support - #15
Merged
Merged
Conversation
|
Important Review skippedToo many files! This PR contains 272 files, which is 172 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (272)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR adds Fabric account identity derivation, encrypted identity backups, browser state handling, wallet authorization, document-offer and peer routing, sidechain state APIs, Hub setup security, desktop startup probing, semantic asset wiring, and related UI and test updates. ChangesHub identity, wallet, and client flows
Hub runtime, peer, document, and sidechain flows
Runtime, assets, and validation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
martindale
marked this pull request as draft
June 25, 2026 11:37
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 3 medium 17 high |
| Security | 16 critical 64 high |
🟢 Metrics 4278 complexity · 2 duplication
Metric Results Complexity 4278 Duplication 2
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
scripts/desktop.js (1)
160-213: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
waitForHubpolling can hang if the socket connects but never responds.The overall
timeoutMsdeadline is only evaluated inside theres.on('end')andreq.on('error')handlers. If the port accepts the connection but never sends a response (or never ends the body), neither handler fires,pingis never rescheduled, and the returned Promise never settles — the deadline is silently bypassed. Add a per-request timeout so a stalled socket is forced into the error/retry path.🛡️ Proposed fix: add a request timeout
const req = http.get( settingsUrl, { headers: { Accept: 'application/json' - } + }, + timeout: 5000 }, (res) => {Add alongside the existing
req.on('error', ...):req.on('timeout', () => { req.destroy(new Error('Hub /settings request timed out')); });(
req.destroy(err)will surface through the existingerrorhandler, which already re-checks the deadline and reschedules.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/desktop.js` around lines 160 - 213, The waitForHub polling in ping can hang when http.get connects but never finishes the response, because the existing timeout only runs in the end and error paths. Add a per-request timeout on the req object in ping so stalled requests are destroyed and routed through the existing req.on('error') retry logic, ensuring the overall deadline in waitForHub is still enforced.functions/bitcoinClient.js (2)
1246-1262: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
bypassCacheandmaxCacheAgeMson fallback.Line 1262 always passes
Infinity, so a manual refresh or explicitmaxCacheAgeMs: 0can still return stale wallet balances afterfetchWalletSummarythrows.Proposed fix
- const cached = cacheKey ? getCachedBalance(cacheKey, Infinity) : null; + const cached = cacheKey ? getCachedBalance(cacheKey, maxCacheAgeMs) : null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/bitcoinClient.js` around lines 1246 - 1262, The fallback cache lookup in fetchWalletSummaryWithCache ignores the computed cache policy, so bypassCache and maxCacheAgeMs are not respected after fetchWalletSummary fails. Update the cache read path in fetchWalletSummaryWithCache to use the derived maxCacheAgeMs (or bypassCache) instead of always calling getCachedBalance with Infinity, so explicit refresh requests cannot return stale balances.
1623-1626: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the selected Fabric account for crowdfunding keys.
These paths still derive account
0, while payments now usegetBitcoinBip44AccountForIdentity(identity). In multi-account mode, beneficiary/refund keys won’t match the active wallet account.Proposed fix
+ const accountN = getBitcoinBip44AccountForIdentity(identity); if (masterXprv) { try { - const d = deriveFabricBitcoinAccountKeys(masterXprv, masterXpub, BITCOIN_PAYMENTS_BIP44_ACCOUNT_INDEX); + const d = deriveFabricBitcoinAccountKeys(masterXprv, masterXpub, accountN);+ const accountN = getBitcoinBip44AccountForIdentity(identity); try { - const d = deriveFabricBitcoinAccountKeys(masterXprv, masterXpub, BITCOIN_PAYMENTS_BIP44_ACCOUNT_INDEX); + const d = deriveFabricBitcoinAccountKeys(masterXprv, masterXpub, accountN);Also applies to: 1667-1669
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/bitcoinClient.js` around lines 1623 - 1626, The crowdfunding key derivation is still hardcoded to account 0, so beneficiary/refund keys can drift from the active Fabric wallet account. Update the crowdfunding derivation in bitcoinClient.js to use the selected identity account via getBitcoinBip44AccountForIdentity(identity) instead of BITCOIN_PAYMENTS_BIP44_ACCOUNT_INDEX, and apply the same change in the related beneficiary/refund key path so deriveFabricBitcoinAccountKeys stays aligned with the active account.actions/documentActions.js (2)
109-117: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLet the upload timeout race the request.
await fetch(...)completes beforePromise.race, so a hung upload cannot be interrupted bytimeoutPromise.Proposed fix
- const fetchPromise = await fetch(assertClientFetchPath('/files'), { + const fetchPromise = fetch(assertClientFetchPath('/files'), {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@actions/documentActions.js` around lines 109 - 117, The upload timeout is not working because fetchPromise is awaited before Promise.race, so a slow or hung request cannot be interrupted. Update the upload flow in documentActions.js so fetch(assertClientFetchPath('/files'), ...) is passed directly into Promise.race alongside timeoutPromise, and keep the existing fileCreation handling in the upload routine.
191-204: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck edit failures before dispatching success.
A 4xx/5xx edit response is parsed and dispatched through
editDocumentSuccess, which makes failed edits look successful.Proposed fix
const response = await fetch( assertClientFetchPath(`/documents/${encodeURIComponent(fabricID)}`), { @@ body: JSON.stringify({ title }) }); + if (!response.ok) { + const errorData = await response.json().catch(() => ({})); + throw new Error(errorData.message || 'Server error'); + } + const document = await response.json();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@actions/documentActions.js` around lines 191 - 204, The edit flow in editDocument is dispatching editDocumentSuccess even when the PATCH request fails, so add a response.ok (or equivalent status check) before parsing and dispatching. In the action that calls fetch for /documents/${encodeURIComponent(fabricID)}, branch on failure to stop the success path and handle the error state instead, then only call editDocumentSuccess(document) when the response is successful.components/IdentityManager.js (2)
1903-1925: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftPersist imported private-key backups encrypted, not watch-only.
When importing an
xprv, local storage only receivesid/xpub, whilenextIdentitykeeps the private key in memory withpasswordProtected: false. After reload the user loses signing access, and before reload the key is not covered by auto-lock. Require a local encryption password and store via the encrypted identity path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/IdentityManager.js` around lines 1903 - 1925, The import flow in IdentityManager is treating xprv-backed identities like watch-only entries by writing only the xpub and marking nextIdentity as passwordProtected false. Update the import path in the local-storage block and the nextIdentity construction so xprv imports require a local encryption password and are persisted through the encrypted identity flow instead of fabric.identity.local. Make sure the logic around xprv, writeStorageJSON, and nextIdentity keeps the private key protected and reloadable.
397-407: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not persist unlocked private keys to extension storage.
This sync writes
xprv/masterXprvintochrome.storage.localwhenever the identity is unlocked, bypassing the encrypted-at-rest flow used forfabric.identity.local. Keep extension sync watch-only unless the key material is encrypted first.Proposed fix
const payload = { id: localIdentity.id, xpub: localIdentity.xpub, - xprv: localIdentity.xprv || undefined, passwordProtected: !!localIdentity.passwordProtected, fabricIdentityMode: localIdentity.fabricIdentityMode || undefined, fabricAccountIndex: localIdentity.fabricAccountIndex, fabricHdRole: localIdentity.fabricHdRole || undefined, - masterXprv: localIdentity.masterXprv || undefined, masterXpub: localIdentity.masterXpub || undefined };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/IdentityManager.js` around lines 397 - 407, The sync payload in IdentityManager’s local identity persistence is writing unlocked private key material directly to extension storage. Update the logic around the localIdentity payload build so it does not include xprv or masterXprv in the chrome.storage.local sync path; keep this flow watch-only unless the key material is first encrypted using the existing encrypted-at-rest mechanism for fabric.identity.local. Verify the payload construction in the local identity save/sync branch only carries public or non-sensitive fields.
🟡 Minor comments (6)
assets/scripts/assets/manifest.json-6-8 (1)
6-8: 🗄️ Data Integrity & Integration | 🟡 MinorRemove duplicate
main.jsentry mapping toindex.min.js.The manifest defines both
index.min.jsandmain.jspointing to the same external CDN URL (https://fabric.pub/index.min.js) with identical integrity hashes. Sincemain.jsis not used as a distinct entry point in the loading scripts and serves no differentiating purpose here, this appears to be an accidental duplicate. Please remove themain.jskey to avoid confusion."main.js": { "src": "https://fabric.pub/index.min.js", "integrity": "sha256-..." }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/scripts/assets/manifest.json` around lines 6 - 8, The manifest has a duplicate asset mapping where main.js points to the same CDN source and integrity as index.min.js, so remove the redundant main.js entry from the assets manifest. Update the manifest object in assets/scripts/assets/manifest.json so only the distinct entry remains, and keep the existing index.min.js mapping unchanged.functions/fabricBrowserState.js-235-238 (1)
235-238: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winClear session unlock state through the resolved browser global.
This function uses
weverywhere else, but Line 236 falls back to directwindow; ifgetFabricBrowserGlobal()resolves a provided browser global, the unlock blob may remain.Proposed fix
- if (typeof window !== 'undefined' && window.sessionStorage) { - window.sessionStorage.removeItem('fabric.identity.unlocked'); + if (w.sessionStorage) { + w.sessionStorage.removeItem('fabric.identity.unlocked'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/fabricBrowserState.js` around lines 235 - 238, The unlock-state cleanup in fabricBrowserState should use the resolved browser global consistently instead of falling back to window. Update the sessionStorage removal inside the unlock cleanup logic to reference the same browser-global variable used elsewhere in the function (for example, the one returned by getFabricBrowserGlobal and stored in w), so the identity key is cleared even when a provided browser global is resolved.components/BitcoinWalletBranchBar.js-12-13 (1)
12-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisplay
masterXpubfor the “Master xpub” label.In account mode,
identity.xpubis the selected account xpub. Useidentity.masterXpubfor the master label, or relabel this as the account xpub.Proposed fix
function BitcoinWalletBranchBar ({ identity }) { const xpub = identity && identity.xpub ? String(identity.xpub) : ''; + const masterXpub = identity && identity.masterXpub ? String(identity.masterXpub) : xpub; const acct = getBitcoinBip44AccountForIdentity(identity || {}); @@ - {xpub ? ( + {masterXpub ? ( <p style={{ margin: 0, color: '`#666`', fontSize: '0.85em', wordBreak: 'break-all' }}> <strong>Master xpub:</strong>{' '} <code> - {xpub.slice(0, 18)}…{xpub.slice(-10)} + {masterXpub.slice(0, 18)}…{masterXpub.slice(-10)} </code> </p>Also applies to: 36-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/BitcoinWalletBranchBar.js` around lines 12 - 13, The “Master xpub” label is currently using the selected account xpub from BitcoinWalletBranchBar, so update the xpub source to use identity.masterXpub for that label and keep identity.xpub only for the account-level display. Adjust the related rendering logic in BitcoinWalletBranchBar so the master/account distinction is explicit, and verify any other uses in the referenced range still point to the correct symbol.components/BitcoinBlockList.js-39-48 (1)
39-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the same effective admin token for both channels.
When the token exists only in browser storage,
upstreamAdmin.hubAdminTokengets it butfetchExplorerData(..., { adminToken })still receives''. That can break admin-gated explorer calls that read the options token.Proposed fix
const adminTok = (this.props.adminToken != null && String(this.props.adminToken).trim()) ? String(this.props.adminToken).trim() : ''; + const effectiveAdminTok = adminTok || readHubAdminTokenFromBrowser(null) || ''; const upstreamAdmin = { ...this.state.upstream, - hubAdminToken: adminTok || readHubAdminTokenFromBrowser(null) || '' + hubAdminToken: effectiveAdminTok }; const [data, status] = await Promise.all([ - fetchExplorerData(upstreamAdmin, spend, { network: net, adminToken: adminTok }).catch(() => ({})), + fetchExplorerData(upstreamAdmin, spend, { network: net, adminToken: effectiveAdminTok }).catch(() => ({})), fetchBitcoinStatus(upstreamAdmin).catch(() => ({})) ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/BitcoinBlockList.js` around lines 39 - 48, The admin token is resolved into upstreamAdmin.hubAdminToken but fetchExplorerData still receives the raw adminTok value, so browser-stored tokens are not forwarded to the explorer call. Update BitcoinBlockList’s token handling so the same effective token is computed once and passed consistently to both upstreamAdmin and the fetchExplorerData options.adminToken, keeping fetchBitcoinStatus aligned with the same resolved token path.functions/hubUiFeatureFlags.js-189-190 (1)
189-190: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLet persisted settings replace stale local flags.
The header says persisted
{ value }overwrites locals, but merging overloadHubUiFeatureFlags()preserves old browser-only values for any omitted server keys. Normalize the persisted payload directly so omitted keys fall back to bundled defaults.Proposed fix
- const localBefore = loadHubUiFeatureFlags(); - const next = normalizeFlags({ ...localBefore, ...raw }); + const next = normalizeFlags(raw);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/hubUiFeatureFlags.js` around lines 189 - 190, The flag merge in loadHubUiFeatureFlags/normalizeFlags is preserving stale local browser-only values because it spreads loadHubUiFeatureFlags() into raw before normalization. Update this flow so the persisted payload is normalized directly, with omitted keys falling back to bundled defaults rather than any previously saved local state. Keep the fix focused in hubUiFeatureFlags.js around loadHubUiFeatureFlags and the normalizeFlags call.services/hub.js-208-212 (1)
208-212: 🔒 Security & Privacy | 🟡 MinorDefault
exposePaymentTestRouteimplements opt-out posture instead of secure opt-in.The current logic exposes the payment test route unless
FABRIC_HTTP_PAYMENTS_HIDE_TEST_ROUTEis explicitly set, defaulting totruewhen the variable is undefined. Documentation for@fabric/httpindicates this setting should be opt-in (disabled by default) to prevent exposing payment settlement hooks in public hubs. This implementation inadvertently leaves test routes open by default.Refer to the snippet below:
Current logic (services/hub.js:208-212)
exposePaymentTestRoute: !( process.env.FABRIC_HTTP_PAYMENTS_HIDE_TEST_ROUTE === '1' || process.env.FABRIC_HTTP_PAYMENTS_HIDE_TEST_ROUTE === 'true' ),Consider switching to an explicit opt-in approach for production safety.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/hub.js` around lines 208 - 212, The exposePaymentTestRoute setting in hub configuration is using an unsafe opt-out default and should be changed to secure opt-in behavior. Update the payment route flag logic in the hub setup so it is disabled unless explicitly enabled, instead of defaulting to true when FABRIC_HTTP_PAYMENTS_HIDE_TEST_ROUTE is unset. Use the exposePaymentTestRoute configuration block in services/hub.js as the fix point and keep the environment-variable check aligned with the documented default.
🧹 Nitpick comments (6)
package.json (1)
120-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBranch-pinned Fabric deps are non-reproducible.
Pinning
@fabric/coreand@fabric/httptofeature/rsimeansnpm iresolves to whatever the branch HEAD is at install time, so builds are not reproducible and can silently drift. Fine for this WIP PR, but pin to a commit SHA or tag before merging tomaster.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 120 - 121, The Fabric dependencies are pinned to moving branch refs, which makes installs non-reproducible; update the `@fabric/core` and `@fabric/http` entries in package.json to immutable references such as a commit SHA or a release tag before merging. Keep the dependency names the same, but replace the feature/rsi branch targets so future npm i runs resolve to a fixed version.scripts/prepare-hub-dev-port.js (2)
108-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant error branch swallows all probe failures identically.
Both the
ECONNREFUSED/EHOSTUNREACHcase and the fallthroughreturn, so the error-code check has no effect — any OPTIONS failure (timeout, parse, other network error) is treated as "port is free." For a best-effort prestart that's acceptable, but the branch is dead and misleading; consider collapsing it or logging non-connection errors so an unexpectedly-failing sample listener isn't silently ignored.♻️ Optional simplification
try { j = await optionsJson(); } catch (e) { - if (e && (e.code === 'ECONNREFUSED' || e.code === 'EHOSTUNREACH')) { - return; - } + // Nothing reachable / unparseable on the port: treat as free for prestart. return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prepare-hub-dev-port.js` around lines 108 - 116, The error handling in the options probe is redundant because both the ECONNREFUSED/EHOSTUNREACH path and the fallback branch do the same thing. Update the try/catch around optionsJson in prepare-hub-dev-port.js to either collapse the branches into a single best-effort return or, if keeping the distinction, log non-connection failures before returning. Keep the change localized to the optionsJson probe logic so the behavior remains clear and intentional.
63-78: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value
execSyncwith interpolated values — confirm inputs stay numeric.The static analysis flags command injection here. In practice
pisNumber()-coerced andpidcomes from parsedlsofoutput mapped throughNumber(...), so current call sites are safe. To make this robust against future callers and silence the warning, preferexecFileSyncwith an argument array.🛡️ Suggested hardening
-const { execSync } = require('child_process'); +const { execFileSync } = require('child_process'); @@ - const out = execSync(`lsof -nP -iTCP:${p} -sTCP:LISTEN -t`, { encoding: 'utf8' }); + const out = execFileSync('lsof', ['-nP', `-iTCP:${p}`, '-sTCP:LISTEN', '-t'], { encoding: 'utf8' }); @@ - return execSync(`ps -p ${pid} -o args=`, { encoding: 'utf8' }).trim(); + return execFileSync('ps', ['-p', String(pid), '-o', 'args='], { encoding: 'utf8' }).trim();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prepare-hub-dev-port.js` around lines 63 - 78, The `pidsListeningOnPort` and `commandLineForPid` helpers are using `execSync` with interpolated values, which triggers command-injection warnings. Harden these calls by switching to `execFileSync` (or equivalent argument-array execution) in both helpers, keeping the existing numeric parsing in place so `p` and `pid` remain validated inputs. This change should be applied within the `pidsListeningOnPort` and `commandLineForPid` functions in `prepare-hub-dev-port.js`.Source: Linters/SAST tools
tests/fabricHubLocalIdentity.fabricHdRole.test.js (1)
108-115: 📐 Maintainability & Code Quality | 🔵 TrivialRemove unused second argument from
buildLocalFabricIdentityPayloadcall.The function
buildLocalFabricIdentityPayloaddefined infunctions/fabricHubLocalIdentity.jsaccepts only a singleparsedobject. The second argument{ unlockPlaintextMaster: true }is ignored and serves no purpose.Code that should be removed
const bl = buildLocalFabricIdentityPayload( { fabricIdentityMode: 'account', fabricAccountIndex: 2, xpub: dk.xpub }, - { unlockPlaintextMaster: true } );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fabricHubLocalIdentity.fabricHdRole.test.js` around lines 108 - 115, Remove the unused second argument from the `buildLocalFabricIdentityPayload` call in `fabricHubLocalIdentity.fabricHdRole.test.js`; `buildLocalFabricIdentityPayload` in `functions/fabricHubLocalIdentity.js` only accepts the parsed identity object, so update the test to pass just that single object and drop the ignored `{ unlockPlaintextMaster: true }` argument.functions/resolveFabricHttpSend402.js (1)
8-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDynamic
requireis safe here, but the Codacy gate is failing.The argument is built from
require.resolve('@fabric/http/package.json'), so the path is package-controlled, not untrusted input — the warning is a false positive. Since the Codacy check is reported as failing on Line 10, consider adding a scoped suppression so the gate stays green without weakening the analysis elsewhere.♻️ Optional: scoped suppression
try { const root = path.dirname(require.resolve('`@fabric/http/package.json`')); + // codacy:ignore detect-non-literal-require — path derived from resolved `@fabric/http` root module.exports = require(path.join(root, 'functions/sendPaymentRequired402Response.js')); } catch (_) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@functions/resolveFabricHttpSend402.js` around lines 8 - 20, The dynamic require in resolveFabricHttpSend402 is using a package-controlled path from require.resolve('`@fabric/http/package.json`'), so this is a false positive from Codacy. Add a scoped suppression around the require(path.join(root, 'functions/sendPaymentRequired402Response.js')) call (or the minimal surrounding block) so the gate passes without disabling the rule globally, and keep the fallback export for sendPaymentRequired402Response unchanged.Source: Linters/SAST tools
services/hub.js (1)
5691-5703: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueUse a constant-time comparison for the xpub query token.
bearer === required/qTok === required/headerStr === requiredare short-circuiting string comparisons and are timing-observable. The codebase already hastimingSafeSha256Utf8Matchfor secret comparison; reuse it here for parity with the setup-secret path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/hub.js` around lines 5691 - 5703, The xpub query token check in the token-validation path is using plain string equality on bearer, query, and header values, which should be replaced with a constant-time secret comparison. Update the comparison logic in the hub token gate to use timingSafeSha256Utf8Match, following the same pattern used for the setup-secret flow, and keep the existing ok/403 response behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@actions/documentActions.js`:
- Around line 87-89: The document load path is destructuring the auth token
incorrectly, so the value passed into fetchFromAPI is undefined. Update the
token retrieval in documentActions.js to read directly from getState().auth and
pass that token into fetchFromAPI, using the existing load logic around the
document fetch in the action that requests
`/documents/${encodeURIComponent(fabricID)}`.
In `@components/BitcoinHome.js`:
- Around line 316-332: The admin token is being forwarded to non-Hub endpoints
through the bitcoin client helpers, which can leak credentials to external
services. Update the call flow around BitcoinHome’s fetches and the underlying
bitcoinClient functions (fetchBitcoinStatusAtBase, fetchWalletSummary,
fetchUTXOs, and any shared tryRequests/auth token setup) so hubAdminToken is
only used when the destination is the internal Hub RPC endpoint
(/services/bitcoin). For explorerBaseUrl and paymentsBaseUrl, either validate
they are internal before attaching auth or explicitly omit hubAdminToken and
fall back to non-admin auth only.
In `@components/Bridge.js`:
- Around line 4271-4273: The relay filter in Bridge.js is too narrow: the
inventory branch only allows INVENTORY_RESPONSE and
FABRIC_DOCUMENT_OFFER_RESPONSE before calling
isDocumentInventoryDocumentsOfferResponse, so FABRIC_DOCUMENT_OFFER_REPLY never
reaches the merge path. Update the conditional around the relay handling in
Bridge to include all document-offer response aliases accepted by the predicate,
especially FABRIC_DOCUMENT_OFFER_REPLY, so those envelopes are parsed and merged
into peer inventory.
In `@components/HubInterface.js`:
- Around line 984-993: The forget/destroy cleanup paths currently clear
persisted identity data but leave fabric.identity.unlocked in sessionStorage.
Update _handleIdentityManagerForget and the matching destroy cleanup block to
invoke the existing session cleanup helper that removes unlocked session key
material, alongside the current storage removals, so private keys are cleared
consistently.
- Around line 1185-1187: The safety timer in HubInterface.js is marking setup as
checked without also indicating that setup is configured, which can bypass
onboarding when /settings hangs. Update the timeout handler in the setup flow so
that the fallback state does not leave needsSetup at its default false; ensure
the logic around setupChecked and needsSetup in the relevant HubInterface
methods treats an unknown setup status as not configured until the settings
check completes.
- Around line 1019-1022: Account switching is reading the master key only from
stored local identity, so it can no-op when the unlocked session already has the
master key. Update the account-switch flow in HubInterface’s account switching
logic to prefer the unlocked master key from the current identity/session state
(where masterXprv is kept) and only fall back to
plaintextMasterFromStored(parsed) if needed. Keep the
deriveFabricAccountIdentityKeys path and the surrounding switch-account handling
intact, but ensure it can proceed when fabric.identity.local is empty.
In `@components/IdentityManager.js`:
- Around line 1493-1503: The new identity save path in IdentityManager should
not use encryptLocalIdentityAtRest for fresh key material because it relies on
weak unauthenticated encryption. Update the identity persistence flow around the
plaintextPayload/encryptLocalIdentityAtRest/writeStorageJSON block to use an
authenticated scheme like the backup helper’s PBKDF2 + AES-GCM, or strengthen
encryptLocalIdentityAtRest before it is called here. Make sure the fix applies
to the account identity creation path that stores fabric.identity.local so newly
saved identities are protected with the stronger format.
In `@components/SettingsBitcoinWallet.js`:
- Around line 35-37: The “Master xpub” display in SettingsBitcoinWallet is still
using identity.xpub, which can mismatch Fabric account-mode derivation; update
the xpub source in the SettingsBitcoinWallet component to prefer
identity.masterXpub for account-mode identities, with a safe fallback only when
that field is unavailable, so the shown value matches the derivation model
described in the copy.
In `@functions/fabricAccountDerivedIdentity.js`:
- Around line 44-48: The helper fabricBech32IdFromCompressedPubHex currently
trusts Buffer.from(..., "hex"), which can accept malformed input; add strict
validation before hashing. In fabricBech32IdFromCompressedPubHex, verify
compressedPubHex is a trimmed hex string for a compressed secp256k1 pubkey:
exactly 66 hex characters, starts with 02 or 03, and represents 33 bytes, then
only proceed to Hash256.digest and Bech32 conversion. If the input fails
validation, throw a clear error before any Buffer.from processing.
In `@functions/fabricBrowserIdentityDev.js`:
- Around line 139-140: Only clear suppression for the seed currently being
restored, not globally. Update the restore/import flow in
fabricBrowserIdentityDev.js so the wipe marker is tied to the specific mnemonic
or seed being re-enabled, and adjust the call around clearDevSeedSuppression()
accordingly. Use the existing restore/import logic and the seed-handling code
paths in this module to ensure importing a different seed does not re-enable
bootstrap for a previously wiped one.
In `@functions/fabricDesktopLoginVerify.js`:
- Around line 126-140: The desktop login verifier currently falls through to
success when only one of the expected bindings is present, so tighten the guard
in fabricDesktopLoginVerify.js around parseDesktopLoginMessage and
originsMatchForDesktopSession to fail closed unless both sessionId and origin
are supplied. Update the existing expected/session binding check so the helper
returns a rejection error whenever either wantSid or wantOrigin is missing, and
only accepts a signed message after both parsed.sessionId and parsed.origin are
validated against the expected values.
In `@functions/fabricHttpSemantic.js`:
- Around line 78-95: Add a guard in syncSemanticAssetsFromRoot to prevent
copying when fabricHttpRoot and hubRoot resolve to the same assets directory,
since the current fs.rmSync/fs.cpSync flow can delete the source before copying.
Use the existing syncSemanticAssetsFromRoot function and the
sourceAssets/hubAssets paths to detect this self-sync case early and skip or
throw before iterating SEMANTIC_FILES and SEMANTIC_DIRS.
In `@functions/fabricHubLocalIdentity.js`:
- Around line 200-203: The key derivation and encryption in
fabricHubLocalIdentity are too weak and unauthenticated; replace the current
`crypto.createHash('sha256').update(salt + pwd)` flow with a stronger KDF in the
identity encryption/decryption path, and switch the private-key protection logic
to an authenticated mode such as AES-GCM. Update the relevant encrypt/decrypt
helpers and any code that reads or writes `xprvEnc` to store the extra auth data
needed by `aes-256-gcm`, and add a compatibility/migration path so existing
encrypted records can still be decrypted and re-encrypted in the new format.
In `@functions/fabricIdentityBackupCrypto.js`:
- Around line 108-124: Validate and bound the KDF inputs in
fabricIdentityBackupCrypto before calling subtle.deriveBits: in the backup
import path, reject malformed or missing encryptedFile.kdf.salt and
encryptedFile.iv after b64ToU8 conversion, and clamp/validate
encryptedFile.kdf.iterations to a safe numeric range instead of blindly using
the file-provided value. Apply the checks near the existing salt, iv, rawCipher,
and iterations handling in the backup decryption flow so deriveBits only runs
with trusted parameters.
In `@functions/fabricIdentityCapabilities.js`:
- Around line 22-34: `hasMaster` in `fabricIdentityCapabilities` is using
`plaintextMasterFromStored(parsed)`, which is always empty and keeps
`canSwitchFabricAccount` and `canExportFabricAccountSubtreeBackup` disabled.
Update `fabricIdentityCapabilities` to derive `hasMaster` from the in-memory
unlocked state on `parsed` (for example `parsed.masterXprv` or the equivalent
runtime flag) instead of the stored plaintext helper. Keep the
`canSwitchFabricAccount` and `canExportFabricAccountSubtreeBackup` conditions
unchanged aside from the corrected master check so the capability flags reflect
actual device state.
In `@functions/hubCollaboration.js`:
- Around line 22-26: The fixed ID validators in hubCollaboration.js are using
dynamic new RegExp(...) calls, which triggers the static check. Replace the
CONTACT_ID_RE, INVITATION_ID_RE, and GROUP_ID_RE definitions with equivalent
literal regex expressions while keeping the same matching behavior, and leave
COLLAB_ID_SUFFIX_HEX_LEN only if it is still needed elsewhere.
In `@functions/hubPublicVisitor.js`:
- Around line 40-49: `computePublicHubVisitor` is missing a persisted-identity
check, so a stored locked/watch-only Fabric identity can still be classified as
a public visitor before `localIdentity` hydrates. Update the decision flow in
`computePublicHubVisitor` to consult `hasPersistedFabricIdentity()` alongside
the existing `hasUnlockedHubSigningIdentity`, `localIdentity`, and
`hasExternalSigningDelegation` checks, and return false whenever a persisted
identity is present even if `localIdentity` is not yet populated.
In `@scripts/build.js`:
- Around line 43-52: The Semantic asset sync in syncSemanticAssetsFromFabricHttp
can crash the build when `@fabric/http` is a source-only checkout because
sourceRoot may point to a directory without assets/ and
syncSemanticAssetsFromRoot then throws. Update scripts/build.js to handle this
path gracefully by either wrapping the syncSemanticAssetsFromRoot call in a
try/catch and logging/skipping on failure, or by matching the source-checkout
handling used in buildSemanticAssets/runBuildSemantic so source-only installs
build assets before syncing. Use the existing syncSemanticAssetsFromFabricHttp
and main flow as the fix point.
In `@services/setup.js`:
- Around line 226-230: Update verifyAdminToken in setup.js so it no longer
treats any token signed by _rootKey as admin access. Keep using
Token.verifySigned, but capture its returned payload and only return true when
the payload is non-null and its capability is OP_IDENTITY with subject equal to
admin; otherwise return false. Use verifyAdminToken, Token.verifySigned, and
_rootKey as the key symbols when making the change.
---
Outside diff comments:
In `@actions/documentActions.js`:
- Around line 109-117: The upload timeout is not working because fetchPromise is
awaited before Promise.race, so a slow or hung request cannot be interrupted.
Update the upload flow in documentActions.js so
fetch(assertClientFetchPath('/files'), ...) is passed directly into Promise.race
alongside timeoutPromise, and keep the existing fileCreation handling in the
upload routine.
- Around line 191-204: The edit flow in editDocument is dispatching
editDocumentSuccess even when the PATCH request fails, so add a response.ok (or
equivalent status check) before parsing and dispatching. In the action that
calls fetch for /documents/${encodeURIComponent(fabricID)}, branch on failure to
stop the success path and handle the error state instead, then only call
editDocumentSuccess(document) when the response is successful.
In `@components/IdentityManager.js`:
- Around line 1903-1925: The import flow in IdentityManager is treating
xprv-backed identities like watch-only entries by writing only the xpub and
marking nextIdentity as passwordProtected false. Update the import path in the
local-storage block and the nextIdentity construction so xprv imports require a
local encryption password and are persisted through the encrypted identity flow
instead of fabric.identity.local. Make sure the logic around xprv,
writeStorageJSON, and nextIdentity keeps the private key protected and
reloadable.
- Around line 397-407: The sync payload in IdentityManager’s local identity
persistence is writing unlocked private key material directly to extension
storage. Update the logic around the localIdentity payload build so it does not
include xprv or masterXprv in the chrome.storage.local sync path; keep this flow
watch-only unless the key material is first encrypted using the existing
encrypted-at-rest mechanism for fabric.identity.local. Verify the payload
construction in the local identity save/sync branch only carries public or
non-sensitive fields.
In `@functions/bitcoinClient.js`:
- Around line 1246-1262: The fallback cache lookup in
fetchWalletSummaryWithCache ignores the computed cache policy, so bypassCache
and maxCacheAgeMs are not respected after fetchWalletSummary fails. Update the
cache read path in fetchWalletSummaryWithCache to use the derived maxCacheAgeMs
(or bypassCache) instead of always calling getCachedBalance with Infinity, so
explicit refresh requests cannot return stale balances.
- Around line 1623-1626: The crowdfunding key derivation is still hardcoded to
account 0, so beneficiary/refund keys can drift from the active Fabric wallet
account. Update the crowdfunding derivation in bitcoinClient.js to use the
selected identity account via getBitcoinBip44AccountForIdentity(identity)
instead of BITCOIN_PAYMENTS_BIP44_ACCOUNT_INDEX, and apply the same change in
the related beneficiary/refund key path so deriveFabricBitcoinAccountKeys stays
aligned with the active account.
In `@scripts/desktop.js`:
- Around line 160-213: The waitForHub polling in ping can hang when http.get
connects but never finishes the response, because the existing timeout only runs
in the end and error paths. Add a per-request timeout on the req object in ping
so stalled requests are destroyed and routed through the existing
req.on('error') retry logic, ensuring the overall deadline in waitForHub is
still enforced.
---
Minor comments:
In `@assets/scripts/assets/manifest.json`:
- Around line 6-8: The manifest has a duplicate asset mapping where main.js
points to the same CDN source and integrity as index.min.js, so remove the
redundant main.js entry from the assets manifest. Update the manifest object in
assets/scripts/assets/manifest.json so only the distinct entry remains, and keep
the existing index.min.js mapping unchanged.
In `@components/BitcoinBlockList.js`:
- Around line 39-48: The admin token is resolved into
upstreamAdmin.hubAdminToken but fetchExplorerData still receives the raw
adminTok value, so browser-stored tokens are not forwarded to the explorer call.
Update BitcoinBlockList’s token handling so the same effective token is computed
once and passed consistently to both upstreamAdmin and the fetchExplorerData
options.adminToken, keeping fetchBitcoinStatus aligned with the same resolved
token path.
In `@components/BitcoinWalletBranchBar.js`:
- Around line 12-13: The “Master xpub” label is currently using the selected
account xpub from BitcoinWalletBranchBar, so update the xpub source to use
identity.masterXpub for that label and keep identity.xpub only for the
account-level display. Adjust the related rendering logic in
BitcoinWalletBranchBar so the master/account distinction is explicit, and verify
any other uses in the referenced range still point to the correct symbol.
In `@functions/fabricBrowserState.js`:
- Around line 235-238: The unlock-state cleanup in fabricBrowserState should use
the resolved browser global consistently instead of falling back to window.
Update the sessionStorage removal inside the unlock cleanup logic to reference
the same browser-global variable used elsewhere in the function (for example,
the one returned by getFabricBrowserGlobal and stored in w), so the identity key
is cleared even when a provided browser global is resolved.
In `@functions/hubUiFeatureFlags.js`:
- Around line 189-190: The flag merge in loadHubUiFeatureFlags/normalizeFlags is
preserving stale local browser-only values because it spreads
loadHubUiFeatureFlags() into raw before normalization. Update this flow so the
persisted payload is normalized directly, with omitted keys falling back to
bundled defaults rather than any previously saved local state. Keep the fix
focused in hubUiFeatureFlags.js around loadHubUiFeatureFlags and the
normalizeFlags call.
In `@services/hub.js`:
- Around line 208-212: The exposePaymentTestRoute setting in hub configuration
is using an unsafe opt-out default and should be changed to secure opt-in
behavior. Update the payment route flag logic in the hub setup so it is disabled
unless explicitly enabled, instead of defaulting to true when
FABRIC_HTTP_PAYMENTS_HIDE_TEST_ROUTE is unset. Use the exposePaymentTestRoute
configuration block in services/hub.js as the fix point and keep the
environment-variable check aligned with the documented default.
---
Nitpick comments:
In `@functions/resolveFabricHttpSend402.js`:
- Around line 8-20: The dynamic require in resolveFabricHttpSend402 is using a
package-controlled path from require.resolve('`@fabric/http/package.json`'), so
this is a false positive from Codacy. Add a scoped suppression around the
require(path.join(root, 'functions/sendPaymentRequired402Response.js')) call (or
the minimal surrounding block) so the gate passes without disabling the rule
globally, and keep the fallback export for sendPaymentRequired402Response
unchanged.
In `@package.json`:
- Around line 120-121: The Fabric dependencies are pinned to moving branch refs,
which makes installs non-reproducible; update the `@fabric/core` and `@fabric/http`
entries in package.json to immutable references such as a commit SHA or a
release tag before merging. Keep the dependency names the same, but replace the
feature/rsi branch targets so future npm i runs resolve to a fixed version.
In `@scripts/prepare-hub-dev-port.js`:
- Around line 108-116: The error handling in the options probe is redundant
because both the ECONNREFUSED/EHOSTUNREACH path and the fallback branch do the
same thing. Update the try/catch around optionsJson in prepare-hub-dev-port.js
to either collapse the branches into a single best-effort return or, if keeping
the distinction, log non-connection failures before returning. Keep the change
localized to the optionsJson probe logic so the behavior remains clear and
intentional.
- Around line 63-78: The `pidsListeningOnPort` and `commandLineForPid` helpers
are using `execSync` with interpolated values, which triggers command-injection
warnings. Harden these calls by switching to `execFileSync` (or equivalent
argument-array execution) in both helpers, keeping the existing numeric parsing
in place so `p` and `pid` remain validated inputs. This change should be applied
within the `pidsListeningOnPort` and `commandLineForPid` functions in
`prepare-hub-dev-port.js`.
In `@services/hub.js`:
- Around line 5691-5703: The xpub query token check in the token-validation path
is using plain string equality on bearer, query, and header values, which should
be replaced with a constant-time secret comparison. Update the comparison logic
in the hub token gate to use timingSafeSha256Utf8Match, following the same
pattern used for the setup-secret flow, and keep the existing ok/403 response
behavior intact.
In `@tests/fabricHubLocalIdentity.fabricHdRole.test.js`:
- Around line 108-115: Remove the unused second argument from the
`buildLocalFabricIdentityPayload` call in
`fabricHubLocalIdentity.fabricHdRole.test.js`; `buildLocalFabricIdentityPayload`
in `functions/fabricHubLocalIdentity.js` only accepts the parsed identity
object, so update the test to pass just that single object and drop the ignored
`{ unlockPlaintextMaster: true }` argument.
🪄 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: b14ac3d3-dcdc-4a38-9b3d-6f703884d9dd
⛔ Files ignored due to path filters (92)
assets/bundles/browser.min.jsis excluded by!**/*.min.jsassets/scripts/semantic.min.jsis excluded by!**/*.min.jsassets/semantic.min.jsis excluded by!**/*.min.jsassets/styles/themes/default/assets/fonts/RobotoMono-Bold.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-BoldItalic.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-ExtraLight.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-ExtraLightItalic.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-Italic.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-Light.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-LightItalic.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-Medium.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-MediumItalic.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-Regular.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-SemiBold.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-SemiBoldItalic.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-Thin.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/RobotoMono-ThinItalic.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/brand-icons.eotis excluded by!**/*.eotassets/styles/themes/default/assets/fonts/brand-icons.svgis excluded by!**/*.svgassets/styles/themes/default/assets/fonts/brand-icons.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/brand-icons.woffis excluded by!**/*.woffassets/styles/themes/default/assets/fonts/brand-icons.woff2is excluded by!**/*.woff2assets/styles/themes/default/assets/fonts/icons.eotis excluded by!**/*.eotassets/styles/themes/default/assets/fonts/icons.svgis excluded by!**/*.svgassets/styles/themes/default/assets/fonts/icons.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/icons.woffis excluded by!**/*.woffassets/styles/themes/default/assets/fonts/icons.woff2is excluded by!**/*.woff2assets/styles/themes/default/assets/fonts/outline-icons.eotis excluded by!**/*.eotassets/styles/themes/default/assets/fonts/outline-icons.svgis excluded by!**/*.svgassets/styles/themes/default/assets/fonts/outline-icons.ttfis excluded by!**/*.ttfassets/styles/themes/default/assets/fonts/outline-icons.woffis excluded by!**/*.woffassets/styles/themes/default/assets/fonts/outline-icons.woff2is excluded by!**/*.woff2assets/styles/themes/default/assets/images/flags.pngis excluded by!**/*.pngassets/themes/default/assets/fonts/Lato-Bold.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/Lato-Bold.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/Lato-BoldItalic.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/Lato-BoldItalic.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/Lato-Italic.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/Lato-Italic.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/Lato-Regular.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/Lato-Regular.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/LatoLatin-Bold.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/LatoLatin-Bold.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/LatoLatin-BoldItalic.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/LatoLatin-BoldItalic.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/LatoLatin-Italic.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/LatoLatin-Italic.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/LatoLatin-Regular.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/LatoLatin-Regular.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/brand-icons.eotis excluded by!**/*.eotassets/themes/default/assets/fonts/brand-icons.svgis excluded by!**/*.svgassets/themes/default/assets/fonts/brand-icons.ttfis excluded by!**/*.ttfassets/themes/default/assets/fonts/brand-icons.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/brand-icons.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/icons.eotis excluded by!**/*.eotassets/themes/default/assets/fonts/icons.svgis excluded by!**/*.svgassets/themes/default/assets/fonts/icons.ttfis excluded by!**/*.ttfassets/themes/default/assets/fonts/icons.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/icons.woff2is excluded by!**/*.woff2assets/themes/default/assets/fonts/outline-icons.eotis excluded by!**/*.eotassets/themes/default/assets/fonts/outline-icons.svgis excluded by!**/*.svgassets/themes/default/assets/fonts/outline-icons.ttfis excluded by!**/*.ttfassets/themes/default/assets/fonts/outline-icons.woffis excluded by!**/*.woffassets/themes/default/assets/fonts/outline-icons.woff2is excluded by!**/*.woff2assets/themes/default/assets/images/flags.pngis excluded by!**/*.pngassets/themes/fabric/assets/fonts/Arvo.woff2is excluded by!**/*.woff2assets/themes/fabric/assets/fonts/arvo-italic-400.ttfis excluded by!**/*.ttfassets/themes/fabric/assets/fonts/arvo-italic-400.woff2is excluded by!**/*.woff2assets/themes/fabric/assets/fonts/arvo-italic-700.ttfis excluded by!**/*.ttfassets/themes/fabric/assets/fonts/arvo-italic-700.woff2is excluded by!**/*.woff2assets/themes/fabric/assets/fonts/arvo-normal-400.ttfis excluded by!**/*.ttfassets/themes/fabric/assets/fonts/arvo-normal-400.woff2is excluded by!**/*.woff2assets/themes/fabric/assets/fonts/arvo-normal-700.ttfis excluded by!**/*.ttfassets/themes/fabric/assets/fonts/arvo-normal-700.woff2is excluded by!**/*.woff2assets/themes/fabric/assets/fonts/brand-icons.eotis excluded by!**/*.eotassets/themes/fabric/assets/fonts/brand-icons.svgis excluded by!**/*.svgassets/themes/fabric/assets/fonts/brand-icons.ttfis excluded by!**/*.ttfassets/themes/fabric/assets/fonts/brand-icons.woffis excluded by!**/*.woffassets/themes/fabric/assets/fonts/brand-icons.woff2is excluded by!**/*.woff2assets/themes/fabric/assets/fonts/icons.eotis excluded by!**/*.eotassets/themes/fabric/assets/fonts/icons.svgis excluded by!**/*.svgassets/themes/fabric/assets/fonts/icons.ttfis excluded by!**/*.ttfassets/themes/fabric/assets/fonts/icons.woffis excluded by!**/*.woffassets/themes/fabric/assets/fonts/icons.woff2is excluded by!**/*.woff2assets/themes/fabric/assets/fonts/outline-icons.eotis excluded by!**/*.eotassets/themes/fabric/assets/fonts/outline-icons.svgis excluded by!**/*.svgassets/themes/fabric/assets/fonts/outline-icons.ttfis excluded by!**/*.ttfassets/themes/fabric/assets/fonts/outline-icons.woffis excluded by!**/*.woffassets/themes/fabric/assets/fonts/outline-icons.woff2is excluded by!**/*.woff2assets/themes/fabric/assets/images/flags.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.jsonreports/install.logis excluded by!**/*.log
📒 Files selected for processing (108)
.codacy.yamlDEVELOPERS.mdactions/apiActions.jsactions/bridgeActions.jsactions/chatActions.jsactions/documentActions.jsassets/index.htmlassets/scripts/assets/manifest.jsonassets/scripts/semantic.jsassets/semantic.cssassets/semantic.jsassets/semantic.min.cssassets/semantic.rtl.cssassets/semantic.rtl.min.cssassets/styles/semantic.min.cssassets/themes/default/assets/fonts/LICENSE_Lato.txtassets/themes/default/assets/fonts/LICENSE_icons.txtcomponents/AdminHome.jscomponents/BeaconAdminPanel.jscomponents/BitcoinBlockList.jscomponents/BitcoinHome.jscomponents/BitcoinPaymentsHome.jscomponents/BitcoinResourcesHome.jscomponents/BitcoinTransactionsHome.jscomponents/BitcoinWalletBranchBar.jscomponents/Bridge.jscomponents/Dashboard.jscomponents/FaucetHome.jscomponents/HubInterface.jscomponents/IdentityManager.jscomponents/Onboarding.jscomponents/PeerList.jscomponents/PeerView.jscomponents/SettingsBitcoinWallet.jscomponents/TopPanel.jscomponents/fabricIdentity/FabricHubAdminTokenNotice.jscomponents/fabricIdentity/FabricIdentityAccountControls.jscomponents/fabricIdentity/FabricPostSetupIdentityWizard.jsfunctions/bitcoinClient.jsfunctions/fabricAccountDerivedIdentity.jsfunctions/fabricBrowserIdentityDev.jsfunctions/fabricBrowserState.jsfunctions/fabricBrowserStore.jsfunctions/fabricDesktopAuth.jsfunctions/fabricDesktopLoginVerify.jsfunctions/fabricDocumentOfferEnvelope.jsfunctions/fabricHttpSemantic.jsfunctions/fabricHubLocalIdentity.jsfunctions/fabricIdentityBackupCrypto.jsfunctions/fabricIdentityCapabilities.jsfunctions/fabricIdentityLockPrefs.jsfunctions/fabricMessageRegistry.jsfunctions/fabricPostSetupBrowserIdentity.jsfunctions/fabricProtocolUrl.jsfunctions/httpSpaShell.jsfunctions/hubAdminTokenBrowser.jsfunctions/hubCollaboration.jsfunctions/hubPublicVisitor.jsfunctions/hubUiFeatureFlags.jsfunctions/patchLinkedFabricNodePath.jsfunctions/resolveFabricHttpSend402.jsfunctions/sampleHubOptions.jslibraries/semantic/src/themes/fabric/globals/site.variablespackage.jsonreducers/documentReducer.jsroutes/contracts/create_contract.jsroutes/documents/create_document.jsroutes/documents/view_document.jsroutes/index.jsroutes/messages/create_message.jsroutes/messages/list_messages.jsroutes/messages/view_message.jsroutes/peers/create_peer.jsroutes/peers/list_peers.jsroutes/peers/view_peer.jsscripts/build-semantic-assets.jsscripts/build.jsscripts/desktop-preload.jsscripts/desktop.jsscripts/desktopHubProbe.jsscripts/download-fonts.jsscripts/hub.jsscripts/prepare-hub-dev-port.jsservices/hub.jsservices/setup.jssettings/local.jstests/bitcoinClient.bip44TabAccount.test.jstests/bridge.webrtc.signal.test.jstests/browser.hubAlerts.test.jstests/browser.interface.test.jstests/browser.onboardingPeers.e2e.test.jstests/desktopHubProbe.test.jstests/electron.desktopHubProbe.test.jstests/fabricBrowserIdentityClear.test.jstests/fabricBrowserIdentityDev.test.jstests/fabricDesktopLoginVerify.test.jstests/fabricDocumentOfferEnvelope.test.jstests/fabricHubLocalIdentity.fabricHdRole.test.jstests/fabricIdentityBackupCrypto.test.jstests/fabricIdentityCapabilities.test.jstests/fabricIdentityLockPrefs.test.jstests/formativeVision.test.jstests/hub.collaboration.test.jstests/hub.http.jstests/hubPublicVisitor.test.jstests/hubUiFeatureFlags.test.jstypes/spa.jswebpack.config.js
💤 Files with no reviewable changes (11)
- routes/documents/create_document.js
- assets/themes/default/assets/fonts/LICENSE_Lato.txt
- routes/peers/view_peer.js
- routes/contracts/create_contract.js
- assets/themes/default/assets/fonts/LICENSE_icons.txt
- routes/peers/create_peer.js
- scripts/download-fonts.js
- routes/messages/list_messages.js
- routes/peers/list_peers.js
- routes/messages/create_message.js
- routes/messages/view_message.js
Comment on lines
+43
to
+52
| function syncSemanticAssetsFromFabricHttp () { | ||
| const root = path.join(__dirname, '..'); | ||
| const roots = resolveFabricHttpRoots(root); | ||
| const sourceRoot = roots.withAssets || roots.withSources; | ||
| if (!sourceRoot) { | ||
| console.warn('[BUILD:SITE] @fabric/http assets not found, skipping Semantic asset sync.'); | ||
| return; | ||
| } | ||
| syncSemanticAssetsFromRoot(sourceRoot, root); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm withSources is keyed on gulpfile (not assets) and that build.js never rebuilds.
rg -nP 'hasFomanticSources|hasSemanticAssets|withSources|withAssets' functions/fabricHttpSemantic.js -C1
rg -nP 'runBuildSemantic' scripts/build.jsRepository: FabricLabs/hub.fabric.pub
Length of output: 683
🏁 Script executed:
#!/bin/bash
# Verify syncSemanticAssetsFromRoot behavior on missing assets and main() error handling
cat -n scripts/build.js | sed -n '30,70p'
echo "--- Sync Semantic Assets From Root implementation ---"
rg -A 10 "function syncSemanticAssetsFromRoot" scripts/functionalities/semanticSync.js 2>/dev/null || rg -A 10 "function syncSemanticAssetsFromRoot" scripts/build.js
echo "--- Check build-semantic-assets.js logic for comparison ---"
rg -A 15 "resolveFabricHttpRoots|syncSemanticAssetsFromRoot" scripts/build-semantic-assets.js | head -30Repository: FabricLabs/hub.fabric.pub
Length of output: 2793
🏁 Script executed:
#!/bin/bash
# Inspect syncSemanticAssetsFromRoot to confirm throwing behavior and import source
rg -B 5 -A 20 "function syncSemanticAssetsFromRoot" functions/fabricHttpSemantic.js
echo "--- Check imports in build.js and build-semantic-assets.js ---"
head -20 scripts/build.js
head -20 scripts/build-semantic-assets.jsRepository: FabricLabs/hub.fabric.pub
Length of output: 2171
Critical failure in build process if @fabric/http is installed in source-only mode.
When @fabric/http is present as a source checkout (detected via gulpfile.js) without pre-built assets, syncSemanticAssetsFromFabricHttp sets sourceRoot to that source directory. Since sourceRoot lacks the assets/ directory, syncSemanticAssetsFromRoot throws an error. Unlike scripts/build-semantic-assets.js, scripts/build.js does not invoke runBuildSemantic for source checkouts, nor does it wrap the sync in a try/catch block inside main(), causing the entire build to crash.
Required fix:
- Add a
try/catchblock around thesyncSemanticAssetsFromRootcall to fail gracefully, OR - Replicate the logic in
scripts/build-semantic-assets.jsto detect source-only checkouts and invokerunBuildSemanticbefore syncing.
🛠️ Current failing code path
function syncSemanticAssetsFromFabricHttp () {
const root = path.join(__dirname, '..');
const roots = resolveFabricHttpRoots(root);
const sourceRoot = roots.withAssets || roots.withSources;
if (!sourceRoot) {
console.warn('[BUILD:SITE] `@fabric/http` assets not found, skipping Semantic asset sync.');
return;
}
- syncSemanticAssetsFromRoot(sourceRoot, root);
+ try {
+ syncSemanticAssetsFromRoot(sourceRoot, root);
+ } catch (err) {
+ console.warn(`[BUILD:SITE] Skipping Semantic asset sync: ${err && err.message ? err.message : err}`);
+ return;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function syncSemanticAssetsFromFabricHttp () { | |
| const root = path.join(__dirname, '..'); | |
| const roots = resolveFabricHttpRoots(root); | |
| const sourceRoot = roots.withAssets || roots.withSources; | |
| if (!sourceRoot) { | |
| console.warn('[BUILD:SITE] @fabric/http assets not found, skipping Semantic asset sync.'); | |
| return; | |
| } | |
| syncSemanticAssetsFromRoot(sourceRoot, root); | |
| } | |
| function syncSemanticAssetsFromFabricHttp () { | |
| const root = path.join(__dirname, '..'); | |
| const roots = resolveFabricHttpRoots(root); | |
| const sourceRoot = roots.withAssets || roots.withSources; | |
| if (!sourceRoot) { | |
| console.warn('[BUILD:SITE] `@fabric/http` assets not found, skipping Semantic asset sync.'); | |
| return; | |
| } | |
| try { | |
| syncSemanticAssetsFromRoot(sourceRoot, root); | |
| } catch (err) { | |
| console.warn(`[BUILD:SITE] Skipping Semantic asset sync: ${err && err.message ? err.message : err}`); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build.js` around lines 43 - 52, The Semantic asset sync in
syncSemanticAssetsFromFabricHttp can crash the build when `@fabric/http` is a
source-only checkout because sourceRoot may point to a directory without assets/
and syncSemanticAssetsFromRoot then throws. Update scripts/build.js to handle
this path gracefully by either wrapping the syncSemanticAssetsFromRoot call in a
try/catch and logging/skipping on failure, or by matching the source-checkout
handling used in buildSemanticAssets/runBuildSemantic so source-only installs
build assets before syncing. Use the existing syncSemanticAssetsFromFabricHttp
and main flow as the fix point.
Comment on lines
226
to
230
| verifyAdminToken (bearerToken) { | ||
| if (!bearerToken || typeof bearerToken !== 'string') return false; | ||
| if (this._rootKey && Token.verifySigned(bearerToken, this._rootKey) !== null) return true; | ||
| if (!this.fs) return false; | ||
| const raw = this.fs.readFile(ADMIN_TOKEN_FILE); | ||
| if (!raw) return false; | ||
| try { | ||
| const stored = typeof raw === 'string' ? JSON.parse(raw) : JSON.parse(raw.toString('utf8')); | ||
| const storedToken = stored && stored.token; | ||
| if (!storedToken) return false; | ||
| const a = Buffer.from(bearerToken, 'utf8'); | ||
| const b = Buffer.from(storedToken, 'utf8'); | ||
| if (a.length !== b.length) return false; | ||
| return crypto.timingSafeEqual(a, b); | ||
| } catch { | ||
| return false; | ||
| } | ||
| if (!this._rootKey) return false; | ||
| return Token.verifySigned(bearerToken, this._rootKey) !== null; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether Token.verifySigned enforces expiry and exposes capability/subject.
fd token.js -p node_modules/@fabric/core/types --exec sed -n '1,200p' {}Repository: FabricLabs/hub.fabric.pub
Length of output: 282
Enforce capability and subject validation in verifyAdminToken.
The current implementation relies solely on Token.verifySigned returning a non-null value, which confirms signature validity but does not restrict the token's scope. Since _rootKey is used to mint various tokens (e.g., this._fabric.token), any token signed by this key—regardless of its capability or subject—would currently grant access.
You must decode the token payload returned by verifySigned and assert that capability === 'OP_IDENTITY' and subject === 'admin' before granting access.
verifyAdminToken (bearerToken) {
if (!bearerToken || typeof bearerToken !== 'string') return false;
if (!this._rootKey) return false;
const payload = Token.verifySigned(bearerToken, this._rootKey);
return payload !== null &&
payload.capability === 'OP_IDENTITY' &&
payload.subject === 'admin';
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/setup.js` around lines 226 - 230, Update verifyAdminToken in
setup.js so it no longer treats any token signed by _rootKey as admin access.
Keep using Token.verifySigned, but capture its returned payload and only return
true when the payload is non-null and its capability is OP_IDENTITY with subject
equal to admin; otherwise return false. Use verifyAdminToken,
Token.verifySigned, and _rootKey as the key symbols when making the change.
martindale
marked this pull request as ready for review
June 25, 2026 14:14
martindale
commented
Jun 25, 2026
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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
1 participant
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.
Initial sketches for ARC support.
Summary by CodeRabbit
scantxoutset-based endpoints) and expanded document-market/feature-flag presets.scantxoutsetrequests and operator setup secret verification.